Reconfigure, not reconstruct, AudioReceiveStreams.

In preparation of moving ownership of voe::Channel to the audio stream
classes, semantics for changing configuration properties on the receive
streams need to change, otherwise RTP, audio decoding and NetEq state
will be discarded when streams are recreated. The same pattern as for
AudioSendStream is applied, and the reconfigurable information is kept
to a minimum.

AudioReceiveStream:s may still be recreated when an unsignaled stream
is 'promoted' to signaled state, and the sync label changes at the
same time.

Bug: webrtc:4690
Change-Id: Ibad282965310c3c8174a91e05a659fa3e1827607
Reviewed-on: https://webrtc-review.googlesource.com/38300
Reviewed-by: Oskar Sundbom <ossu@webrtc.org>
Commit-Queue: Fredrik Solenberg <solenberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#21560}

audio/audio_receive_stream.cc

@@ -68,22 +68,16 @@ AudioReceiveStream::AudioReceiveStream(
     const webrtc::AudioReceiveStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
     webrtc::RtcEventLog* event_log)
-    : config_(config), audio_state_(audio_state) {
+    : audio_state_(audio_state) {
-  RTC_LOG(LS_INFO) << "AudioReceiveStream: " << config_.ToString();
+  RTC_LOG(LS_INFO) << "AudioReceiveStream: " << config.ToString();
-  RTC_DCHECK_NE(config_.voe_channel_id, -1);
+  RTC_DCHECK_NE(config.voe_channel_id, -1);
   RTC_DCHECK(audio_state_.get());
   RTC_DCHECK(packet_router);
 
   module_process_thread_checker_.DetachFromThread();
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());
-  channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
+  channel_proxy_ = voe_impl->GetChannelProxy(config.voe_channel_id);
-  channel_proxy_->SetRtcEventLog(event_log);
-  channel_proxy_->SetLocalSSRC(config.rtp.local_ssrc);
-  // TODO(solenberg): Config NACK history window (which is a packet count),
-  // using the actual packet size for the configured codec.
-  channel_proxy_->SetNACKStatus(config_.rtp.nack.rtp_history_ms != 0,
-                                config_.rtp.nack.rtp_history_ms / 20);
 
   // TODO(ossu): This is where we'd like to set the decoder factory to
   // use. However, since it needs to be included when constructing Channel, we
@@ -94,25 +88,18 @@ AudioReceiveStream::AudioReceiveStream(
   RTC_CHECK_EQ(config.decoder_factory,
                channel_proxy_->GetAudioDecoderFactory());
 
+  channel_proxy_->SetRtcEventLog(event_log);
   channel_proxy_->RegisterTransport(config.rtcp_send_transport);
-  channel_proxy_->SetReceiveCodecs(config.decoder_map);
 
-  for (const auto& extension : config.rtp.extensions) {
-    if (extension.uri == RtpExtension::kAudioLevelUri) {
-      channel_proxy_->SetReceiveAudioLevelIndicationStatus(true, extension.id);
-    } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
-      channel_proxy_->EnableReceiveTransportSequenceNumber(extension.id);
-    } else {
-      RTC_NOTREACHED() << "Unsupported RTP extension.";
-    }
-  }
   // Configure bandwidth estimation.
   channel_proxy_->RegisterReceiverCongestionControlObjects(packet_router);
 
   // Register with transport.
   rtp_stream_receiver_ =
-      receiver_controller->CreateReceiver(config_.rtp.remote_ssrc,
+      receiver_controller->CreateReceiver(config.rtp.remote_ssrc,
                                           channel_proxy_.get());
+
+  ConfigureStream(this, config, true);
 }
 
 AudioReceiveStream::~AudioReceiveStream() {
@@ -125,6 +112,13 @@ AudioReceiveStream::~AudioReceiveStream() {
   channel_proxy_->SetRtcEventLog(nullptr);
 }
 
+void AudioReceiveStream::Reconfigure(
+    const webrtc::AudioReceiveStream::Config& config) {
+  RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+  RTC_LOG(LS_INFO) << "AudioReceiveStream::Reconfigure: " << config_.ToString();
+  ConfigureStream(this, config, false);
+}
+
 void AudioReceiveStream::Start() {
   RTC_DCHECK_RUN_ON(&worker_thread_checker_);
   if (playing_) {
@@ -329,5 +323,67 @@ internal::AudioState* AudioReceiveStream::audio_state() const {
   RTC_DCHECK(audio_state);
   return audio_state;
 }
+
+AudioReceiveStream::ExtensionIds AudioReceiveStream::FindExtensionIds(
+    const std::vector<RtpExtension>& extensions) {
+  ExtensionIds ids;
+  for (const auto& extension : extensions) {
+    if (extension.uri == RtpExtension::kAudioLevelUri) {
+      ids.audio_level = extension.id;
+    } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
+      ids.transport_sequence_number = extension.id;
+    }
+  }
+  return ids;
+}
+
+void AudioReceiveStream::ConfigureStream(AudioReceiveStream* stream,
+                                         const Config& new_config,
+                                         bool first_time) {
+  RTC_DCHECK(stream);
+  const auto& channel_proxy = stream->channel_proxy_;
+  const auto& old_config = stream->config_;
+
+  // Configuration parameters which cannot be changed.
+  RTC_DCHECK(first_time ||
+             old_config.voe_channel_id == new_config.voe_channel_id);
+  RTC_DCHECK(first_time ||
+             old_config.rtp.remote_ssrc == new_config.rtp.remote_ssrc);
+  RTC_DCHECK(first_time ||
+             old_config.rtcp_send_transport == new_config.rtcp_send_transport);
+  // Decoder factory cannot be changed because it is configured at
+  // voe::Channel construction time.
+  RTC_DCHECK(first_time ||
+             old_config.decoder_factory == new_config.decoder_factory);
+
+  if (first_time || old_config.rtp.local_ssrc != new_config.rtp.local_ssrc) {
+    channel_proxy->SetLocalSSRC(new_config.rtp.local_ssrc);
+  }
+  // TODO(solenberg): Config NACK history window (which is a packet count),
+  // using the actual packet size for the configured codec.
+  if (first_time || old_config.rtp.nack.rtp_history_ms !=
+                        new_config.rtp.nack.rtp_history_ms) {
+    channel_proxy->SetNACKStatus(new_config.rtp.nack.rtp_history_ms != 0,
+                                 new_config.rtp.nack.rtp_history_ms / 20);
+  }
+  if (first_time || old_config.decoder_map != new_config.decoder_map) {
+    channel_proxy->SetReceiveCodecs(new_config.decoder_map);
+  }
+
+  // RTP Header Extensions.
+  const ExtensionIds old_ids = FindExtensionIds(old_config.rtp.extensions);
+  const ExtensionIds new_ids = FindExtensionIds(new_config.rtp.extensions);
+  if (first_time || new_ids.audio_level != old_ids.audio_level) {
+    channel_proxy->SetReceiveAudioLevelIndicationStatus(
+        new_ids.audio_level != 0, new_ids.audio_level);
+  }
+  if (first_time || new_ids.transport_sequence_number !=
+                        old_ids.transport_sequence_number) {
+    channel_proxy->EnableReceiveTransportSequenceNumber(
+        new_ids.transport_sequence_number);
+  }
+
+  stream->config_ = new_config;
+}
 }  // namespace internal
 }  // namespace webrtc

audio/audio_receive_stream.h

@@ -48,6 +48,7 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream,
   ~AudioReceiveStream() override;
 
   // webrtc::AudioReceiveStream implementation.
+  void Reconfigure(const webrtc::AudioReceiveStream::Config& config) override;
   void Start() override;
   void Stop() override;
   webrtc::AudioReceiveStream::Stats GetStats() const override;
@@ -80,12 +81,25 @@ class AudioReceiveStream final : public webrtc::AudioReceiveStream,
   const webrtc::AudioReceiveStream::Config& config() const;
 
  private:
+  // RFC 5285: Each distinct extension MUST have a unique ID. The value 0 is
+  // reserved for padding and MUST NOT be used as a local identifier.
+  // So it should be safe to use 0 here to indicate "not configured".
+  struct ExtensionIds {
+    int audio_level = 0;
+    int transport_sequence_number = 0;
+  };
+  static ExtensionIds FindExtensionIds(
+      const std::vector<RtpExtension>& extensions);
+  static void ConfigureStream(AudioReceiveStream* stream,
+                              const Config& new_config,
+                              bool first_time);
+
   VoiceEngine* voice_engine() const;
   AudioState* audio_state() const;
 
   rtc::ThreadChecker worker_thread_checker_;
   rtc::ThreadChecker module_process_thread_checker_;
-  const webrtc::AudioReceiveStream::Config config_;
+  webrtc::AudioReceiveStream::Config config_;
   rtc::scoped_refptr<webrtc::AudioState> audio_state_;
   std::unique_ptr<voe::ChannelProxy> channel_proxy_;
 

audio/audio_receive_stream_unittest.cc

@@ -394,5 +394,46 @@ TEST(AudioReceiveStreamTest, StreamsShouldBeAddedToMixerOnceOnStart) {
   // Stop stream before it is being destructed.
   recv_stream2.Stop();
 }
+
+TEST(AudioReceiveStreamTest, ReconfigureWithSameConfig) {
+  ConfigHelper helper;
+  internal::AudioReceiveStream recv_stream(
+      helper.rtp_stream_receiver_controller(),
+      helper.packet_router(),
+      helper.config(), helper.audio_state(), helper.event_log());
+  recv_stream.Reconfigure(helper.config());
+}
+
+TEST(AudioReceiveStreamTest, ReconfigureWithUpdatedConfig) {
+  ConfigHelper helper;
+  internal::AudioReceiveStream recv_stream(
+      helper.rtp_stream_receiver_controller(),
+      helper.packet_router(),
+      helper.config(), helper.audio_state(), helper.event_log());
+
+  auto new_config = helper.config();
+  new_config.rtp.local_ssrc = kLocalSsrc + 1;
+  new_config.rtp.nack.rtp_history_ms = 300 + 20;
+  new_config.rtp.extensions.clear();
+  new_config.rtp.extensions.push_back(
+        RtpExtension(RtpExtension::kAudioLevelUri, kAudioLevelId + 1));
+  new_config.rtp.extensions.push_back(RtpExtension(
+        RtpExtension::kTransportSequenceNumberUri,
+        kTransportSequenceNumberId + 1));
+  new_config.decoder_map.emplace(1, SdpAudioFormat("foo", 8000, 1));
+
+  MockVoEChannelProxy& channel_proxy = *helper.channel_proxy();
+  EXPECT_CALL(channel_proxy, SetLocalSSRC(kLocalSsrc + 1)).Times(1);
+  EXPECT_CALL(channel_proxy, SetNACKStatus(true, 15 + 1)).Times(1);
+  EXPECT_CALL(channel_proxy, SetReceiveCodecs(new_config.decoder_map));
+  EXPECT_CALL(channel_proxy,
+      SetReceiveAudioLevelIndicationStatus(true, kAudioLevelId + 1))
+          .Times(1);
+  EXPECT_CALL(channel_proxy,
+      EnableReceiveTransportSequenceNumber(kTransportSequenceNumberId + 1))
+          .Times(1);
+
+  recv_stream.Reconfigure(new_config);
+}
 }  // namespace test
 }  // namespace webrtc

call/audio_receive_stream.h

@@ -120,6 +120,9 @@ class AudioReceiveStream {
     rtc::scoped_refptr<AudioDecoderFactory> decoder_factory;
   };
 
+  // Reconfigure the stream according to the Configuration.
+  virtual void Reconfigure(const Config& config) = 0;
+
   // Starts stream activity.
   // When a stream is active, it can receive, process and deliver packets.
   virtual void Start() = 0;

media/engine/fakewebrtccall.cc

@@ -98,6 +98,11 @@ bool FakeAudioReceiveStream::DeliverRtp(const uint8_t* packet,
   return true;
 }
 
+void FakeAudioReceiveStream::Reconfigure(
+    const webrtc::AudioReceiveStream::Config& config) {
+  config_ = config;
+}
+
 webrtc::AudioReceiveStream::Stats FakeAudioReceiveStream::GetStats() const {
   return stats_;
 }

media/engine/fakewebrtccall.h

@@ -57,7 +57,6 @@ class FakeAudioSendStream final : public webrtc::AudioSendStream {
  private:
   // webrtc::AudioSendStream implementation.
   void Reconfigure(const webrtc::AudioSendStream::Config& config) override;
-
   void Start() override { sending_ = true; }
   void Stop() override { sending_ = false; }
   void SendAudioData(std::unique_ptr<webrtc::AudioFrame> audio_frame) override {
@@ -96,6 +95,7 @@ class FakeAudioReceiveStream final : public webrtc::AudioReceiveStream {
 
  private:
   // webrtc::AudioReceiveStream implementation.
+  void Reconfigure(const webrtc::AudioReceiveStream::Config& config) override;
   void Start() override { started_ = true; }
   void Stop() override { started_ = false; }
 

media/engine/webrtcvoiceengine.cc

@@ -1185,37 +1185,38 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
     call_->DestroyAudioReceiveStream(stream_);
   }
 
-  void RecreateAudioReceiveStream(uint32_t local_ssrc) {
+  void SetLocalSsrc(uint32_t local_ssrc) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     config_.rtp.local_ssrc = local_ssrc;
-    RecreateAudioReceiveStream();
+    ReconfigureAudioReceiveStream();
   }
 
-  void RecreateAudioReceiveStream(bool use_transport_cc, bool use_nack) {
+  void SetUseTransportCc(bool use_transport_cc, bool use_nack) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     config_.rtp.transport_cc = use_transport_cc;
     config_.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0;
-    RecreateAudioReceiveStream();
+    ReconfigureAudioReceiveStream();
   }
 
-  void RecreateAudioReceiveStream(
+  void SetRtpExtensions(const std::vector<webrtc::RtpExtension>& extensions) {
-      const std::vector<webrtc::RtpExtension>& extensions) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     config_.rtp.extensions = extensions;
-    RecreateAudioReceiveStream();
+    ReconfigureAudioReceiveStream();
   }
 
   // Set a new payload type -> decoder map.
-  void RecreateAudioReceiveStream(
+  void SetDecoderMap(const std::map<int, webrtc::SdpAudioFormat>& decoder_map) {
-      const std::map<int, webrtc::SdpAudioFormat>& decoder_map) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     config_.decoder_map = decoder_map;
-    RecreateAudioReceiveStream();
+    ReconfigureAudioReceiveStream();
   }
 
   void MaybeRecreateAudioReceiveStream(const std::string& sync_group) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (config_.sync_group != sync_group) {
+      RTC_LOG(LS_INFO) << "Recreating AudioReceiveStream for SSRC="
+                       << config_.rtp.remote_ssrc
+                       << " because of sync group change.";
       config_.sync_group = sync_group;
       RecreateAudioReceiveStream();
     }
@@ -1278,6 +1279,12 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
     SetPlayout(playout_);
   }
 
+  void ReconfigureAudioReceiveStream() {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    RTC_DCHECK(stream_);
+    stream_->Reconfigure(config_);
+  }
+
   rtc::ThreadChecker worker_thread_checker_;
   webrtc::Call* call_ = nullptr;
   webrtc::AudioReceiveStream::Config config_;
@@ -1373,7 +1380,7 @@ bool WebRtcVoiceMediaChannel::SetRecvParameters(
   if (recv_rtp_extensions_ != filtered_extensions) {
     recv_rtp_extensions_.swap(filtered_extensions);
     for (auto& it : recv_streams_) {
-      it.second->RecreateAudioReceiveStream(recv_rtp_extensions_);
+      it.second->SetRtpExtensions(recv_rtp_extensions_);
     }
   }
   return true;
@@ -1588,7 +1595,7 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
 
   decoder_map_ = std::move(decoder_map);
   for (auto& kv : recv_streams_) {
-    kv.second->RecreateAudioReceiveStream(decoder_map_);
+    kv.second->SetDecoderMap(decoder_map_);
   }
   recv_codecs_ = codecs;
 
@@ -1720,8 +1727,8 @@ bool WebRtcVoiceMediaChannel::SetSendCodecs(
     recv_transport_cc_enabled_ = send_codec_spec_->transport_cc_enabled;
     recv_nack_enabled_ = send_codec_spec_->nack_enabled;
     for (auto& kv : recv_streams_) {
-      kv.second->RecreateAudioReceiveStream(recv_transport_cc_enabled_,
+      kv.second->SetUseTransportCc(recv_transport_cc_enabled_,
-                                            recv_nack_enabled_);
+                                   recv_nack_enabled_);
     }
   }
 
@@ -1846,8 +1853,8 @@ bool WebRtcVoiceMediaChannel::AddSendStream(const StreamParams& sp) {
     receiver_reports_ssrc_ = ssrc;
     for (const auto& kv : recv_streams_) {
       // TODO(solenberg): Allow applications to set the RTCP SSRC of receive
-      // streams instead, so we can avoid recreating the streams here.
+      // streams instead, so we can avoid reconfiguring the streams here.
-      kv.second->RecreateAudioReceiveStream(ssrc);
+      kv.second->SetLocalSsrc(ssrc);
     }
   }
 

voice_engine/channel.cc

@@ -1168,9 +1168,11 @@ void Channel::EnableSendTransportSequenceNumber(int id) {
 void Channel::EnableReceiveTransportSequenceNumber(int id) {
   rtp_header_parser_->DeregisterRtpHeaderExtension(
       kRtpExtensionTransportSequenceNumber);
-  bool ret = rtp_header_parser_->RegisterRtpHeaderExtension(
+  if (id != 0) {
-      kRtpExtensionTransportSequenceNumber, id);
+    bool ret = rtp_header_parser_->RegisterRtpHeaderExtension(
-  RTC_DCHECK(ret);
+        kRtpExtensionTransportSequenceNumber, id);
+    RTC_DCHECK(ret);
+  }
 }
 
 void Channel::RegisterSenderCongestionControlObjects(