Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546493002:
Update smoothed bitrate. (Closed)
DescriptionUpdate smoothed bitrate.
BUG=webrtc:6443
Review-Url: https://codereview.webrtc.org/2546493002
Cr-Commit-Position: refs/heads/master@{#16036}
Committed: https://chromium.googlesource.com/external/webrtc/+/566d820e004700df9a152602cbda44fac839d7e7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix thread safety problem and changed smoothing filter. #
Total comments: 1
Patch Set 3 : Removed smoothing filter changes from CL. #Patch Set 4 : Fix for unittest. #Patch Set 5 : Check if AdaptCodec runs on worker queue. #
Total comments: 8
Patch Set 6 : Sync worker queue for unittests. #Patch Set 7 : Response to comments. #
Total comments: 16
Patch Set 8 : Rebased #Patch Set 9 : Response to comments #
Total comments: 2
Patch Set 10 : Response to comments. #
Total comments: 6
Patch Set 11 : Response to comments #
Total comments: 4
Patch Set 12 : Response to comments #
Total comments: 4
Patch Set 13 : Responde to comments. #Patch Set 14 : Changed cl according to offline discussions. #
Total comments: 20
Patch Set 15 : Rename MayUpdateUplinkBandwidth to MaybeUpdateUplinkBandwidth #Patch Set 16 : Response to comments. #Patch Set 17 : Deprecated OnReceivedTargetAudioBitrate. #
Total comments: 8
Patch Set 18 : Respond to comments #Patch Set 19 : Fix opus dependency #Patch Set 20 : Fix android build error #
Total comments: 2
Patch Set 21 : Response to comments. #Patch Set 22 : fix clock set to nullptr by accident. #Patch Set 23 : Add metrics::Enable() to unittest main to activate metrics. #Patch Set 24 : Fix linking error #Patch Set 25 : Response to comments #Patch Set 26 : Rebased #Patch Set 27 : Fix for windwos build #Patch Set 28 : Renamed OnReceivedTargetAudioBitrate to OnReceivedUplinkBandwidth #Messages
Total messages: 186 (139 generated)
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
Good work! this looks very promising. But my preference would be 1. Call a method "AdaptCodec" in a regular time basis. 2. The method query BWE smoother for new value 3. Change smoother so that it gives output based on when it is queried (so we don't add "virtual" samples explicitly) In this way, data races will only be possible in smoother. we may need to add thread checker etc there.
solenberg@webrtc.org changed reviewers: + solenberg@webrtc.org
https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:230: uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps, If we can't RTC_DCHECK(thread_checker_.CalledOnValidThread()); please document what thread(s) we expect this method runs on. https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:243: if (first_update_bitrate_()) nit: always {} in this file, even for one-liners https://codereview.webrtc.org/2546493002/diff/1/webrtc/voice_engine/channel_p... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2546493002/diff/1/webrtc/voice_engine/channel_p... webrtc/voice_engine/channel_proxy.cc:232: int ChannelProxy::NeededFrequency() const { Comment about thread affinity missing
https://codereview.webrtc.org/2546493002/diff/20001/webrtc/common_audio/smoot... File webrtc/common_audio/smoothing_filter.cc (right): https://codereview.webrtc.org/2546493002/diff/20001/webrtc/common_audio/smoot... webrtc/common_audio/smoothing_filter.cc:62: return rtc::Optional<float>( I like the idea of making the query of average time dependent. But is this mathematically rigorous? It feels to me that we use exponential filter when we add sample and use another filter when we query the average
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_more_configs/bu...) ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13012) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/15374) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15234) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/20590) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19311) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10944)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13028) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15248) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/19328) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10950)
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10955)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/10963)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11063)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/11069)
https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:230: uint32_t AudioSendStream::OnBitrateUpdated(uint32_t bitrate_bps, OnBitrateUpdated is called on the worker queue. https://cs.chromium.org/chromium/src/third_party/webrtc/base/task_queue.h?q=r... Since TaskQueue don't guarantee to be called on the same thread, a thread_checker may fail. I could check if the function is called on the worker queue. https://codereview.webrtc.org/2546493002/diff/1/webrtc/audio/audio_send_strea... webrtc/audio/audio_send_stream.cc:243: if (first_update_bitrate_()) On 2016/12/01 09:16:55, the sun wrote: > nit: always {} in this file, even for one-liners Done. https://codereview.webrtc.org/2546493002/diff/1/webrtc/voice_engine/channel_p... File webrtc/voice_engine/channel_proxy.cc (right): https://codereview.webrtc.org/2546493002/diff/1/webrtc/voice_engine/channel_p... webrtc/voice_engine/channel_proxy.cc:232: int ChannelProxy::NeededFrequency() const { I think we should fix this in a separate CL, since it seams unrelated to this CL.
Description was changed from ========== Update smoothed bitrate. BUG= ========== to ========== Update smoothed bitrate. BUG= ==========
michaelt@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2546493002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:417: constexpr uint32_t kAdaptCodecIntervalMs = 200; any reason for this value? https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1356: bitrate_bps_ = rtc::Optional<int>(bitrate_bps); With our new smoothing filter planned, we can add sample here bitrate_smoother_.AddSample(bitrate_bps) https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2757: bitrate_smoother_.AddSample(*bitrate_bps_); with our new smoothing filter planned, there is no need to add sample here https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:552: rtc::CriticalSection smoothed_bitrate_lock_; how about calling it "bitrate_smoother_lock_"
https://codereview.webrtc.org/2546493002/diff/140001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:417: constexpr uint32_t kAdaptCodecIntervalMs = 200; No this was just a first guess. https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1356: bitrate_bps_ = rtc::Optional<int>(bitrate_bps); Will rebase as soon that the changed filter is landed. https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2757: bitrate_smoother_.AddSample(*bitrate_bps_); Will rebase as soon that the changed filter is landed. https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2546493002/diff/140001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:552: rtc::CriticalSection smoothed_bitrate_lock_; On 2016/12/07 16:44:44, minyue-webrtc wrote: > how about calling it "bitrate_smoother_lock_" Done.
https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:53: if (send_stream_) { Are we not able to DCHECK this because the worker queue lives longer than the send stream? https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:269: if (first_update_bitrate_()) { I think it would make sense to comment on why this is only done the first time. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:419: worker_queue_->PostDelayedTask( Is there a point in posting another task here? Can't you do the work directly since you're already on the worker queue? https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:109: rtc::RaceChecker worker_queue_checker_; worker_queue_race_checker_ Or do you really want a rtc::SequencedTaskChecker?
https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:420: std::unique_ptr<rtc::QueuedTask>(new AdaptCodecTask(weak_ptr_)), 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? https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2755: rtc::CritScope lock(&bitrate_smoother_lock_); Which threads can we be running on? Is it possible to use a thread checker instead?
https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:53: if (send_stream_) { Yes and that is as well the reason i used weak pointer her. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:269: if (first_update_bitrate_()) { On 2016/12/13 12:29:55, stefan-webrtc (holmer) wrote: > I think it would make sense to comment on why this is only done the first time. Done. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:419: worker_queue_->PostDelayedTask( 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. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:420: std::unique_ptr<rtc::QueuedTask>(new AdaptCodecTask(weak_ptr_)), 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. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2755: rtc::CritScope lock(&bitrate_smoother_lock_); No, since AdaptCodec() runs on the worker queue. And SetBitRate on the network thread. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel_proxy.h (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/voice_engine/chan... webrtc/voice_engine/channel_proxy.h:109: rtc::RaceChecker worker_queue_checker_; Changed it to a SequencedTaskChecker
lgtm
adding one comment https://codereview.webrtc.org/2546493002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:422: constexpr uint32_t kAdaptCodecIntervalMs = 200; oh, this, how about making it a config_ field?
https://codereview.webrtc.org/2546493002/diff/220001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/220001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:422: constexpr uint32_t kAdaptCodecIntervalMs = 200; On 2016/12/13 17:21:27, minyue-webrtc wrote: > oh, this, how about making it a config_ field? Done.
lgtm % nit https://codereview.webrtc.org/2546493002/diff/240001/webrtc/call/audio_send_s... File webrtc/call/audio_send_stream.h (right): https://codereview.webrtc.org/2546493002/diff/240001/webrtc/call/audio_send_s... webrtc/call/audio_send_stream.h:103: // Interval in which adapt codec is called. Default is an interval of 200ms. Time interval on which AdaptCodec is called. (no need to say default value, since it is clear by the code.)
https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:128: weak_ptr_ = weak_ptr_factory_->GetWeakPtr(); We should give weak_ptr_ a more descriptive name. https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:273: worker_queue_->PostTask([this]() { AdaptCodec(); }); It's still not clear to me why we can't call AdaptCodec() here directly? You're already running on the worker queue, so there should be no reason to post it?
https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:128: weak_ptr_ = weak_ptr_factory_->GetWeakPtr(); On 2016/12/14 11:58:33, stefan-webrtc (holmer) wrote: > We should give weak_ptr_ a more descriptive name. Done. https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:273: worker_queue_->PostTask([this]() { AdaptCodec(); }); I removed "RTC_DCHECK_RUN_ON(worker_queue_);" since the function is called from the unit test which are not run on the worker queue. I leaf "worker_queue_->PostTask([this]() { AdaptCodec(); });" as it is for the same reason.
Description was changed from ========== Update smoothed bitrate. BUG= ========== to ========== Update smoothed bitrate. BUG=webrtc:6443 ==========
lgtm https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/240001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:273: worker_queue_->PostTask([this]() { AdaptCodec(); }); On 2016/12/14 13:45:51, michaelt wrote: > I removed "RTC_DCHECK_RUN_ON(worker_queue_);" since the function is called from > the unit test which are not run on the worker queue. > > I leaf "worker_queue_->PostTask([this]() { AdaptCodec(); });" as it is for the > same reason. So we want to allow calling OnBitrateUpdated on a queue different than the worker queue? I don't want us to reduce the restrictions due to test purposes. I'll leave that the decision to Fredrik and Minyue though.
lgtm
https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:270: worker_queue_->PostTask([this]() { AdaptCodec(); }); 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. https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:420: std::unique_ptr<rtc::QueuedTask>(new AdaptCodecTask(weak_ptr_)), On 2016/12/13 17:01:26, michaelt wrote: > 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. > > 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.
solenberg@webrtc.org changed reviewers: + ossu@webrtc.org
On 2016/12/15 14:58:38, the sun wrote: > https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:270: worker_queue_->PostTask([this]() { > AdaptCodec(); }); > 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. > > https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:420: std::unique_ptr<rtc::QueuedTask>(new > AdaptCodecTask(weak_ptr_)), > On 2016/12/13 17:01:26, michaelt wrote: > > 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. > > > > > > 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. Adding ossu@ for another pair of eyes.
Why is bitrate smoothing put in AudioSendStream? It seems to me either something only ANA should be concerned with, and so should be done somewhere down in that implementation, or it's a problem with our bandwidth estimator (producing spikey estimates), which should be solved at the source. Why does the setbitrate thing need to be called at regular intervals like this? Can't it just collect the information as it gets it and aggregate it when needed, i.e. when it's time to encode a frame. https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:49: : send_stream_(std::move(send_stream)) {} How do you move from a const&? https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:421: worker_queue_->PostDelayedTask( Why isn't this up in AdaptCodecTask and why isn't it making use of the ability to re-post a task as described in task_queue.h?
On 2016/12/19 14:09:27, ossu wrote: > Why is bitrate smoothing put in AudioSendStream? It seems to me either something > only ANA should be concerned with, and so should be done somewhere down in that > implementation, or it's a problem with our bandwidth estimator (producing spikey > estimates), which should be solved at the source. As you noted, there are two options: 1. Perform the smoothing in the BWE because the BWE knows more about how the estimate is produced. It could then provide the listeners with both a smoothed and a non-smoothed version. 2. Do the smoothing it where it's actually needed, in the audio code. Video codecs, at least at the moment, don't care about the smoothed version. We went with #1, but I admit that there are pros and cons with both. > > Why does the setbitrate thing need to be called at regular intervals like this? > Can't it just collect the information as it gets it and aggregate it when > needed, i.e. when it's time to encode a frame. > > https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:49: : send_stream_(std::move(send_stream)) {} > How do you move from a const&? > > https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:421: worker_queue_->PostDelayedTask( > Why isn't this up in AdaptCodecTask and why isn't it making use of the ability > to re-post a task as described in task_queue.h?
https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:270: worker_queue_->PostTask([this]() { AdaptCodec(); }); I removed the conditions here a start the task in Start(); https://codereview.webrtc.org/2546493002/diff/180001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:420: std::unique_ptr<rtc::QueuedTask>(new AdaptCodecTask(weak_ptr_)), On 2016/12/15 14:58:37, the sun wrote: > On 2016/12/13 17:01:26, michaelt wrote: > > 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. > > > > > > 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. Done. https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:49: : send_stream_(std::move(send_stream)) {} I pass rtc::WeakPtr<AudioSendStream> send_stream now by value. To make clear that the class take the ownership of the pointer. https://codereview.webrtc.org/2546493002/diff/260001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:421: worker_queue_->PostDelayedTask( On 2016/12/19 14:09:27, ossu wrote: > Why isn't this up in AdaptCodecTask and why isn't it making use of the ability > to re-post a task as described in task_queue.h? Done.
Calling adapt encode in each encode would be possible as well. But it think we would call the function too often and I kind of dislike to make the call interval dependent on the frame_length of the audio frame.
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15765) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/20437)
On 2016/12/19 14:21:48, stefan-webrtc (holmer) wrote: > As you noted, there are two options: > > 1. Perform the smoothing in the BWE because the BWE knows more about how the > estimate is produced. It could then provide the listeners with both a smoothed > and a non-smoothed version. > > 2. Do the smoothing it where it's actually needed, in the audio code. Video > codecs, at least at the moment, don't care about the smoothed version. > > > We went with #1, but I admit that there are pros and cons with both. I expect you meant #2, since we're now doing smoothing in the audio code. I think that's alright, it's mostly a matter of documentation / expectations. The problem I have with this solution is that it's so spread out: - The smoothing is done in Channel - It is run as a periodic task by AudioSendStream, on a task queue, and it seems to be difficult to get right. - The value is finally used only within the Opus implementation. I'd prefer it if we were able to either move the smoothing completely into AudioDecoderOpus, or keep it up in AudioSendStream and have it communicate the smoothed target bitrate down to the encoder, preferably without having to post asynchronous tasks. On 2016/12/19 14:47:23, michaelt wrote: > Calling adapt encode in each encode would be possible as well. But it think we > would call the function too often and I kind of dislike to make the call > interval dependent on the frame_length of the audio frame. You wouldn't have to adapt for each and every frame. Since you've got a set frame length at each call to Encode(), it should be easy to choose a reasonable multiple to do adaption, say, once every 240 ms. https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:55: if (send_stream_) { I'm not too well versed in the TaskQueue, but I expect as-is, this would just continue to get called every adapt_codec_interval_ms_ even after Stop() is called, right? Perhaps only re-post the task if(send_stream_), and otherwise return true? It still doesn't seem foolproof though, for fast turn-around between Start() and Stop(). https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1349: bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4); Not strictly part of this CL, but I'm interested: what happens when/if the time constant changes? Will the smoothing be temporarily wrong, or will old values be invalidated or will everything be fine? I'm mainly wondering why SetTimeConstantMs is a separate call? It seems to be you'd want it as part of AddSample, i.e. this is the value and it's at a point X ms after the previous sample.
On 2016/12/20 13:58:09, ossu wrote: > On 2016/12/19 14:21:48, stefan-webrtc (holmer) wrote: > > As you noted, there are two options: > > > > 1. Perform the smoothing in the BWE because the BWE knows more about how the > > estimate is produced. It could then provide the listeners with both a smoothed > > and a non-smoothed version. > > > > 2. Do the smoothing it where it's actually needed, in the audio code. Video > > codecs, at least at the moment, don't care about the smoothed version. > > > > > > We went with #1, but I admit that there are pros and cons with both. > > I expect you meant #2, since we're now doing smoothing in the audio code. I Yes :) > think that's alright, it's mostly a matter of documentation / expectations. The > problem I have with this solution is that it's so spread out: > - The smoothing is done in Channel > - It is run as a periodic task by AudioSendStream, on a task queue, and it seems > to be difficult to get right. > - The value is finally used only within the Opus implementation. I agree that it's probably better to keep it close to where it's going to be used. I don't think this is actually going to be given to the opus encoder, but to the audio network adaptation logic, so maybe that's where it should live... Whether it's possible to pass in without an async task I don't know, I'll leave that to someone who knows this code better. > > I'd prefer it if we were able to either move the smoothing completely into > AudioDecoderOpus, or keep it up in AudioSendStream and have it communicate the > smoothed target bitrate down to the encoder, preferably without having to post > asynchronous tasks. > > On 2016/12/19 14:47:23, michaelt wrote: > > Calling adapt encode in each encode would be possible as well. But it think we > > would call the function too often and I kind of dislike to make the call > > interval dependent on the frame_length of the audio frame. > > You wouldn't have to adapt for each and every frame. Since you've got a set > frame length at each call to Encode(), it should be easy to choose a reasonable > multiple to do adaption, say, once every 240 ms. > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:55: if (send_stream_) { > I'm not too well versed in the TaskQueue, but I expect as-is, this would just > continue to get called every adapt_codec_interval_ms_ even after Stop() is > called, right? Perhaps only re-post the task if(send_stream_), and otherwise > return true? It still doesn't seem foolproof though, for fast turn-around > between Start() and Stop(). > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:1349: > bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4); > Not strictly part of this CL, but I'm interested: what happens when/if the time > constant changes? Will the smoothing be temporarily wrong, or will old values be > invalidated or will everything be fine? I'm mainly wondering why > SetTimeConstantMs is a separate call? It seems to be you'd want it as part of > AddSample, i.e. this is the value and it's at a point X ms after the previous > sample.
https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... File webrtc/audio/audio_send_stream.cc (right): https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... webrtc/audio/audio_send_stream.cc:55: if (send_stream_) { You are right. Done I think a fast turn-around between Start() and Stop() should not be a problem since the Start and Stop runs in the same task queue. Which means that the task can not overlap. the send_stream_ pointer will be invalidated in stop. https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1349: bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4); The same filter is used in a different place where we don't adapt the time constant. That is why we set it in a different function. As far that i can see. Everything should be fine on a time constant change. The filter will just adapt faster or slower to a value change.
On 2016/12/20 15:13:04, michaelt wrote: > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... > File webrtc/audio/audio_send_stream.cc (right): > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/audio/audio_send_... > webrtc/audio/audio_send_stream.cc:55: if (send_stream_) { > You are right. > Done > > I think a fast turn-around between Start() and Stop() should not be a problem > since the Start and Stop runs in the same task queue. Which means that the task > can not overlap. the send_stream_ pointer will be invalidated in stop. > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... > File webrtc/voice_engine/channel.cc (right): > > https://codereview.webrtc.org/2546493002/diff/280001/webrtc/voice_engine/chan... > webrtc/voice_engine/channel.cc:1349: > bitrate_smoother_.SetTimeConstantMs(probing_interval_ms * 4); > The same filter is used in a different place where we don't adapt the time > constant. That is why we set it in a different function. > > As far that i can see. Everything should be fine on a time constant change. The > filter will just adapt faster or slower to a value change. Per offline discussion, we decided to seek for another solution. The solution will 1. get rid of task queue. Very big benefit, thanks to Oskar's suggestion. 2. regularly invoke ANA in the Encoder level (AudioEncoderOpus), the trigger will be placed in Encode() method. It is a natural task queue. 3. add probing interval as an argument to OnReceivedTargetBitrate 4. do the BWE smoothing in AudioEncoderOpus. ANA can, in principle, do it too, but it changes the concept, and requires changes that we may later consider reverting back. Michael will upload a new patch soon.
The patch change the cl complete. It should be simpler to understand now.
lgtm with a tiny nit (the May -> Maybe thing). https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:177: virtual void OnReceivedTargetAudioBitrate( Going forward, I think we should try and find a cleaner interface between encoders and the BWE, decoupling encoder implementations from BWE specifics. This is fine for now, though! https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:540: void AudioEncoderOpus::MayUpdateUplinkBandwidth() { nit: May -> Maybe There are several Maybe functions in WebRTC but only one other May function, and it's a getter (i.e. MayDrawIncompleteShapes returns whether or not that is allowed, rather than actually performing an action).
nice work. see some nits https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:177: virtual void OnReceivedTargetAudioBitrate( RTC_DEPRECATED virtual void OnReceivedTargetAudioBitrate(int target_bps); and add a default implementation OnReceivedTargetAudioBitrate(int target_bps) { OnReceivedTargetAudioBitrate(target_bps, rtc::Optional<int64_t>()) } then add virtual void OnReceivedTargetAudioBitrate(int target_bps, rtc::Optional<int64_t> probing_interval_ms); May break internal bots otherwise (not likely though, since it is new) https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:190: new SmoothingFilterImpl(500, config.clock))) { need to explain 500 https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:547: *bitrate_smoother_->GetAverage()); shall Dcheck bitrate_smoother_.GetAverage() is not empty https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:171: rtc::Optional<int64_t> last_smoothed_bandwith_update_; bitrate_smoother_last_update_time_ https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:451: TEST(AudioEncoderOpusTest, UpdateUplinkBandwithInAudioNetworkAdaptor) { Bandwidth https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:464: // Don't update till its time to update again. its -> it is https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:470: // Update when its time to update. its -> it is https://codereview.webrtc.org/2546493002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1331: if (*encoder) { ... }
https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:540: void AudioEncoderOpus::MayUpdateUplinkBandwidth() { On 2016/12/22 14:01:15, ossu wrote: > nit: May -> Maybe > > There are several Maybe functions in WebRTC but only one other May function, and > it's a getter (i.e. MayDrawIncompleteShapes returns whether or not that is > allowed, rather than actually performing an action). Done.
https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:177: virtual void OnReceivedTargetAudioBitrate( Do you think this is necessary, wouldn't it be enough to run internal server ? https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:547: *bitrate_smoother_->GetAverage()); Added a if since this could relay happens. https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:171: rtc::Optional<int64_t> last_smoothed_bandwith_update_; On 2016/12/22 14:51:26, minyue-webrtc wrote: > bitrate_smoother_last_update_time_ Done. https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:451: TEST(AudioEncoderOpusTest, UpdateUplinkBandwithInAudioNetworkAdaptor) { On 2016/12/22 14:51:27, minyue-webrtc wrote: > Bandwidth Done. https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:464: // Don't update till its time to update again. On 2016/12/22 14:51:27, minyue-webrtc wrote: > its -> it is Done. https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus_unittest.cc:470: // Update when its time to update. On 2016/12/22 14:51:27, minyue-webrtc wrote: > its -> it is Done. https://codereview.webrtc.org/2546493002/diff/320001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1331: if (*encoder) On 2016/12/22 14:51:27, minyue-webrtc wrote: > { > ... > } Done.
https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:177: virtual void OnReceivedTargetAudioBitrate( On 2016/12/22 15:10:10, michaelt wrote: > Do you think this is necessary, wouldn't it be enough to run internal server ? No hurt to be cautious. then we avoid risk of revert.
https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/320001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:177: virtual void OnReceivedTargetAudioBitrate( On 2016/12/22 15:12:02, minyue-webrtc wrote: > On 2016/12/22 15:10:10, michaelt wrote: > > Do you think this is necessary, wouldn't it be enough to run internal server ? > > No hurt to be cautious. then we avoid risk of revert. Done.
suggest rewording a comment. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:179: // Provides target audio bitrate to this encoder to allow it to adapt and the // Provides target audio bitrate and corresponding probing interval of the bandwidth estimator to this encoder to allow it to adapt.
The CQ bit was checked by minyue@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/13714) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/13722) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16066) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/15922) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/21296) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/20037)
Hi Michael, I cannot help you landing it since owner of the CL (you) can make changes. Please rebase and consider my final comments (in last two threads). You get my LGTM anyway. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (left): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:171: virtual void OnReceivedUplinkBandwidth(int uplink_bandwidth_bps); try not to deprecate it now. since it is not a necessary change in this CL. but of course, remove the override in AudioEncoderOpus as you did. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:190: new SmoothingFilterImpl(500, config.clock))) { add a comment on the choice of 500 https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:67: int update_uplink_bandwidth_interval_ms = 200; uplink_bandwidth_update_interval_ms
https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (left): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:171: virtual void OnReceivedUplinkBandwidth(int uplink_bandwidth_bps); On 2016/12/28 08:46:07, minyue-webrtc wrote: > try not to deprecate it now. since it is not a necessary change in this CL. but > of course, remove the override in AudioEncoderOpus as you did. Done. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/audio_encoder.h (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/audio_encoder.h:179: // Provides target audio bitrate to this encoder to allow it to adapt and the On 2016/12/27 14:48:58, minyue-webrtc wrote: > // Provides target audio bitrate and corresponding probing interval of the > bandwidth estimator to this encoder to allow it to adapt. Done. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:190: new SmoothingFilterImpl(500, config.clock))) { On 2016/12/28 08:46:07, minyue-webrtc(OOOtoJan16) wrote: > add a comment on the choice of 500 Done. https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/380001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:67: int update_uplink_bandwidth_interval_ms = 200; On 2016/12/28 08:46:07, minyue-webrtc wrote: > uplink_bandwidth_update_interval_ms Done.
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16253) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16109)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/16488)
lgtm % comment https://codereview.webrtc.org/2546493002/diff/440001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/440001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:66: const Clock* clock = Clock::GetRealTimeClock(); The Clock:: API is being deprecated - use those in base/timeutils.h instead.
https://codereview.webrtc.org/2546493002/diff/440001/webrtc/modules/audio_cod... File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h (right): https://codereview.webrtc.org/2546493002/diff/440001/webrtc/modules/audio_cod... webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.h:66: const Clock* clock = Clock::GetRealTimeClock(); I changed the direct in this CL used time function to the new API. Will add a followup CL which will change smoothing filter and other dependencies as well.
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21537)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/17612)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
henrik.lundin@webrtc.org changed reviewers: + nisse@webrtc.org
nisse@, please take a look at the deps problems/solutions that Michael ran into. You can probably look at the diff between PS20 and PS24. In PS20, solenberg@ urges Michael not to use Clock::. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
On 2017/01/10 09:18:08, hlundin-webrtc wrote: > nisse@, please take a look at the deps problems/solutions that Michael ran into. > You can probably look at the diff between PS20 and PS24. In PS20, solenberg@ > urges Michael not to use Clock::. > > Thanks! The changes in webrtc/modules/BUILD.gn will make modules_unittests to use a completely different main() for running the test, which probably explains why it fails on Android. Is there a way to avoid that change? We really should try to get rid of webrtc/base/unittest_main.cc entirely, but that's of course out of scope of this CL. There's actually a bug tracking that work: https://bugs.chromium.org/p/webrtc/issues/detail?id=5996
I could create a build target for fakeclock in base. An link modules unittest to this target. It would solve my dependency problems. But I'm not sure if this is a good solution.
On 2017/01/10 12:36:45, michaelt wrote: > I could create a build target for fakeclock in base. An link modules unittest to > this target. It would solve my dependency problems. > But I'm not sure if this is a good solution. If we think base/unittest_main.cc should be eliminated, it would make sense to me to move it out of the rtc_base_tests_utils into a new target, e.g., rtc_base_tests_main. My thinking with having fakeclock.cc, natserver.cc, etc, in rtc_base_tests_utils was to make that code available to tests of code outside of base, and then having a main function in the same target is a bad idea.
Ok then i will create a target "rtc_base_tests_main" and move "base/unittest_main.cc" to it. Sounds ok to me.
On 2017/01/10 13:25:58, michaelt wrote: > Ok then i will create a target "rtc_base_tests_main" and move > "base/unittest_main.cc" to it. > Sounds ok to me. Done
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/16323) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16176) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/21569)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/9724) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...) win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
Patchset #28 (id:600001) has been deleted
Patchset #27 (id:580001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Patchset #27 (id:620001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Patchset #27 (id:640001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/20001)
Patchset #27 (id:660001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Patchset #27 (id:680001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
Patchset #27 (id:700001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
Patchset #27 (id:720001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
Patchset #27 (id:740001) has been deleted
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by michaelt@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by michaelt@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, ossu@webrtc.org, minyue@webrtc.org, solenberg@webrtc.org Link to the patchset: https://codereview.webrtc.org/2546493002/#ps770001 (title: "Renamed OnReceivedTargetAudioBitrate to OnReceivedUplinkBandwidth")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 770001, "attempt_start_ts": 1484244894106790,
"parent_rev": "891419f8e8a1b0fa2d53c887a033693652ff7d28", "commit_rev":
"566d820e004700df9a152602cbda44fac839d7e7"}
Message was sent while issue was closed.
Description was changed from ========== Update smoothed bitrate. BUG=webrtc:6443 ========== to ========== Update smoothed bitrate. BUG=webrtc:6443 Review-Url: https://codereview.webrtc.org/2546493002 Cr-Commit-Position: refs/heads/master@{#16036} Committed: https://chromium.googlesource.com/external/webrtc/+/566d820e004700df9a152602c... ==========
Message was sent while issue was closed.
Committed patchset #28 (id:770001) as https://chromium.googlesource.com/external/webrtc/+/566d820e004700df9a152602c... |
