Initial asymmetric codec support in MediaSessionDescription

webrtc/pc/mediasession.cc

 /*
  *  Copyright 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1012 matching lines @@
     return false;
   }
 
   if (!AddStreamParams(answer->type(), options, current_streams, answer,
                        add_legacy_stream)) {
     return false;  // Something went seriously wrong.
   }
 
   // Make sure the answer media content direction is per default set as
   // described in RFC3264 section 6.1.
+  const bool should_send =
+      !IsRtpProtocol(answer->protocol()) || !answer->streams().empty();

[pthatcher1, 2016/05/31 21:52:36]

This would be more clear as:

const bool is_data = !IsRtpProtocol(answer->protocol());
const bool has_send_streams = !answer->streams().empty();
const bool wants_send = has_send_streams || is_data;

const bool recv_video = (answer->type() == cricket::MEDIA_TYPE_VIDEO &&
options.recv_video);
const bool recv_audio = (answer->type() == cricket::MEDIA_TYPE_AUDIO &&
options.recv_audio);
const bool wants_recv = recv_audio || recv_video || is_data;

[ossu, 2016/06/02 13:28:02]

Acknowledged.

+  const bool should_receive =
+      answer->type() == cricket::MEDIA_TYPE_VIDEO ? options.recv_video :
+      (answer->type() == cricket::MEDIA_TYPE_AUDIO ? options.recv_audio : true);

[ossu, 2016/05/25 11:26:14]

I expect that since there's no flag for data, we'll always want to receive if
asked. This should equal to how it used to work before checking the recv_*
flags.

+
   switch (offer->direction()) {
     case MD_INACTIVE:
       answer->set_direction(MD_INACTIVE);
       break;
     case MD_SENDONLY:
-      answer->set_direction(MD_RECVONLY);
+      answer->set_direction(should_receive ? MD_RECVONLY : MD_INACTIVE);
       break;
     case MD_RECVONLY:
-      answer->set_direction(IsRtpProtocol(answer->protocol()) &&
-                                    answer->streams().empty()
-                                ? MD_INACTIVE
-                                : MD_SENDONLY);
+      answer->set_direction(should_send ? MD_SENDONLY : MD_INACTIVE);
       break;
     case MD_SENDRECV:
-      answer->set_direction(IsRtpProtocol(answer->protocol()) &&
-                                    answer->streams().empty()
-                                ? MD_RECVONLY
-                                : MD_SENDRECV);
+      answer->set_direction(should_send
+                            ? (should_receive ? MD_SENDRECV : MD_SENDONLY)
+                            : (should_receive ? MD_RECVONLY : MD_INACTIVE));
       break;

[pthatcher1, 2016/05/31 21:52:36]

I think this would be more clear if we made an RtpTransceiverDirection struct
(RtpTransceiverDirection is the enum recently introduced into the standard):

struct RtpTransceiverDirection {
  bool send;
  bool recv;  
  
  RtpTransceiverDirection(bool send, bool recv) : send(send), recv(recv) {}

  bool operator==(const RtpTransceiverDirection& o) const {
    return send == o.send && recv == o.recv; 
  }

  bool operator!=(const RtpTransceiverDirection& o) const {
    return !(*this == 0);
  }

  static RtpTransceiverDirection FromMediaContentDirection(MediaContentDirection
md) {
    bool send = (md == MD_SENDRECV || md == MD_SENDONLY);
    bool recv = (md == MD_SENDRECV || md == MD_RECVONLY);
    return RtpTransceiverDirection(send, recv);   
  }

  MediaContentDescription ToMediaContentDescription() {
    if (send && recv) {
      return MD_SENDRECV;
    }
    if (send) {
      return MD_SENDONLY;
    }
    if (recv) {
      return MD_RECVONLY;
    }
    return MD_INACTIVE;  
  }
}

RtpTransceiverDirection NegotiateRtpTransceiverDirection(RtpTransceiverDirection
offer, RtpTranscevierDirection wants) {
  return RtpTransceiverDirection(offer.send && wants_to.send, offer.recv &&
wants_to.recv);
}

The, finally, the code in question could just be:
auto offer_rtd =
RtpTransceiverDirection::FromMediaContentDirection(offer->direction());
auto wants_rtd = RtpTransceiverDirection(wants_send, wants_recv);
answer->set_direction(NegotiateRtpTranscevierDirection(offer_rtd,
wants_rtd).ToMediaContentDirection());


Eventually, we could even replace MediaContentDescription with it (hopefully we
can make a global RTD_SENDRECV = RtpTransceiverDirection(true, true) without a
static initializer).

[ossu, 2016/06/02 13:28:02]

I've put an RtpTranscieverDirection in and changed to using it. I also updated
the offer side of audio to use it, rather than replicating the logic there.

     default:
       RTC_DCHECK(false && "MediaContentDescription has unexpected direction.");
       break;
   }
 
   return true;
 }
 
 static bool IsDtlsRtp(const std::string& protocol) {
   // Most-likely values first.
@@ skipping 104 matching lines @@
     case MEDIA_TYPE_DATA:
       type_str = "data";
       break;
     default:
       ASSERT(false);
       break;
   }
   return type_str;
 }
 
+std::string MediaContentDirectionToString(MediaContentDirection direction) {
+  switch (direction) {
+    case MD_INACTIVE:
+      return "inactive";
+    case MD_SENDONLY:
+      return "sendonly";
+    case MD_RECVONLY:
+      return "recvonly";
+    case MD_SENDRECV:
+      return "sendrecv";
+    default:
+      ASSERT(false);
+      break;
+  }
+}
+
 void MediaSessionOptions::AddSendStream(MediaType type,
                                     const std::string& id,
                                     const std::string& sync_label) {
   AddSendStreamInternal(type, id, sync_label, 1);
 }
 
 void MediaSessionOptions::AddSendVideoStream(
     const std::string& id,
     const std::string& sync_label,
     int num_sim_layers) {
@@ skipping 41 matching lines @@
       add_legacy_(true),
       transport_desc_factory_(transport_desc_factory) {
 }
 
 MediaSessionDescriptionFactory::MediaSessionDescriptionFactory(
     ChannelManager* channel_manager,
     const TransportDescriptionFactory* transport_desc_factory)
     : secure_(SEC_DISABLED),
       add_legacy_(true),
       transport_desc_factory_(transport_desc_factory) {
-  channel_manager->GetSupportedAudioCodecs(&audio_codecs_);
+  channel_manager->GetSupportedAudioCodecs(&audio_sendrecv_codecs_);
   channel_manager->GetSupportedAudioRtpHeaderExtensions(&audio_rtp_extensions_);
   channel_manager->GetSupportedVideoCodecs(&video_codecs_);
   channel_manager->GetSupportedVideoRtpHeaderExtensions(&video_rtp_extensions_);
   channel_manager->GetSupportedDataCodecs(&data_codecs_);
+  audio_send_codecs_ = audio_sendrecv_codecs_;
+  audio_recv_codecs_ = audio_sendrecv_codecs_;
+}
+
+const AudioCodecs& MediaSessionDescriptionFactory::audio_codecs() const {

[pthatcher1, 2016/05/31 21:52:35]

This could be renamed to audio_sendrecv_codecs().

+  return audio_sendrecv_codecs_;
+}
+
+const AudioCodecs& MediaSessionDescriptionFactory::audio_send_codecs() const {
+  return audio_send_codecs_;
+}
+
+const AudioCodecs& MediaSessionDescriptionFactory::audio_recv_codecs() const {
+  return audio_recv_codecs_;
+}
+
+void MediaSessionDescriptionFactory::set_audio_codecs(
+    const AudioCodecs& send_and_recv_codecs) {
+  audio_send_codecs_ = send_and_recv_codecs;
+  audio_recv_codecs_ = send_and_recv_codecs;
+  audio_sendrecv_codecs_ = send_and_recv_codecs;
+}
+
+void MediaSessionDescriptionFactory::set_audio_codecs(
+    const AudioCodecs& send_codecs, const AudioCodecs& recv_codecs) {
+  audio_send_codecs_ = send_codecs;
+  audio_recv_codecs_ = recv_codecs;
+  audio_sendrecv_codecs_.clear();
+
+  // Intersect the two lists of codecs, preserving the order of the send codecs.
+  // If there's any difference in priorities, chances are encoding is more
+  // expensive than decoding, and high-priority send codecs are likely handled
+  // better (e.g. through hardware encoder support) than low-priority ones.
+  // TODO(ossu): This is O(n^2). However, each list should generally be small
+  // enough for this to not be a problem. They are also nicely local in memory
+  // like this.
+  for (const auto& sc : audio_send_codecs_) {
+    for (const auto& rc : audio_recv_codecs_) {
+      const size_t send_channels = (sc.channels == 0) ? 1 : sc.channels;
+      const size_t recv_channels = (rc.channels == 0) ? 1 : rc.channels;
+      if (sc.clockrate == rc.clockrate &&
+          sc.bitrate == rc.bitrate &&
+          send_channels == recv_channels &&
+          (_stricmp(sc.name.c_str(), rc.name.c_str()) == 0) &&
+          sc.params == rc.params &&
+          sc.feedback_params == rc.feedback_params) {
+        AudioCodec out_codec = sc;
+        out_codec.channels = send_channels;
+        audio_sendrecv_codecs_.push_back(out_codec);
+        break;
+      }
+    }
+  }
 }
 
 SessionDescription* MediaSessionDescriptionFactory::CreateOffer(
     const MediaSessionOptions& options,
     const SessionDescription* current_description) const {
   std::unique_ptr<SessionDescription> offer(new SessionDescription());
 
   StreamParamsVec current_streams;
   GetCurrentStreamParams(current_description, &current_streams);
 
+  const AudioCodecs *supported_audio_codecs;
+  if (options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_) {
+    if (options.recv_audio) {
+      // sendrecv
+      supported_audio_codecs = &audio_sendrecv_codecs_;
+    } else {
+      // sendonly
+      supported_audio_codecs = &audio_send_codecs_;
+    }
+  } else if (options.recv_audio) {
+    // recvonly
+    supported_audio_codecs = &audio_recv_codecs_;
+  } else {
+    // Stream is inactive - generate list as if sendrecv.
+    // See RFC 3264 Section 6.1.
+    supported_audio_codecs = &audio_sendrecv_codecs_;
+  }

[pthatcher1, 2016/05/31 21:52:36]

Would it make sense to make a method like this?

const AudioCodecs& GetAudioCodecsForDirection(MediaContentDirection md) {
  switch (md) {
    case MD_SENDONLY:
      return audio_send_codecs;
    case MD_RECVONLY:
      return audio_recv_codecs; 
  }
  return audio_sendrecv_codecs;
}

Then this code could just be:

bool send_audio = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_;
bool recv_audio = options.recv_audio;
const AudioCodecs& supported_audio_codecs = GetAudioCodecsForDirection(
  RtpTransceiverDirection(send_audio, recv_audio).ToMediaContentDescription()));

[ossu, 2016/06/02 13:28:02]

Why not have it take an RtpTranscieverDirection directly?

[pthatcher1, 2016/06/02 22:29:17]

Sounds like a great idea.

+
   AudioCodecs audio_codecs;
   VideoCodecs video_codecs;
   DataCodecs data_codecs;
-  GetCodecsToOffer(current_description, &audio_codecs, &video_codecs,
-                   &data_codecs);
+  GetCodecsToOffer(current_description, *supported_audio_codecs,
+                   video_codecs_, data_codecs_,
+                   &audio_codecs, &video_codecs, &data_codecs);
 
   if (!options.vad_enabled) {
     // If application doesn't want CN codecs in offer.
     StripCNCodecs(&audio_codecs);
   }
 
   RtpHeaderExtensions audio_rtp_extensions;
   RtpHeaderExtensions video_rtp_extensions;
   GetRtpHdrExtsToOffer(current_description, &audio_rtp_extensions,
                        &video_rtp_extensions);
@@ skipping 140 matching lines @@
         return NULL;
       }
     }
   }
 
   return answer.release();
 }
 
 void MediaSessionDescriptionFactory::GetCodecsToOffer(
     const SessionDescription* current_description,
+    const AudioCodecs& supported_audio_codecs,
+    const VideoCodecs& supported_video_codecs,
+    const DataCodecs& supported_data_codecs,
     AudioCodecs* audio_codecs,
     VideoCodecs* video_codecs,
     DataCodecs* data_codecs) const {
   UsedPayloadTypes used_pltypes;
   audio_codecs->clear();
   video_codecs->clear();
   data_codecs->clear();
 
 
   // First - get all codecs from the current description if the media type
@@ skipping 15 matching lines @@
     }
     const DataContentDescription* data =
         GetFirstDataContentDescription(current_description);
     if (data) {
       *data_codecs = data->codecs();
       used_pltypes.FindAndSetIdUsed<DataCodec>(data_codecs);
     }
   }
 
   // Add our codecs that are not in |current_description|.
-  FindCodecsToOffer<AudioCodec>(audio_codecs_, audio_codecs, &used_pltypes);
-  FindCodecsToOffer<VideoCodec>(video_codecs_, video_codecs, &used_pltypes);
-  FindCodecsToOffer<DataCodec>(data_codecs_, data_codecs, &used_pltypes);
+  FindCodecsToOffer<AudioCodec>(supported_audio_codecs, audio_codecs,
+                                &used_pltypes);
+  FindCodecsToOffer<VideoCodec>(supported_video_codecs, video_codecs,
+                                &used_pltypes);
+  FindCodecsToOffer<DataCodec>(supported_data_codecs, data_codecs,
+                               &used_pltypes);
 }
 
 void MediaSessionDescriptionFactory::GetRtpHdrExtsToOffer(
     const SessionDescription* current_description,
     RtpHeaderExtensions* audio_extensions,
     RtpHeaderExtensions* video_extensions) const {
   // All header extensions allocated from the same range to avoid potential
   // issues when using BUNDLE.
   UsedRtpHeaderExtensionIds used_ids;
   RtpHeaderExtensions all_extensions;
@@ skipping 261 matching lines @@
   return true;
 }
 
 bool MediaSessionDescriptionFactory::AddAudioContentForAnswer(
     const SessionDescription* offer,
     const MediaSessionOptions& options,
     const SessionDescription* current_description,
     StreamParamsVec* current_streams,
     SessionDescription* answer) const {
   const ContentInfo* audio_content = GetFirstAudioContent(offer);
+  const AudioContentDescription* audio_content_description =

[pthatcher1, 2016/05/31 21:52:36]

A better name would be offer_audio.

+      static_cast<const AudioContentDescription*>(audio_content->description);
 
   std::unique_ptr<TransportDescription> audio_transport(CreateTransportAnswer(
       audio_content->name, offer,
       GetTransportOptions(options, audio_content->name), current_description));
   if (!audio_transport) {
     return false;
   }
 
-  AudioCodecs audio_codecs = audio_codecs_;
+  // Pick codecs based on the requested communications direction in the offer.
+  // According to RFC 3264 Section 6.1, inactive is to be treated as sendrecv
+  // when constructing the list of supported formats.
+  AudioCodecs audio_codecs;
+  switch (audio_content_description->direction()) {
+    case MD_INACTIVE:
+      audio_codecs = audio_sendrecv_codecs_;
+      break;
+    case MD_SENDRECV:
+      if (options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_) {
+        audio_codecs = options.recv_audio
+            ? audio_sendrecv_codecs_
+            : audio_send_codecs_;
+      } else {
+        audio_codecs = options.recv_audio
+            ? audio_recv_codecs_
+            : audio_sendrecv_codecs_;           // inactive
+      }
+      break;
+    case MD_RECVONLY:
+      if (options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_)
+        audio_codecs = audio_send_codecs_;
+      else
+        audio_codecs = audio_sendrecv_codecs_;  // inactive
+      break;
+    case MD_SENDONLY:
+      if (options.recv_audio)
+        audio_codecs = audio_recv_codecs_;
+      else
+        audio_codecs = audio_sendrecv_codecs_;  // inactive
+      break;
+  }

[pthatcher1, 2016/05/31 21:52:36]

This could be more simple also:


auto offer_rtd =
RtpTransceiverDirection::FromMediaContentDirection(offer_audio->direction());
bool wants_send = options.HasSendMediaStream(MEDIA_TYPE_AUDIO) || add_legacy_;
bool wants_recv = options.recv_audio;
auto wants_rtd = RtpTransceiverDirection(wants_send, wants_recv);
auto answer_rtd = NegotiateRtpTransceiverDirection(offer_rtd, wants_rtd);
answer_audio_codecs = GetAudioCodecs(answer_rtd.ToMediaContentDescription()));
 

+
   if (!options.vad_enabled) {
     StripCNCodecs(&audio_codecs);
   }
 
   bool bundle_enabled =
       offer->HasGroup(GROUP_TYPE_BUNDLE) && options.bundle_enabled;
   std::unique_ptr<AudioContentDescription> audio_answer(
       new AudioContentDescription());
   // Do not require or create SDES cryptos if DTLS is used.
   cricket::SecurePolicy sdes_policy =
       audio_transport->secure() ? cricket::SEC_DISABLED : secure();
   if (!CreateMediaContentAnswer(
-          static_cast<const AudioContentDescription*>(
-              audio_content->description),
+          audio_content_description,
           options,
           audio_codecs,
           sdes_policy,
           GetCryptos(GetFirstAudioContentDescription(current_description)),
           audio_rtp_extensions_,
           current_streams,
           add_legacy_,
           bundle_enabled,
           audio_answer.get())) {
     return false;  // Fails the session setup.
@@ skipping 211 matching lines @@
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_VIDEO));
 }
 
 const DataContentDescription* GetFirstDataContentDescription(
     const SessionDescription* sdesc) {
   return static_cast<const DataContentDescription*>(
       GetFirstMediaContentDescription(sdesc, MEDIA_TYPE_DATA));
 }
 
 }  // namespace cricket