Refactor the relationship between BaseChannel and MediaChannel so that we send over all the paramet…

talk/session/media/channel.cc

 /*
  * libjingle
  * Copyright 2004 Google Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  *  1. Redistributions of source code must retain the above copyright notice,
  *     this list of conditions and the following disclaimer.
  *  2. Redistributions in binary form must reproduce the above copyright notice,
@@ skipping 895 matching lines @@
                                   std::string* error_desc) {
   *dtls = transport_channel_->IsDtlsActive();
   if (*dtls && !cryptos.empty()) {
     SafeSetError("Cryptos must be empty when DTLS is active.",
                  error_desc);
     return false;
   }
   return true;
 }
 
-bool BaseChannel::SetRecvRtpHeaderExtensions_w(
-    const MediaContentDescription* content,
-    MediaChannel* media_channel,
-    std::string* error_desc) {
-  if (content->rtp_header_extensions_set()) {
-    if (!media_channel->SetRecvRtpHeaderExtensions(
-            content->rtp_header_extensions())) {
-      std::ostringstream desc;
-      desc << "Failed to set receive rtp header extensions for "
-           << MediaTypeToString(content->type()) << " content.";
-      SafeSetError(desc.str(), error_desc);
-      return false;
-    }
-  }
-  return true;
-}
-
-bool BaseChannel::SetSendRtpHeaderExtensions_w(
-    const MediaContentDescription* content,
-    MediaChannel* media_channel,
-    std::string* error_desc) {
-  if (content->rtp_header_extensions_set()) {
-    if (!media_channel->SetSendRtpHeaderExtensions(
-            content->rtp_header_extensions())) {
-      std::ostringstream desc;
-      desc << "Failed to set send rtp header extensions for "
-           << MediaTypeToString(content->type()) << " content.";
-      SafeSetError(desc.str(), error_desc);
-      return false;
-    } else {
-      MaybeCacheRtpAbsSendTimeHeaderExtension(content->rtp_header_extensions());
-    }
-  }
-  return true;
-}
-
 bool BaseChannel::SetSrtp_w(const std::vector<CryptoParams>& cryptos,
                             ContentAction action,
                             ContentSource src,
                             std::string* error_desc) {
   if (action == CA_UPDATE) {
     // no crypto params.
     return true;
   }
   bool ret = false;
   bool dtls = false;
@@ skipping 241 matching lines @@
         desc << "Failed to add remote stream ssrc: " << it->first_ssrc();
         SafeSetError(desc.str(), error_desc);
         ret = false;
       }
     }
   }
   remote_streams_ = streams;
   return ret;
 }
 
-bool BaseChannel::SetBaseLocalContent_w(const MediaContentDescription* content,
-                                        ContentAction action,
-                                        std::string* error_desc) {
-  // Cache secure_required_ for belt and suspenders check on SendPacket
-  secure_required_ = content->crypto_required() != CT_NONE;
-  // Set local RTP header extensions.
-  bool ret = SetRecvRtpHeaderExtensions_w(content, media_channel(), error_desc);
-  // Set local SRTP parameters (what we will encrypt with).
-  ret &= SetSrtp_w(content->cryptos(), action, CS_LOCAL, error_desc);
-  // Set local RTCP mux parameters.
-  ret &= SetRtcpMux_w(content->rtcp_mux(), action, CS_LOCAL, error_desc);
-
-  // Call UpdateLocalStreams_w last to make sure as many settings as possible
-  // are already set when creating streams.
-  ret &= UpdateLocalStreams_w(content->streams(), action, error_desc);
-  set_local_content_direction(content->direction());
-  return ret;
-}
-
-bool BaseChannel::SetBaseRemoteContent_w(const MediaContentDescription* content,
-                                         ContentAction action,
-                                         std::string* error_desc) {
-  // Set remote RTP header extensions.
-  bool ret = SetSendRtpHeaderExtensions_w(content, media_channel(), error_desc);
-  // Set remote SRTP parameters (what the other side will encrypt with).
-  ret &= SetSrtp_w(content->cryptos(), action, CS_REMOTE, error_desc);
-  // Set remote RTCP mux parameters.
-  ret &= SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc);
-  if (!media_channel()->SetMaxSendBandwidth(content->bandwidth())) {
-    std::ostringstream desc;
-    desc << "Failed to set max send bandwidth for "
-         << MediaTypeToString(content->type()) << " content.";
-    SafeSetError(desc.str(), error_desc);
-    ret = false;
-  }
-
-  // Call UpdateRemoteStreams_w last to make sure as many settings as possible
-  // are already set when creating streams.
-  ret &= UpdateRemoteStreams_w(content->streams(), action, error_desc);
-  set_remote_content_direction(content->direction());
-  return ret;
-}
-
 void BaseChannel::MaybeCacheRtpAbsSendTimeHeaderExtension(
     const std::vector<RtpHeaderExtension>& extensions) {
   const RtpHeaderExtension* send_time_extension =
       FindHeaderExtension(extensions, kRtpAbsoluteSenderTimeHeaderExtension);
   rtp_abs_sendtime_extn_id_ =
       send_time_extension ? send_time_extension->id : -1;
 }
 
 void BaseChannel::OnMessage(rtc::Message *pmsg) {
   switch (pmsg->message_id) {
@@ skipping 229 matching lines @@
   LOG(LS_INFO) << "Setting local voice description";
 
   const AudioContentDescription* audio =
       static_cast<const AudioContentDescription*>(content);
   ASSERT(audio != NULL);
   if (!audio) {
     SafeSetError("Can't find audio content in local description.", error_desc);
     return false;
   }
 
-  bool ret = SetBaseLocalContent_w(content, action, error_desc);
-  // Set local audio codecs (what we want to receive).
-  // TODO(whyuan): Change action != CA_UPDATE to !audio->partial() when partial
-  // is set properly.
-  if (action != CA_UPDATE || audio->has_codecs()) {
-    if (!media_channel()->SetRecvCodecs(audio->codecs())) {
-      SafeSetError("Failed to set audio receive codecs.", error_desc);
-      ret = false;
-    }
+  // CA_UPDATE is only used by the Harmony library to update send streams.
+  if (action == CA_UPDATE &&
+      !UpdateLocalStreams_w(content->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local audio description send streams.",
+                 error_desc);
+    return false;
   }
 
-  // If everything worked, see if we can start receiving.
-  if (ret) {
-    std::vector<AudioCodec>::const_iterator it = audio->codecs().begin();
-    for (; it != audio->codecs().end(); ++it) {
-      bundle_filter()->AddPayloadType(it->id);
-    }
-    ChangeState();
+  // Cache secure_required_ for belt and suspenders check on SendPacket
+  set_secure_required(content->crypto_required() != CT_NONE);
+  if (!SetSrtp_w(content->cryptos(), action, CS_LOCAL, error_desc)) {
+    SafeSetError("Failed to set local audio description SRTP.", error_desc);
+    return false;
+  }
+
+  if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_LOCAL, error_desc)) {
+    SafeSetError("Failed to set local audio description RTCP mux.", error_desc);
+    return false;
+  }
+
+  AudioRecvParameters recv_params;
+  recv_params.codecs = audio->codecs();
+  // TODO(pthatcher): See if we really need
+  // rtp_header_extensions_set() and remove it if we don't.
+  if (audio->rtp_header_extensions_set()) {
+    recv_params.extensions = audio->rtp_header_extensions();
   } else {
-    LOG(LS_WARNING) << "Failed to set local voice description";
+    // Just maintain the last set of header extensions.

[pbos-webrtc, 2015/07/12 15:56:08]

Should this be stateful? Shouldn't all extensions always be in the content here?
If no extensions are negotiated we shouldn't be using the previously set ones
right?

[pthatcher1, 2015/07/13 22:58:28]

The existing behavior is that if !rtp_header_extensions_set(), the header
extensions are not changed.  So, if they aren't set, we should be using the
previously set ones.  I'd like to fix the code up the stack to not rely on that
and pass down a full list every time, but that will have to be in later CLs.

+    recv_params.extensions = last_recv_params_.extensions;
   }
-  return ret;
+
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set local video description recv parameters.",
+                 error_desc);
+    return false;
+  }
+
+  // TODO(pthatcher): Move local streams into AudioSendParameters, and
+  // only give it to the media channel once we have a remote
+  // description too (without a remote description, we won't be able
+  // to send them anyway).
+  if (!UpdateLocalStreams_w(audio->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local audio description streams.", error_desc);
+    return false;
+  }
+
+  for (const AudioCodec& codec : audio->codecs()) {
+    bundle_filter()->AddPayloadType(codec.id);
+  }
+
+  set_local_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
                                       ContentAction action,
                                       std::string* error_desc) {
   ASSERT(worker_thread() == rtc::Thread::Current());
   LOG(LS_INFO) << "Setting remote voice description";
 
   const AudioContentDescription* audio =
       static_cast<const AudioContentDescription*>(content);
   ASSERT(audio != NULL);
   if (!audio) {
     SafeSetError("Can't find audio content in remote description.", error_desc);
     return false;
   }
 
-  bool ret = true;
-  // Set remote video codecs (what the other side wants to receive).
-  if (action != CA_UPDATE || audio->has_codecs()) {
-    if (!media_channel()->SetSendCodecs(audio->codecs())) {
-      SafeSetError("Failed to set audio send codecs.", error_desc);
-      ret = false;
+  // CA_UPDATE is only used by the Harmony library to update send

[pbos-webrtc, 2015/07/12 15:56:08]

This why we need to maintain last set of parameters? Can we add a TODO to move
the state to Harmony and remove CA_UPDATE?

[pthatcher1, 2015/07/13 22:58:27]

No, this is a separate thing, not related to the header extensions.  But I did
add the TODO.  That makes sense.

+  // codecs and receive streams.
+  if (action == CA_UPDATE) {
+    if (audio->has_codecs() &&
+        !media_channel()->SetSendCodecs(audio->codecs())) {
+      SafeSetError("Failed to set remote audio description send codecs.",
+                   error_desc);
+      return false;
     }
+    if (!UpdateRemoteStreams_w(content->streams(), action, error_desc)) {
+      SafeSetError("Failed to set remote audio description receive streams.",
+                   error_desc);
+      return false;
+    }
+    return true;
   }
 
-  ret &= SetBaseRemoteContent_w(content, action, error_desc);
-
-  if (action != CA_UPDATE) {
-    // Tweak our audio processing settings, if needed.
-    AudioOptions audio_options;
-    if (!media_channel()->GetOptions(&audio_options)) {
-      LOG(LS_WARNING) << "Can not set audio options from on remote content.";
-    } else {
-      if (audio->conference_mode()) {
-        audio_options.conference_mode.Set(true);
-      }
-      if (audio->agc_minus_10db()) {
-        audio_options.adjust_agc_delta.Set(kAgcMinus10db);
-      }
-      if (!media_channel()->SetOptions(audio_options)) {
-        // Log an error on failure, but don't abort the call.
-        LOG(LS_ERROR) << "Failed to set voice channel options";
-      }
-    }
+  if (!SetSrtp_w(content->cryptos(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description SRTP";
+    return false;
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
+  if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description RTCP mux";
+    return false;
+  }
+
+  AudioSendParameters send_params;
+  send_params.codecs = audio->codecs();
+  // TODO(pthatcher): See if we really need
+  // rtp_header_extensions_set() and remove it if we don't.
+  if (audio->rtp_header_extensions_set()) {
+    send_params.extensions = audio->rtp_header_extensions();
   } else {
-    LOG(LS_WARNING) << "Failed to set remote voice description";
+    // Just maintain the last set of header extensions.
+    send_params.extensions = last_send_params_.extensions;

[pbos-webrtc, 2015/07/12 15:56:08]

Is this tied to CA_UPDATE? Otherwise no set extensions should mean no
extensions, correct?

[pthatcher1, 2015/07/13 22:58:27]

No, this has nothing to do with CA_UPDATE.  Honestly, I'm not entirely sure
what's relying on this.  But I'm taking enough risk changing as much code as I
am, and I don't want to change any more behavior than I have to.  But I've added
a TODO to remove rtp_header_extensions_set() if we can.

   }
-  return ret;
+  send_params.max_bandwidth_bps = audio->bandwidth();
+  send_params.options = last_send_params_.options;
+  if (audio->conference_mode()) {
+    send_params.options.conference_mode.Set(true);
+  }
+  if (audio->agc_minus_10db()) {
+    send_params.options.adjust_agc_delta.Set(kAgcMinus10db);
+  }
+  if (!media_channel()->SetSendParameters(send_params)) {
+    SafeSetError("Failed to set remote audio description send parameters.",
+                 error_desc);
+    return false;
+  }
+  last_send_params_ = send_params;
+
+  // TODO(pthatcher): Move remote streams into AudioRecvParameters,
+  // and only give it to the media channel once we have a local
+  // description too (without a local description, we won't be able to
+  // recv them anyway).
+  if (!UpdateRemoteStreams_w(audio->streams(), action, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote audio description receive streams";
+    return false;
+  }
+
+  set_remote_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 bool VoiceChannel::SetRingbackTone_w(const void* buf, int len) {
   ASSERT(worker_thread() == rtc::Thread::Current());
   return media_channel()->SetRingbackTone(static_cast<const char*>(buf), len);
 }
 
 bool VoiceChannel::PlayRingbackTone_w(uint32 ssrc, bool play, bool loop) {
   ASSERT(worker_thread() == rtc::Thread::Current());
   if (play) {
@@ skipping 249 matching lines @@
   LOG(LS_INFO) << "Setting local video description";
 
   const VideoContentDescription* video =
       static_cast<const VideoContentDescription*>(content);
   ASSERT(video != NULL);
   if (!video) {
     SafeSetError("Can't find video content in local description.", error_desc);
     return false;
   }
 
-  bool ret = SetBaseLocalContent_w(content, action, error_desc);
-  // Set local video codecs (what we want to receive).
-  if (action != CA_UPDATE || video->has_codecs()) {
-    if (!media_channel()->SetRecvCodecs(video->codecs())) {
-      SafeSetError("Failed to set video receive codecs.", error_desc);
-      ret = false;
-    }
+  // CA_UPDATE is only used by the Harmony library to update send streams.
+  if (action == CA_UPDATE &&
+      !UpdateLocalStreams_w(content->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local audio description send streams.",
+                 error_desc);
+    return false;
   }
 
-  if (action != CA_UPDATE) {
-    VideoOptions video_options;
-    media_channel()->GetOptions(&video_options);
-    video_options.buffered_mode_latency.Set(video->buffered_mode_latency());
-
-    if (!media_channel()->SetOptions(video_options)) {
-      // Log an error on failure, but don't abort the call.
-      LOG(LS_ERROR) << "Failed to set video channel options";
-    }
+  // Cache secure_required_ for belt and suspenders check on SendPacket
+  set_secure_required(content->crypto_required() != CT_NONE);
+  if (!SetSrtp_w(content->cryptos(), action, CS_LOCAL, error_desc)) {
+    SafeSetError("Failed to set local video description SRTP.", error_desc);
+    return false;
   }
 
-  // If everything worked, see if we can start receiving.
-  if (ret) {
-    std::vector<VideoCodec>::const_iterator it = video->codecs().begin();
-    for (; it != video->codecs().end(); ++it) {
-      bundle_filter()->AddPayloadType(it->id);
-    }
-    ChangeState();
+  if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_LOCAL, error_desc)) {
+    SafeSetError("Failed to set local video description RTCP mux.", error_desc);
+    return false;
+  }
+
+  VideoRecvParameters recv_params;
+  recv_params.codecs = video->codecs();
+  // TODO(pthatcher): See if we really need
+  // rtp_header_extensions_set() and remove it if we don't.
+  if (video->rtp_header_extensions_set()) {
+    recv_params.extensions = video->rtp_header_extensions();

[pbos-webrtc, 2015/07/12 15:56:08]

Feels like I've read this code 4x times now, can some of this be refactored out?
GetExtensions(const MediaContentDescription&, const std::vector<RtpExtension>&
last_extensions, std::vector<RtpExtension>*), or something?

[pthatcher1, 2015/07/13 22:58:27]

Maybe.  The code before was structured a lot to share code, but that ended up
being a big problem: it's what encouraged the "break up content into pieces"
behavior that I'm trying to remove.  Plus, it just made the code much less
readable.  I'm in favor of a little duplication for more readability.  

That said, I think there is some duplication I can remove around
set_secure_required/SetSrtp_w/SetRtcpMux_w.  I'll try to improve that area a
bit.

   } else {
-    LOG(LS_WARNING) << "Failed to set local video description";
+    // Just maintain the last set of header extensions.
+    recv_params.extensions = last_recv_params_.extensions;
   }
-  return ret;
+
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set local video description recv parameters.",
+                 error_desc);
+    return false;
+  }
+  last_recv_params_ = recv_params;
+
+  // TODO(pthatcher): Move local streams into VideoSendParameters, and
+  // only give it to the media channel once we have a remote
+  // description too (without a remote description, we won't be able
+  // to send them anyway).
+  if (!UpdateLocalStreams_w(video->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local video description streams.", error_desc);
+    return false;
+  }
+
+  for (const VideoCodec& codec : video->codecs()) {
+    bundle_filter()->AddPayloadType(codec.id);
+  }
+
+  set_local_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
                                       ContentAction action,
                                       std::string* error_desc) {
   ASSERT(worker_thread() == rtc::Thread::Current());
   LOG(LS_INFO) << "Setting remote video description";
 
   const VideoContentDescription* video =
       static_cast<const VideoContentDescription*>(content);
   ASSERT(video != NULL);
   if (!video) {
     SafeSetError("Can't find video content in remote description.", error_desc);
     return false;
   }
 
-  bool ret = true;
-  // Set remote video codecs (what the other side wants to receive).
-  if (action != CA_UPDATE || video->has_codecs()) {
-    if (!media_channel()->SetSendCodecs(video->codecs())) {
-      SafeSetError("Failed to set video send codecs.", error_desc);
-      ret = false;
+  // CA_UPDATE is only used by the Harmony library to update send

[pbos-webrtc, 2015/07/12 15:56:08]

Can some of this go in the base class?

[pthatcher1, 2015/07/13 22:58:27]

SetSendCodecs can't very easily (which is why it wasn't before). And
updateRemoteStreams_w already is.  But I'll try SetRtcpMux_w/SetSrtp_w, like I
mentioned.

+  // codecs and receive streams.
+  if (action == CA_UPDATE) {
+    if (video->has_codecs() &&
+        !media_channel()->SetSendCodecs(video->codecs())) {
+      SafeSetError("Failed to set remote video description send codecs.",
+                   error_desc);
+      return false;
     }
+    if (!UpdateRemoteStreams_w(content->streams(), action, error_desc)) {
+      SafeSetError("Failed to set remote video description receive streams.",
+                   error_desc);
+      return false;
+    }
+    return true;
   }
 
-  ret &= SetBaseRemoteContent_w(content, action, error_desc);
-
-  if (action != CA_UPDATE) {
-    // Tweak our video processing settings, if needed.
-    VideoOptions video_options;
-    media_channel()->GetOptions(&video_options);
-    if (video->conference_mode()) {
-      video_options.conference_mode.Set(true);
-    }
-    video_options.buffered_mode_latency.Set(video->buffered_mode_latency());
-
-    if (!media_channel()->SetOptions(video_options)) {
-      // Log an error on failure, but don't abort the call.
-      LOG(LS_ERROR) << "Failed to set video channel options";
-    }
+  if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description RTCP mux";
+    return false;
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
+  if (!SetSrtp_w(content->cryptos(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description SRTP";
+    return false;
+  }
+
+  VideoSendParameters send_params;
+  send_params.codecs = video->codecs();
+  // TODO(pthatcher): See if we really need
+  // rtp_header_extensions_set() and remove it if we don't.
+  if (video->rtp_header_extensions_set()) {
+    send_params.extensions = video->rtp_header_extensions();
   } else {
-    LOG(LS_WARNING) << "Failed to set remote video description";
+    // Just maintain the last set of header extensions.
+    send_params.extensions = last_send_params_.extensions;
   }
-  return ret;
+  send_params.max_bandwidth_bps = video->bandwidth();
+  send_params.options = last_send_params_.options;
+  if (video->conference_mode()) {
+    send_params.options.conference_mode.Set(true);
+  }
+  if (!media_channel()->SetSendParameters(send_params)) {
+    SafeSetError("Failed to set remote video description send parameters.",
+                 error_desc);
+    return false;
+  }
+  last_send_params_ = send_params;
+
+  if (video->rtp_header_extensions_set()) {
+    MaybeCacheRtpAbsSendTimeHeaderExtension(video->rtp_header_extensions());
+  }
+
+  // TODO(pthatcher): Move remote streams into VideoRecvParameters,
+  // and only give it to the media channel once we have a local
+  // description too (without a local description, we won't be able to
+  // recv them anyway).
+  if (!UpdateRemoteStreams_w(video->streams(), action, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description receive streams";
+    return false;
+  }
+
+  set_remote_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 bool VideoChannel::ApplyViewRequest_w(const ViewRequest& request) {
   bool ret = true;
   // Set the send format for each of the local streams. If the view request
   // does not contain a local stream, set its send format to 0x0, which will
   // drop all frames.
   for (std::vector<StreamParams>::const_iterator it = local_streams().begin();
       it != local_streams().end(); ++it) {
     VideoFormat format(0, 0, 0, cricket::FOURCC_I420);
@@ skipping 284 matching lines @@
   LOG(LS_INFO) << "Setting local data description";
 
   const DataContentDescription* data =
       static_cast<const DataContentDescription*>(content);
   ASSERT(data != NULL);
   if (!data) {
     SafeSetError("Can't find data content in local description.", error_desc);
     return false;
   }
 
-  bool ret = false;
+  // CA_UPDATE is only used by the Harmony library to update send streams.
+  if (action == CA_UPDATE &&

[pbos-webrtc, 2015/07/12 15:56:08]

Same things here, did I read this 4x times? Anything that we can do?

[pthatcher1, 2015/07/13 22:58:27]

There's nothing to be done between local and remote.  So you're going to be
reading it at last 2 times.  

Then it's a matter of re-reading it between audio and video (2 vs 4 times).  For
some parts (UpdateLocalStream_w), there isn't much more to move.  It's already
all there.  But perhaps for SetSrtp_w and SetRtcpMux_w, as I mentioned.  I'll
give that a shot.

+      !UpdateLocalStreams_w(content->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local audio description send streams.",
+                 error_desc);
+    return false;
+  }
+
   if (!SetDataChannelTypeFromContent(data, error_desc)) {
     return false;
   }
 
-  if (data_channel_type_ == DCT_SCTP) {
-    // SCTP data channels don't need the rest of the stuff.
-    ret = UpdateLocalStreams_w(data->streams(), action, error_desc);
-    if (ret) {
-      set_local_content_direction(content->direction());
-      // As in SetRemoteContent_w, make sure we set the local SCTP port
-      // number as specified in our DataContentDescription.
-      if (!media_channel()->SetRecvCodecs(data->codecs())) {
-        SafeSetError("Failed to set data receive codecs.", error_desc);
-        ret = false;
-      }
+  if (data_channel_type_ == DCT_RTP) {
+    // Cache secure_required_ for belt and suspenders check on SendPacket
+    set_secure_required(content->crypto_required() != CT_NONE);
+    if (!SetSrtp_w(content->cryptos(), action, CS_LOCAL, error_desc)) {
+      SafeSetError("Failed to set local data description SRTP.", error_desc);
+      return false;
     }
-  } else {
-    ret = SetBaseLocalContent_w(content, action, error_desc);
-    if (action != CA_UPDATE || data->has_codecs()) {
-      if (!media_channel()->SetRecvCodecs(data->codecs())) {
-        SafeSetError("Failed to set data receive codecs.", error_desc);
-        ret = false;
-      }
+
+    if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_LOCAL, error_desc)) {
+      SafeSetError("Failed to set local data description RTCP mux.",
+                   error_desc);
+      return false;
+    }
+
+    for (const DataCodec& codec : data->codecs()) {
+      bundle_filter()->AddPayloadType(codec.id);
     }
   }
 
-  // If everything worked, see if we can start receiving.
-  if (ret) {
-    std::vector<DataCodec>::const_iterator it = data->codecs().begin();
-    for (; it != data->codecs().end(); ++it) {
-      bundle_filter()->AddPayloadType(it->id);
-    }
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set local data description";
+  // FYI: We send the SCTP port number (not to be confused with the
+  // underlying UDP port number) as a codec parameter.  So even SCTP
+  // data channels need codecs.
+  DataRecvParameters recv_params;
+  recv_params.codecs = data->codecs();
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set remote data description recv parameters.",
+                 error_desc);
+    return false;
   }
-  return ret;
+
+  // TODO(pthatcher): Move local streams into DataSendParameters, and
+  // only give it to the media channel once we have a remote
+  // description too (without a remote description, we won't be able
+  // to send them anyway).
+  if (!UpdateLocalStreams_w(data->streams(), action, error_desc)) {
+    SafeSetError("Failed to set local data description streams.", error_desc);
+    return false;
+  }
+
+  set_local_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 bool DataChannel::SetRemoteContent_w(const MediaContentDescription* content,
                                      ContentAction action,
                                      std::string* error_desc) {
   ASSERT(worker_thread() == rtc::Thread::Current());
 
   const DataContentDescription* data =
       static_cast<const DataContentDescription*>(content);
   ASSERT(data != NULL);
   if (!data) {
     SafeSetError("Can't find data content in remote description.", error_desc);
     return false;
   }
 
-  bool ret = true;
+  // CA_UPDATE is only used by the Harmony library to update send
+  // codecs and receive streams.
+  if (action == CA_UPDATE) {
+    if (data->has_codecs() && !media_channel()->SetSendCodecs(data->codecs())) {
+      SafeSetError("Failed to set remote data description send codecs.",
+                   error_desc);
+      return false;
+    }
+    if (!UpdateRemoteStreams_w(content->streams(), action, error_desc)) {
+      SafeSetError("Failed to set remote data description receive streams.",
+                   error_desc);
+      return false;
+    }
+    return true;
+  }
+
+  // If the remote data doesn't have codecs and isn't an update, it
+  // must be empty, so ignore it.
+  if (!data->has_codecs()) {
+    return true;
+  }
+
   if (!SetDataChannelTypeFromContent(data, error_desc)) {
     return false;
   }
 
-  if (data_channel_type_ == DCT_SCTP) {
-    LOG(LS_INFO) << "Setting SCTP remote data description";
-    // SCTP data channels don't need the rest of the stuff.
-    ret = UpdateRemoteStreams_w(content->streams(), action, error_desc);
-    if (ret) {
-      set_remote_content_direction(content->direction());
-      // We send the SCTP port number (not to be confused with the underlying
-      // UDP port number) as a codec parameter.  Make sure it gets there.
-      if (!media_channel()->SetSendCodecs(data->codecs())) {
-        SafeSetError("Failed to set data send codecs.", error_desc);
-        ret = false;
-      }
-    }
-  } else {
-    // If the remote data doesn't have codecs and isn't an update, it
-    // must be empty, so ignore it.
-    if (action != CA_UPDATE && !data->has_codecs()) {
-      return true;
-    }
-    LOG(LS_INFO) << "Setting remote data description";
-
-    // Set remote video codecs (what the other side wants to receive).
-    if (action != CA_UPDATE || data->has_codecs()) {
-      if (!media_channel()->SetSendCodecs(data->codecs())) {
-        SafeSetError("Failed to set data send codecs.", error_desc);
-        ret = false;
-      }
+  LOG(LS_INFO) << "Setting remote data description";
+  if (data_channel_type_ == DCT_RTP) {
+    // Set remote SRTP parameters (what the other side will encrypt with).
+    if (!SetSrtp_w(content->cryptos(), action, CS_REMOTE, error_desc)) {
+      SafeSetError("Failed to set remote data description SRTP", error_desc);
+      return false;
     }
 
-    if (ret) {
-      ret &= SetBaseRemoteContent_w(content, action, error_desc);
-    }
-
-    if (action != CA_UPDATE) {
-      int bandwidth_bps = data->bandwidth();
-      if (!media_channel()->SetMaxSendBandwidth(bandwidth_bps)) {
-        std::ostringstream desc;
-        desc << "Failed to set max send bandwidth for data content.";
-        SafeSetError(desc.str(), error_desc);
-        ret = false;
-      }
+    if (!SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc)) {
+      SafeSetError("Failed to set remote data description RTCP-mux",
+                   error_desc);
+      return false;
     }
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set remote data description";
+  DataSendParameters send_params;
+  send_params.codecs = data->codecs();
+  send_params.max_bandwidth_bps = data->bandwidth();
+  if (!media_channel()->SetSendParameters(send_params)) {
+    SafeSetError("Failed to set remote data description send parameters.",
+                 error_desc);
+    return false;
   }
-  return ret;
+
+  // TODO(pthatcher): Move remote streams into DataRecvParameters,
+  // and only give it to the media channel once we have a local
+  // description too (without a local description, we won't be able to
+  // recv them anyway).
+  if (!UpdateRemoteStreams_w(data->streams(), action, error_desc)) {
+    SafeSetError("Failed to set remote data description streams.",
+                 error_desc);
+    return false;
+  }
+
+  set_remote_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 void DataChannel::ChangeState() {
   // Render incoming data if we're the active call, and we have the local
   // content. We receive data on the default channel and multiplexed streams.
   bool recv = IsReadyToReceive();
   if (!media_channel()->SetReceive(recv)) {
     LOG(LS_ERROR) << "Failed to SetReceive on data channel";
   }
 
@@ skipping 127 matching lines @@
   return (data_channel_type_ == DCT_RTP);
 }
 
 void DataChannel::OnStreamClosedRemotely(uint32 sid) {
   rtc::TypedMessageData<uint32>* message =
       new rtc::TypedMessageData<uint32>(sid);
   signaling_thread()->Post(this, MSG_STREAMCLOSEDREMOTELY, message);
 }
 
 }  // namespace cricket