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

talk/session/media/channel.cc

Index: talk/session/media/channel.cc
diff --git a/talk/session/media/channel.cc b/talk/session/media/channel.cc
index d30972db06f13ef94bc5ca347d01ae4796034b87..7daa7b1307309c44b17e8bdfc4fcbb9dd7e65262 100644
--- a/talk/session/media/channel.cc
+++ b/talk/session/media/channel.cc
@@ -913,42 +913,6 @@ bool BaseChannel::CheckSrtpConfig(const std::vector<CryptoParams>& cryptos,
   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,
@@ -1210,49 +1174,6 @@ bool BaseChannel::UpdateRemoteStreams_w(
   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 =
@@ -1502,28 +1423,59 @@ bool VoiceChannel::SetLocalContent_w(const MediaContentDescription* content,
     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,
@@ -1540,43 +1492,70 @@ bool VoiceChannel::SetRemoteContent_w(const MediaContentDescription* content,
     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 (!SetSrtp_w(content->cryptos(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description SRTP";
+    return false;
+  }
 
-  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 (!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();
+  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) {
@@ -1846,37 +1825,60 @@ bool VideoChannel::SetLocalContent_w(const MediaContentDescription* content,
     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());
+  // 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 (!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_LOCAL, error_desc)) {
+    SafeSetError("Failed to set local video description RTCP mux.", 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();
+  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,
@@ -1893,39 +1895,71 @@ bool VideoChannel::SetRemoteContent_w(const MediaContentDescription* content,
     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 (!SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc)) {
+    LOG(LS_WARNING) << "Failed to set remote video description RTCP mux";
+    return false;
+  }
 
-    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 (!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();
+  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) {
@@ -2230,44 +2264,60 @@ bool DataChannel::SetLocalContent_w(const MediaContentDescription* content,
     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;
     }
-  }
 
-  // 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);
+    for (const DataCodec& codec : data->codecs()) {
+      bundle_filter()->AddPayloadType(codec.id);
     }
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set local data description";
   }
-  return ret;
+
+  // 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;
+  }
+
+  // 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,
@@ -2283,62 +2333,69 @@ bool DataChannel::SetRemoteContent_w(const MediaContentDescription* content,
     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 (!SetRtcpMux_w(content->rtcp_mux(), action, CS_REMOTE, error_desc)) {
+      SafeSetError("Failed to set remote data description RTCP-mux",
+                   error_desc);
+      return false;
     }
+  }
 
-    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;
-      }
-    }
+  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;
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set remote data description";
+  // 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;
   }
-  return ret;
+
+  set_remote_content_direction(content->direction());
+  ChangeState();
+  return true;
 }
 
 void DataChannel::ChangeState() {