Move some send stream configuration into webrtc::AudioSendStream.

talk/media/webrtc/webrtcvoiceengine.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 378 matching lines @@
 std::string GetEnableString(bool enable) {
   return enable ? "enable" : "disable";
 }
 
 webrtc::AudioState::Config MakeAudioStateConfig(VoEWrapper* voe_wrapper) {
   webrtc::AudioState::Config config;
   config.voice_engine = voe_wrapper->engine();
   return config;
 }
 
+std::vector<webrtc::RtpExtension> FindAudioRtpHeaderExtensions(
+    const std::vector<RtpHeaderExtension>& extensions) {
+  std::vector<webrtc::RtpExtension> result;
+  for (const auto& extension : extensions) {
+    if (extension.uri == kRtpAbsoluteSenderTimeHeaderExtension ||
+        extension.uri == kRtpAudioLevelHeaderExtension) {
+      result.push_back({extension.uri, extension.id});
+    } else {
+      LOG(LS_WARNING) << "Unsupported RTP extension: " << extension.ToString();
+    }
+  }
+  return result;
+}
 } // namespace {
 
 WebRtcVoiceEngine::WebRtcVoiceEngine()
     : voe_wrapper_(new VoEWrapper()),
       tracing_(new VoETraceWrapper()),
       audio_state_(webrtc::AudioState::Create(MakeAudioStateConfig(voe()))),
       log_filter_(SeverityToFilter(kDefaultLogSeverity)) {
   Construct();
 }
 
@@ skipping 921 matching lines @@
 
 int WebRtcVoiceEngine::CreateVoEChannel() {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   return voe_wrapper_->base()->CreateChannel(voe_config_);
 }
 
 class WebRtcVoiceMediaChannel::WebRtcAudioSendStream
     : public AudioRenderer::Sink {
  public:
   WebRtcAudioSendStream(int ch, webrtc::AudioTransport* voe_audio_transport,
-                        uint32_t ssrc, webrtc::Call* call)
+                        uint32_t ssrc, const std::string& c_name,
+                        const std::vector<webrtc::RtpExtension>& extensions,
+                        webrtc::Call* call)
       : channel_(ch),
         voe_audio_transport_(voe_audio_transport),
-        call_(call) {
+        call_(call),
+        config_(nullptr) {
     RTC_DCHECK_GE(ch, 0);
     // TODO(solenberg): Once we're not using FakeWebRtcVoiceEngine anymore:
     // RTC_DCHECK(voe_audio_transport);
     RTC_DCHECK(call);
     audio_capture_thread_checker_.DetachFromThread();
-    webrtc::AudioSendStream::Config config(nullptr);
-    config.voe_channel_id = channel_;
-    config.rtp.ssrc = ssrc;
-    stream_ = call_->CreateAudioSendStream(config);
-    RTC_DCHECK(stream_);
+    config_.rtp.ssrc = ssrc;
+    config_.rtp.c_name = c_name;
+    config_.voe_channel_id = ch;
+    RecreateAudioSendStream(extensions);
   }
+
   ~WebRtcAudioSendStream() override {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     Stop();
     call_->DestroyAudioSendStream(stream_);
   }
 
+  void RecreateAudioSendStream(
+      const std::vector<webrtc::RtpExtension>& extensions) {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    if (stream_) {
+      call_->DestroyAudioSendStream(stream_);
+    }
+    config_.rtp.extensions = extensions;

[tommi, 2015/11/12 13:44:57]

possibly silly question - does the caller need the vector after constructing the
audio send stream or will it be thrown away?  I'm basically wondering if it
would make sense to pass by ptr and do a swap().  Maybe this happens so rarely
and contains so few elements that it's not worth it, but figured I'd ask anyway.

[the sun, 2015/11/12 15:16:43]

Yes, the caller needs it. The same extensions are set for multiple streams, and
I'd rather keep this interface analogous with the c-tor. OTOH, I can use a swap
in SetSendParameters(); thanks.

+    stream_ = call_->CreateAudioSendStream(config_);

[tommi, 2015/11/12 13:44:56]

should we first DCHECK that stream_ is nullptr?

[the sun, 2015/11/12 15:16:43]

Done.

+    RTC_CHECK(stream_);
+  }
+
+  webrtc::AudioSendStream::Stats GetStats() const {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    return stream_->GetStats();

[tommi, 2015/11/12 13:44:56]

any chance that stream_ might be null?

[the sun, 2015/11/12 15:16:44]

Yes, stream_ is mutable so a check is in place.

+  }
+
   // Starts the rendering by setting a sink to the renderer to get data
   // callback.
   // This method is called on the libjingle worker thread.
   // TODO(xians): Make sure Start() is called only once.
   void Start(AudioRenderer* renderer) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     RTC_DCHECK(renderer);
     if (renderer_) {
       RTC_DCHECK(renderer_ == renderer);
       return;
     }
     renderer->SetSink(this);
     renderer_ = renderer;
   }
 
-  webrtc::AudioSendStream::Stats GetStats() const {
-    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    return stream_->GetStats();
-  }
-
   // Stops rendering by setting the sink of the renderer to nullptr. No data
   // callback will be received after this method.
   // This method is called on the libjingle worker thread.
   void Stop() {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (renderer_) {
       renderer_->SetSink(nullptr);
       renderer_ = nullptr;
     }
   }
@@ skipping 30 matching lines @@
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     return channel_;
   }
 
  private:
   rtc::ThreadChecker worker_thread_checker_;
   rtc::ThreadChecker audio_capture_thread_checker_;
   const int channel_ = -1;
   webrtc::AudioTransport* const voe_audio_transport_ = nullptr;
   webrtc::Call* call_ = nullptr;
+  webrtc::AudioSendStream::Config config_;
   webrtc::AudioSendStream* stream_ = nullptr;

[tommi, 2015/11/12 13:44:56]

nit: if you could add a note about ownership or lifetime, that would be nice.

[the sun, 2015/11/12 15:16:44]

Done.

 
   // Raw pointer to AudioRenderer owned by LocalAudioTrackHandler.
   // PeerConnection will make sure invalidating the pointer before the object
   // goes away.
   AudioRenderer* renderer_ = nullptr;
 
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(WebRtcAudioSendStream);
 };
 
 class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
@@ skipping 37 matching lines @@
 
   // Unregister ourselves from the engine.
   engine()->UnregisterChannel(this);
 }
 
 bool WebRtcVoiceMediaChannel::SetSendParameters(
     const AudioSendParameters& params) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   // TODO(pthatcher): Refactor this to be more clean now that we have
   // all the information at once.
-  return (SetSendCodecs(params.codecs) &&
-          SetSendRtpHeaderExtensions(params.extensions) &&
-          SetMaxSendBandwidth(params.max_bandwidth_bps) &&
-          SetOptions(params.options));
+
+  if (!SetSendCodecs(params.codecs)) {
+    return false;
+  }
+
+  std::vector<webrtc::RtpExtension> send_rtp_extensions =
+      FindAudioRtpHeaderExtensions(params.extensions);
+  if (send_rtp_extensions_ != send_rtp_extensions) {

[tommi, 2015/11/12 13:44:57]

does order matter or is it guaranteed that a vector of RtpExtension objects is
always sorted? (would it make sense to use a set?)

[the sun, 2015/11/12 15:16:44]

Order does not matter.

I don't think a set makes a lot of sense here; if we want to express the rules
surrounding rtp header extensions, there are other properties except for
uniqueness (id range, some are mutually exclusive, etc). Rather than using a
data structure that will communicate one such aspect, we could have a
specialized collection which verifies all. 

Such logic really belongs in code handling the SDP. Down here, we might just as
well pass them in a vector and DCHECK that things are sane...

[tommi, 2015/11/12 16:36:18]

Just in case we're talking about different things, my concern was about whether
the != operator would incorrectly handle a case like below or if a case like
below is not possible:

{1,2,3} != {3,2,1}

They're not equal only due to the order.
I might be misunderstanding things.

[the sun, 2015/11/16 13:49:51]

No, you're quite right. Sorry for being daft.

On closer inspection it turns out no validation of the header extension is
provided by layers higher up. The SDP is simply parsed and pushed down here. So,
order will matter, and in other ways other things (ID range, actual supported
extensions, etc) will also matter.

The video engine does a better job validating the supplied extensions, but the
code is contained to WViMC. I started out refactoring that but it shoots off in
a different direction from the changes here. Would you be ok if I do that in a
separate CL (this CL doesn't change the behavior we currently have)? 

[tommi, 2015/11/16 14:05:27]

Yes, another CL is fine.

+    send_rtp_extensions_ = send_rtp_extensions;

[tommi, 2015/11/12 13:44:56]

do the send streams have a pointer back to the channel or some other way to
access it?  I'm wondering if that should be used to access the extension list or
if it's better to copy the list into every stream. (if better to copy, is it a
performance optimization to avoid locks or something like that?)

[the sun, 2015/11/12 15:16:44]

There is some pessimisation here, yes. Keep in mind though that we are talking
about ~124 bytes to be copied for header extensions when streams are
created/reconfigured (which is quite rare). The extensions are stored in the
AudioSendStream::Config POD, and actually copied once more into the
internal::AudioSendStream.

Adding a pointer from WebRtcAudioSendStream back to WVoMC would make that
relationship circular, which IMO is tantamount to building technical debt. I
don't think any conceivable gain is warranted in this case.

Instead, if copying these bytes needs to be avoided at all costs, I would
suggest making each RtpExtension object ref counted, although that will slightly
increase the static memory load (by some 2x16 bytes, which is 26% of the object
sizes in this case). WViE should be changed to work the same too. Also, say
after me please; shared_ptr...

[tommi, 2015/11/12 16:36:18]

Agreed.  I don't want to add a circular relationship but I was wondering if
there already is one (and one might exist for the purposes of making state
accessible).

OK as is.

oh, shared_ptr!

[the sun, 2015/11/16 13:49:51]

Well, strictly speaking there is an implicit circular relationship to WVoMC
because the latter owns the VoE channel used here, but that will be going away.

[tommi, 2015/11/16 14:05:27]

Acknowledged.

+    for (auto& it : send_streams_) {
+      it.second->RecreateAudioSendStream(send_rtp_extensions);
+    }
+  }
+
+  if (!SetMaxSendBandwidth(params.max_bandwidth_bps)) {
+    return false;
+  }
+  if (!SetOptions(params.options)) {

[tommi, 2015/11/12 13:44:56]

nit:

return SetOptions(params.options);

[the sun, 2015/11/12 15:16:43]

Done.

+    return false;
+  }
+  return true;
 }
 
 bool WebRtcVoiceMediaChannel::SetRecvParameters(
     const AudioRecvParameters& params) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   // TODO(pthatcher): Refactor this to be more clean now that we have
   // all the information at once.
   return (SetRecvCodecs(params.codecs) &&
           SetRecvRtpHeaderExtensions(params.extensions));
 }
@@ skipping 360 matching lines @@
 
   for (const auto& ch : receive_channels_) {
     if (!SetChannelRecvRtpHeaderExtensions(ch.second->channel(), extensions)) {
       return false;
     }
   }
 
   receive_extensions_ = extensions;
 
   // Recreate AudioReceiveStream:s.
-  {
-    std::vector<webrtc::RtpExtension> exts;
-
-    const RtpHeaderExtension* audio_level_extension =
-        FindHeaderExtension(extensions, kRtpAudioLevelHeaderExtension);
-    if (audio_level_extension) {
-      exts.push_back({
-          kRtpAudioLevelHeaderExtension, audio_level_extension->id});
-    }
-
-    const RtpHeaderExtension* send_time_extension =
-        FindHeaderExtension(extensions, kRtpAbsoluteSenderTimeHeaderExtension);
-    if (send_time_extension) {
-      exts.push_back({
-          kRtpAbsoluteSenderTimeHeaderExtension, send_time_extension->id});
-    }
-
-    recv_rtp_extensions_.swap(exts);
-    RecreateAudioReceiveStreams();
-  }
+  recv_rtp_extensions_ = FindAudioRtpHeaderExtensions(extensions);
+  RecreateAudioReceiveStreams();
 
   return true;
 }
 
 bool WebRtcVoiceMediaChannel::SetChannelRecvRtpHeaderExtensions(
     int channel_id, const std::vector<RtpHeaderExtension>& extensions) {
   const RtpHeaderExtension* audio_level_extension =
       FindHeaderExtension(extensions, kRtpAudioLevelHeaderExtension);
   if (!SetHeaderExtension(
       &webrtc::VoERTP_RTCP::SetReceiveAudioLevelIndicationStatus, channel_id,
       audio_level_extension)) {
     return false;
   }
 
   const RtpHeaderExtension* send_time_extension =
       FindHeaderExtension(extensions, kRtpAbsoluteSenderTimeHeaderExtension);
   if (!SetHeaderExtension(
       &webrtc::VoERTP_RTCP::SetReceiveAbsoluteSenderTimeStatus, channel_id,
       send_time_extension)) {
     return false;
   }
 
   return true;
 }
 
-bool WebRtcVoiceMediaChannel::SetSendRtpHeaderExtensions(
-    const std::vector<RtpHeaderExtension>& extensions) {
-  RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-  if (send_extensions_ == extensions) {
-    return true;
-  }
-
-  for (const auto& ch : send_streams_) {
-    if (!SetChannelSendRtpHeaderExtensions(ch.second->channel(), extensions)) {
-      return false;
-    }
-  }
-
-  send_extensions_ = extensions;
-  return true;
-}
-
-bool WebRtcVoiceMediaChannel::SetChannelSendRtpHeaderExtensions(
-    int channel_id, const std::vector<RtpHeaderExtension>& extensions) {
-  const RtpHeaderExtension* audio_level_extension =
-      FindHeaderExtension(extensions, kRtpAudioLevelHeaderExtension);
-
-  if (!SetHeaderExtension(
-      &webrtc::VoERTP_RTCP::SetSendAudioLevelIndicationStatus, channel_id,
-      audio_level_extension)) {
-    return false;
-  }
-
-  const RtpHeaderExtension* send_time_extension =
-      FindHeaderExtension(extensions, kRtpAbsoluteSenderTimeHeaderExtension);
-  if (!SetHeaderExtension(
-      &webrtc::VoERTP_RTCP::SetSendAbsoluteSenderTimeStatus, channel_id,
-      send_time_extension)) {
-    return false;
-  }
-
-  return true;
-}
-
 bool WebRtcVoiceMediaChannel::SetPlayout(bool playout) {
   desired_playout_ = playout;
   return ChangePlayout(desired_playout_);
 }
 
 bool WebRtcVoiceMediaChannel::PausePlayout() {
   return ChangePlayout(false);
 }
 
 bool WebRtcVoiceMediaChannel::ResumePlayout() {
@@ skipping 133 matching lines @@
     LOG(LS_ERROR) << "Stream already exists with ssrc " << ssrc;
     return false;
   }
 
   // Create a new channel for sending audio data.
   int channel = CreateVoEChannel();
   if (channel == -1) {
     return false;
   }
 
-  // Enable RTCP (for quality stats and feedback messages).
-  if (engine()->voe()->rtp()->SetRTCPStatus(channel, true) == -1) {
-    LOG_RTCERR2(SetRTCPStatus, channel, 1);
-  }
-
-  SetChannelSendRtpHeaderExtensions(channel, send_extensions_);
-
-  // Set the local (send) SSRC.
-  if (engine()->voe()->rtp()->SetLocalSSRC(channel, ssrc) == -1) {
-    LOG_RTCERR2(SetLocalSSRC, channel, ssrc);
-    DeleteChannel(channel);
-    return false;
-  }
-
-  if (engine()->voe()->rtp()->SetRTCP_CNAME(channel, sp.cname.c_str()) == -1) {
-    LOG_RTCERR2(SetRTCP_CNAME, channel, sp.cname);
-    DeleteChannel(channel);
-    return false;
-  }
-
   // Save the channel to send_streams_, so that RemoveSendStream() can still
   // delete the channel in case failure happens below.
   webrtc::AudioTransport* audio_transport =
       engine()->voe()->base()->audio_transport();
-  send_streams_.insert(
-      std::make_pair(ssrc,
-          new WebRtcAudioSendStream(channel, audio_transport, ssrc, call_)));
+  send_streams_.insert(std::make_pair(ssrc, new WebRtcAudioSendStream(
+      channel, audio_transport, ssrc, sp.cname, send_rtp_extensions_, call_)));
 
   // Set the current codecs to be used for the new channel. We need to do this
   // after adding the channel to send_channels_, because of how max bitrate is
   // currently being configured by SetSendCodec().
   if (!send_codecs_.empty() && !SetSendCodecs(channel, send_codecs_)) {
     RemoveSendStream(ssrc);
     return false;
   }
 
   // At this point the channel's local SSRC has been updated. If the channel is
@@ skipping 11 matching lines @@
       LOG(LS_INFO) << "VoiceEngine channel #" << recv_channel
                    << " is associated with channel #" << channel << ".";
     }
   }
 
   return ChangeSend(channel, desired_send_);
 }
 
 bool WebRtcVoiceMediaChannel::RemoveSendStream(uint32_t ssrc) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+  LOG(LS_INFO) << "RemoveSendStream: " << ssrc;
+
   auto it = send_streams_.find(ssrc);
   if (it == send_streams_.end()) {
     LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc
                     << " which doesn't exist.";
     return false;
   }
 
   int channel = it->second->channel();
   ChangeSend(channel, SEND_NOTHING);
 
@@ skipping 680 matching lines @@
       LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
       return false;
     }
   }
   return true;
 }
 
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE