Update smoothed bitrate.

webrtc/audio/audio_send_stream.cc

 /*
  *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/audio/audio_send_stream.h"
 
 #include <string>
+#include <utility>
 
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/audio/scoped_voe_interface.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/event.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/task_queue.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/pacing/paced_sender.h"
@@ skipping 10 matching lines @@
 namespace {
 
 constexpr char kOpusCodecName[] = "opus";
 
 bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
   return (_stricmp(codec.plname, ref_name) == 0);
 }
 }  // namespace
 
 namespace internal {
+
+class AudioSendStream::AdaptCodecTask : public rtc::QueuedTask {
+ public:
+  explicit AdaptCodecTask(const rtc::WeakPtr<AudioSendStream>& send_stream)
+      : send_stream_(std::move(send_stream)) {}
+
+ private:
+  bool Run() override {
+    if (send_stream_) {

[stefan-webrtc, 2016/12/13 12:29:55]

Are we not able to DCHECK this because the worker queue lives longer than the
send stream?

[michaelt, 2016/12/13 17:01:26]

Yes and that is as well the reason i used weak pointer her.

+      send_stream_->AdaptCodec();
+    }
+    return true;
+  }
+
+  rtc::WeakPtr<AudioSendStream> send_stream_;
+};
+
 AudioSendStream::AudioSendStream(
     const webrtc::AudioSendStream::Config& config,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
     rtc::TaskQueue* worker_queue,
     PacketRouter* packet_router,
     CongestionController* congestion_controller,
     BitrateAllocator* bitrate_allocator,
     RtcEventLog* event_log,
     RtcpRttStats* rtcp_rtt_stats)
     : worker_queue_(worker_queue),
@@ skipping 44 matching lines @@
   channel_proxy_->SetRtcEventLog(nullptr);
   channel_proxy_->SetRtcpRttStats(nullptr);
 }
 
 void AudioSendStream::Start() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   if (config_.min_bitrate_bps != -1 && config_.max_bitrate_bps != -1) {
     RTC_DCHECK_GE(config_.max_bitrate_bps, config_.min_bitrate_bps);
     rtc::Event thread_sync_event(false /* manual_reset */, false);
     worker_queue_->PostTask([this, &thread_sync_event] {
+      weak_ptr_factory_.reset(new rtc::WeakPtrFactory<AudioSendStream>(this));
+      weak_ptr_ = weak_ptr_factory_->GetWeakPtr();
       bitrate_allocator_->AddObserver(this, config_.min_bitrate_bps,
                                       config_.max_bitrate_bps, 0, true);
       thread_sync_event.Set();
     });
     thread_sync_event.Wait(rtc::Event::kForever);
   }
 
   ScopedVoEInterface<VoEBase> base(voice_engine());
   int error = base->StartSend(config_.voe_channel_id);
   if (error != 0) {
     LOG(LS_ERROR) << "AudioSendStream::Start failed with error: " << error;
   }
 }
 
 void AudioSendStream::Stop() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   rtc::Event thread_sync_event(false /* manual_reset */, false);
   worker_queue_->PostTask([this, &thread_sync_event] {
     bitrate_allocator_->RemoveObserver(this);
+    weak_ptr_factory_.reset(nullptr);
     thread_sync_event.Set();
   });
   thread_sync_event.Wait(rtc::Event::kForever);
 
   ScopedVoEInterface<VoEBase> base(voice_engine());
   int error = base->StopSend(config_.voe_channel_id);
   if (error != 0) {
     LOG(LS_ERROR) << "AudioSendStream::Stop failed with error: " << error;
   }
 }
@@ skipping 90 matching lines @@
   // calls on the worker thread. We should move towards always using a network
   // thread. Then this check can be enabled.
   // RTC_DCHECK(!thread_checker_.CalledOnValidThread());
   return channel_proxy_->ReceivedRTCPPacket(packet, length);
 }
 
 uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps,
                                            uint8_t fraction_loss,
                                            int64_t rtt,
                                            int64_t probing_interval_ms) {
+  RTC_DCHECK_RUN_ON(worker_queue_);
   RTC_DCHECK_GE(bitrate_bps,
                 static_cast<uint32_t>(config_.min_bitrate_bps));
   // The bitrate allocator might allocate an higher than max configured bitrate
   // if there is room, to allow for, as example, extra FEC. Ignore that for now.
   const uint32_t max_bitrate_bps = config_.max_bitrate_bps;
   if (bitrate_bps > max_bitrate_bps)
     bitrate_bps = max_bitrate_bps;
 
   channel_proxy_->SetBitrate(bitrate_bps, probing_interval_ms);
 
+  if (first_update_bitrate_()) {

[stefan-webrtc, 2016/12/13 12:29:55]

I think it would make sense to comment on why this is only done the first time.

[michaelt, 2016/12/13 17:01:26]

Done.

+    worker_queue_->PostTask([this]() { AdaptCodec(); });

[the sun, 2016/12/15 14:58:37]

When is OnBitrateUpdated() called?

IIUC you're starting the periodic task here and it will call
channel_proxy_->AdaptCodec(); the first time, and then not call it until Start()
has been called on the stream.

The conditions here appear to me unclear and subtle. I'd like to make the code
simpler.

[michaelt, 2016/12/19 14:43:28]

I removed the conditions here a start the task in Start();

+  }
+
   // The amount of audio protection is not exposed by the encoder, hence
   // always returning 0.
   return 0;
 }
 
 const webrtc::AudioSendStream::Config& AudioSendStream::config() const {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   return config_;
 }
 
@@ skipping 122 matching lines @@
       // interaction between VAD and Opus FEC.
       if (codec->SetVADStatus(channel, true) != 0) {
         LOG(LS_WARNING) << "SetVADStatus() failed: " << base->LastError();
         return false;
       }
     }
   }
   return true;
 }
 
+void AudioSendStream::AdaptCodec() {
+  RTC_DCHECK_RUN_ON(worker_queue_);
+  channel_proxy_->AdaptCodec();
+  constexpr uint32_t kAdaptCodecIntervalMs = 200;
+  worker_queue_->PostDelayedTask(

[stefan-webrtc, 2016/12/13 12:29:55]

Is there a point in posting another task here? Can't you do the work directly
since you're already on the worker queue?

[michaelt, 2016/12/13 17:01:26]

This post is the trick of this CL.
We have to call channel_proxy_->AdaptCodec() in timely base(each 200ms) as long
that AudioSendStream is running.

+      std::unique_ptr<rtc::QueuedTask>(new AdaptCodecTask(weak_ptr_)),

[the sun, 2016/12/13 16:43:24]

Can you just post a lambda here, calling AdaptCodec()?

I'm not sure you need to weak_ptr logic either - you're using it as a flag, so
why not just have a flag?

[michaelt, 2016/12/13 17:01:26]

The QueuedTask and the weak pointer implemented to make sure we will not run in
to trouble when AudioSendStream is deleted but we still have a AdaptCodecTask in
the queue. Because the working queue is not owned by the AudioSendStream the
queue can life longer than the stream.
     

[the sun, 2016/12/15 14:58:37]

I see. A subtle effect here is that you won't be receiving calls strictly
periodically. AudioSendStreams are recreated now and then, even while sending is
started. Just FYI.

I think you can arrange the code a little nicer by following example #3 in
base/task_queue.h - reposting "this". IIUC you could keep the WeakPtrFactory in
AudioSendStream and the WeakPtr only in AdaptCodecTask. I think that would be a
nicer separation.

[michaelt, 2016/12/19 14:43:28]

Done.

+      kAdaptCodecIntervalMs);
+}
+
 }  // namespace internal
 }  // namespace webrtc