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 132 matching lines @@
   return direction == MD_SENDRECV || direction == MD_SENDONLY;
 }
 
 static const MediaContentDescription* GetContentDescription(
     const ContentInfo* cinfo) {
   if (cinfo == NULL)
     return NULL;
   return static_cast<const MediaContentDescription*>(cinfo->description);
 }
 
+template <class Codec>
+void RtpParametersFromMediaDescription(
+    const MediaContentDescriptionImpl<Codec>* desc,
+    RtpParameters<Codec>* params) {
+  // TODO(pthatcher): Remove this once we're sure no one will give us
+  // a description without codecs (currently a CA_UPDATE with just
+  // streams can).
+  if (desc->has_codecs()) {

[Taylor Brandstetter, 2015/07/18 00:16:31]

Are you sure only a CA_UPDATE will give a description without codecs? There was
this type of condition before: "if (action != CA_UPDATE ||
audio->has_codecs())". Which means it *would* set an empty list of codecs if the
action wasn't CA_UPDATE.

[pthatcher1, 2015/07/31 10:59:20]

If anything other than a CA_UPDATE has an empty list of codecs, that's
equivalent to not being able to send or receive.  It may be possible to provide
such a description, but it's probably a programmer error if they do so.

+    params->codecs = desc->codecs();
+  }
+  // TODO(pthatcher): See if we really need
+  // rtp_header_extensions_set() and remove it if we don't.
+  if (desc->rtp_header_extensions_set()) {
+    params->extensions = desc->rtp_header_extensions();
+  }
+}
+
+template <class Codec, class Options>
+void RtpSendParametersFromMediaDescription(
+    const MediaContentDescriptionImpl<Codec>* desc,
+    RtpSendParameters<Codec, Options>* send_params) {
+  RtpParametersFromMediaDescription(desc, send_params);
+  send_params->max_bandwidth_bps = desc->bandwidth();
+}
+
 BaseChannel::BaseChannel(rtc::Thread* thread,
                          MediaEngineInterface* media_engine,
                          MediaChannel* media_channel, BaseSession* session,
                          const std::string& content_name, bool rtcp)
     : worker_thread_(thread),
       media_engine_(media_engine),
       session_(session),
       media_channel_(media_channel),
       content_name_(content_name),
       rtcp_(rtcp),
@@ skipping 729 matching lines @@
   if (!writable_)
     return;
 
   LOG(LS_INFO) << "Channel socket not writable ("
                << transport_channel_->content_name() << ", "
                << transport_channel_->component() << ")";
   writable_ = false;
   ChangeState();
 }
 
+bool BaseChannel::SetRtpTransportParameters_w(
+    const MediaContentDescription* content,
+    ContentAction action,
+    ContentSource src,
+    std::string* error_desc) {
+  if (action == CA_UPDATE) {
+    // These parameters never get changed by a CA_UDPATE.
+    return true;
+  }
+
+  // Cache secure_required_ for belt and suspenders check on SendPacket
+  set_secure_required(content->crypto_required() != CT_NONE);

[Taylor Brandstetter, 2015/07/18 00:16:31]

Should set_secure_required only be called here if src == CS_LOCAL? That's how it
worked before.

[pthatcher1, 2015/07/31 10:59:20]

Good catch.  I've fixed it.

+  if (!SetSrtp_w(content->cryptos(), action, src, error_desc)) {
+    return false;
+  }
+
+  if (!SetRtcpMux_w(content->rtcp_mux(), action, src, error_desc)) {
+    return false;
+  }
+
+  return true;
+}
+
 // |dtls| will be set to true if DTLS is active for transport channel and
 // crypto is empty.
 bool BaseChannel::CheckSrtpConfig(const std::vector<CryptoParams>& cryptos,
                                   bool* dtls,
                                   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;
-    }
+  if (!SetRtpTransportParameters_w(content, action, CS_LOCAL, error_desc)) {

[Taylor Brandstetter, 2015/07/18 00:16:31]

If SetRtpTransportParameters_w fails, this method will just return false,
whereas previously it would log "Failed to set [local|remote] [voice|video]
description". Should we set an error here, or at least log a warning message?

[pthatcher1, 2015/07/31 10:59:20]

SetRtpTransportParameters_w will set the error like it did before, but further
down (in SetSrtp_w, etc).

+    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();
-  } else {
-    LOG(LS_WARNING) << "Failed to set local voice description";
+  AudioRecvParameters recv_params = last_recv_params_;
+  RtpParametersFromMediaDescription(audio, &recv_params);
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set local video description recv parameters.",
+                 error_desc);
+    return false;
   }
-  return ret;
+  for (const AudioCodec& codec : audio->codecs()) {
+    bundle_filter()->AddPayloadType(codec.id);
+  }
+  last_recv_params_ = recv_params;
+
+  // 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;
+  }
+
+  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;
-    }
+  if (!SetRtpTransportParameters_w(content, action, CS_REMOTE, error_desc)) {
+    return false;
   }
 
-  ret &= SetBaseRemoteContent_w(content, action, error_desc);
+  AudioSendParameters send_params = last_send_params_;
+  RtpSendParametersFromMediaDescription(audio, &send_params);
+  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;
 
-  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";
-      }
-    }
+  // 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";

[Taylor Brandstetter, 2015/07/18 00:16:31]

I notice that SafeSetError is used if UpdateLocalStreams_w fails, but
LOG(LS_WARNING) is used if UpdateRemoteStreams_w fails; is this intentional?

[pthatcher1, 2015/07/31 10:59:20]

Good catch. 

Fixed.

+    return false;
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set remote voice description";
-  }
-  return ret;
+  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;
-    }
+  if (!SetRtpTransportParameters_w(content, action, CS_LOCAL, error_desc)) {
+    return false;
   }
 
-  if (action != CA_UPDATE) {
-    VideoOptions video_options;
-    media_channel()->GetOptions(&video_options);
-    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";
-    }
+  VideoRecvParameters recv_params = last_recv_params_;
+  RtpParametersFromMediaDescription(video, &recv_params);
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set local video description recv parameters.",
+                 error_desc);
+    return false;
+  }
+  for (const VideoCodec& codec : video->codecs()) {
+    bundle_filter()->AddPayloadType(codec.id);
+  }
+  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;
   }
 
-  // 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();
-  } else {
-    LOG(LS_WARNING) << "Failed to set local video description";
-  }
-  return ret;
+  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;
-    }
+
+  if (!SetRtpTransportParameters_w(content, action, CS_REMOTE, error_desc)) {
+    return false;
   }
 
-  ret &= SetBaseRemoteContent_w(content, action, error_desc);
+  VideoSendParameters send_params = last_send_params_;
+  RtpSendParametersFromMediaDescription(video, &send_params);
+  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 (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);
-    }
-
-    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";
-    }
+  // 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;
   }
 
-  // If everything worked, see if we can start sending.
-  if (ret) {
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set remote video description";
+  if (video->rtp_header_extensions_set()) {
+    MaybeCacheRtpAbsSendTimeHeaderExtension(video->rtp_header_extensions());
   }
-  return ret;
+
+  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;
   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;
-      }
-    }
-  } 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 (data_channel_type_ == DCT_RTP) {
+    if (!SetRtpTransportParameters_w(content, action, CS_LOCAL, 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);
+  // 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 = last_recv_params_;
+  RtpParametersFromMediaDescription(data, &recv_params);
+  if (!media_channel()->SetRecvParameters(recv_params)) {
+    SafeSetError("Failed to set remote data description recv parameters.",
+                 error_desc);
+    return false;
+  }
+  if (data_channel_type_ == DCT_RTP) {
+    for (const DataCodec& codec : data->codecs()) {
+      bundle_filter()->AddPayloadType(codec.id);
     }
-    ChangeState();
-  } else {
-    LOG(LS_WARNING) << "Failed to set local data description";
   }
-  return ret;
+  last_recv_params_ = recv_params;
+
+  // 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;
+  // If the remote data doesn't have codecs and isn't an update, it
+  // must be empty, so ignore it.
+  if (!data->has_codecs() && action != CA_UPDATE) {
+    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;
-      }
-    }
-
-    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;
-      }
-    }
+  LOG(LS_INFO) << "Setting remote data description";
+  if (data_channel_type_ == DCT_RTP &&
+      !SetRtpTransportParameters_w(content, action, CS_REMOTE, 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 = last_send_params_;
+  RtpSendParametersFromMediaDescription<DataCodec>(data, &send_params);
+  if (!media_channel()->SetSendParameters(send_params)) {
+    SafeSetError("Failed to set remote data description send parameters.",
+                 error_desc);
+    return false;
   }
-  return ret;
+  last_send_params_ = send_params;
+
+  // 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