Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2665693002:
Moves channel-dependent audio input processing to separate encoder task queue (Closed)
|
Created:
3 years, 10 months ago by henrika_webrtc Modified:
3 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tommi Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionMoves channel-dependent audio input processing to separate encoder task queue.
First approach to remove parts of the heavy load done for encoding, and
preparation for sending, from native audio thread to separate task queue.
With this change we will give the native input audio thread more time to
"relax" between successive audio captures.
Separate profiling done on Android has verified that the change works well;
the load is now redistributed and the load of the native AudioRecordThread
is reduced. Similar conclusions should be valid for all other OS:es as well.
BUG=NONE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng
Review-Url: https://codereview.webrtc.org/2665693002
Cr-Commit-Position: refs/heads/master@{#17488}
Committed: https://chromium.googlesource.com/external/webrtc/+/ec6fbd277623cc60007d6a57d91f1e6b78ad6b04
Patch Set 1 #
Total comments: 1
Patch Set 2 : BUILD changes #
Total comments: 7
Patch Set 3 : rebased #Patch Set 4 : Rebased #Patch Set 5 : Added time measurement (to be removed) #Patch Set 6 : rebased #Patch Set 7 : Added audio frame pool #Patch Set 8 : Now works with pool of audio frames #Patch Set 9 : Renamed APIs in audio frame pool #Patch Set 10 : nit #Patch Set 11 : Modified SwapQueue #Patch Set 12 : nit #Patch Set 13 : cleanup #
Total comments: 4
Patch Set 14 : Added PostTask helper #Patch Set 15 : Fixed critical section #Patch Set 16 : Fix for Mac #Patch Set 17 : Increased prio of queue #
Total comments: 10
Patch Set 18 : Removed frame pool #Patch Set 19 : reverted swap queue changes #Patch Set 20 : Moved resampling to queue #Patch Set 21 : nit #Patch Set 22 : rebased #Patch Set 23 : Improves life-time handling of channel object #Patch Set 24 : nit #
Total comments: 6
Patch Set 25 : Removed logging #Patch Set 26 : Final changes #Patch Set 27 : rebased #Patch Set 28 : Fixed dependency #
Total comments: 60
Patch Set 29 : Changes based on feedback from Fredrik and Tommi - part 1(2) #Patch Set 30 : rebased #Patch Set 31 : Changes based on feedback from Fredrik and Tommi - part 2(2) #Patch Set 32 : StopSend is now void #
Total comments: 10
Patch Set 33 : Final feedback from Fredrik #
Total comments: 5
Patch Set 34 : Removed debug logs in ADB #
Total comments: 14
Patch Set 35 : Final comments from Tommi #Messages
Total messages: 129 (85 generated)
Description was changed from ========== Moves channel-dependent audio input processing to separate encoder task queue Cleaned up SharedData now owns the task queue Experimental refactoring of audio encoding in WebRTC Merged methods to prepare for task queue BUG=NONE patch from issue 2652213002 at patchset 60001 (http://crrev.com/2652213002#ps60001) ========== to ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE ==========
henrika@webrtc.org changed reviewers: + solenberg@webrtc.org
As discussed. PTAL. https://codereview.webrtc.org/2665693002/diff/1/webrtc/base/physicalsocketser... File webrtc/base/physicalsocketserver.cc (right): https://codereview.webrtc.org/2665693002/diff/1/webrtc/base/physicalsocketser... webrtc/base/physicalsocketserver.cc:718: // LOG_ERR(LS_WARNING) << "Assuming benign blocking error"; Mu own local change because I hate this spammy error message.
The CQ bit was checked by henrika@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...) 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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/16782) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/22210) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/20945)
Description was changed from ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE ========== to ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE CQ_INCLUDE_TRYBOTS=tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ==========
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by henrika@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: Your CL was about to rely on recently removed CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is no longer supported:
tryserver.chromium.android
For more details, see http://crbug.com/617627.
Description was changed from ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE CQ_INCLUDE_TRYBOTS=tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ========== to ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ==========
The CQ bit was checked by henrika@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/...
https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:404: Channel* const channel_; We have us an ownership issue, I think: what if the Channel is deleted while tasks are pending? https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2787: std::unique_ptr<AudioFrame> audio_source(new AudioFrame()); We don't want to introduce more copying if we can avoid it. At least we should avoid heap allocs. Would it be possible to create a pool of pre-alloced ProcessAndEncodeAudioTasks which can be reused (and own their AudioFrames)? I guess it could be a growing pool with some max size? I guess not, since the TaskQueue interface takes unique_ptrs... :/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/19286)
henrika@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2787: std::unique_ptr<AudioFrame> audio_source(new AudioFrame()); On 2017/01/31 13:34:33, the sun wrote: > We don't want to introduce more copying if we can avoid it. At least we should > avoid heap allocs. > > Would it be possible to create a pool of pre-alloced ProcessAndEncodeAudioTasks > which can be reused (and own their AudioFrames)? I guess it could be a growing > pool with some max size? I guess not, since the TaskQueue interface takes > unique_ptrs... :/ That can still be done I think. After being done with a buffer, ownership can be passed back to a pool. The task objects themselves can also be reused and from that perspective, they could also own the buffers internally. Allocating a task object could btw be done at the same time the AudioFrame is allocated (fewer and less frequent heap allocs). https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2791: new ProcessAndEncodeAudioTask(std::move(audio_source), this))); btw, in order to use move semantics, you don't have to allocate on the heap if the class itself is movable.
Thanks for the comments. No low-hanging fruit but will try to provide changes. https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:404: Channel* const channel_; Will see if I can improve. https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2787: std::unique_ptr<AudioFrame> audio_source(new AudioFrame()); I might have to ask you guys off-line on how what the best approach is. https://codereview.webrtc.org/2665693002/diff/40001/webrtc/voice_engine/chann... webrtc/voice_engine/channel.cc:2791: new ProcessAndEncodeAudioTask(std::move(audio_source), this))); On 2017/01/31 14:51:18, tommi (webrtc) wrote: > btw, in order to use move semantics, you don't have to allocate on the heap if > the class itself is movable. Acknowledged.
FYI, I have paused working on this CL. Will discuss with Fredrik how/if we shall continue or not.
henrika@webrtc.org changed reviewers: + aleloi@webrtc.org
Hi again, seems like the priority of this work has bumped up. Please revisit and provide comments. The latest version: - moves encoder queue into shared objects and is now shared between all channels - adds usage of a pool of audio frames to avoid dynamic resource allocation - has been tested for long periods of time on several Android devices - has been profiled to confirm gain by moving work to separate encoding thread PTAL
The CQ bit was checked by henrika@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 henrika@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/20082)
The CQ bit was checked by henrika@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...)
Sorry but I am actually not sure yet that a pool of audio frames is worth the extra code. Initial profiling results shows an increased complexity using the pool.
nits https://codereview.webrtc.org/2665693002/diff/340001/webrtc/base/swap_queue_u... File webrtc/base/swap_queue_unittest.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/base/swap_queue_u... webrtc/base/swap_queue_unittest.cc:56: EXPECT_EQ(queue.Size(), 2u); nit: check capacity again here (guard against regressions, document behavior) https://codereview.webrtc.org/2665693002/diff/340001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:478: LOG(INFO) << "avg_diff_time: " << avg_diff_time; log max here as well, to avoid it in audio cb
The CQ bit was checked by henrika@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/...
Task queue lifetime comments https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:440: }; Since the task operates on a pool and channel pointer, these objects must not be destroyed before the task is run. Tasks can potentially be alive for as long as the task queue is not destroyed. I know only of these ways to accomplish that: 1. Make the task queue be destroyed before the channel and pool (but after the last attempt to post anything on it) 2. Use reference counting to delay the destruction of channel and pool (maybe infeasible if Channel itself has to be refcounted) 3. Delay destroying the channel until the last task has finished running with some kind of messaging between task and channel d-tor. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2648: RemixAndResample(audio_data, number_of_frames, number_of_channels, Perhaps resampling could also be done on the task queue? https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.h (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.h:73: rtc::TaskQueue encoder_queue_; The queue needs to be alive when methods in Channel try to access it. I think this means it should outlive ChannelManager. Everything's probably fine when it passes the tsan tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/21121)
Thanks for feedback. Note that frame pool is now removed. Will upload new reference. Might not resolve all issues. Stay tuned. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/base/swap_queue_u... File webrtc/base/swap_queue_unittest.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/base/swap_queue_u... webrtc/base/swap_queue_unittest.cc:56: EXPECT_EQ(queue.Size(), 2u); These changes are now removed. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:478: LOG(INFO) << "avg_diff_time: " << avg_diff_time; On 2017/03/23 12:45:55, the sun wrote: > log max here as well, to avoid it in audio cb Done. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:440: }; Thanks! The pool is now removed. Will take your comments into account and discuss with Fredrik as well. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2648: RemixAndResample(audio_data, number_of_frames, number_of_channels, Goof point. Will have to create unique method for it. https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.h (right): https://codereview.webrtc.org/2665693002/diff/340001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.h:73: rtc::TaskQueue encoder_queue_; All channels are deleted before shared data (and the queue). Hence, the issue is that outstanding tasks might try to access invalid channel instances.
henrika@webrtc.org changed reviewers: - tommi@webrtc.org
Now moved resampling to queue as well and also verified that the "PushCaptureData path" works.
What remains now is to ensure that any task does not try to access and invalid channel object.
The CQ bit was checked by henrika@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/20118)
I have now uploaded a new version where I try to ensure that the task queue never attempts to access an invalid channel object. I used a similar design as in ViEEncoder. Please check it out. There are also some extra log statements but I will remove them before landing. PTAL
The CQ bit was checked by henrika@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/20148)
The CQ bit was checked by henrika@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.
...and we are green ;-)
Looks good! A few comments more. https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1118: This is a cleanup of dead code, right? https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:429: RemixAndResample(audio_data, number_of_frames, number_of_channels, Can resampling be done on the queue? E.g. copy the data to the frame, save the parameters, set a flag saying resampling should be done, and do this at the start of the Run() method? https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:417: class ProcessAndEncodeAudioTask; This declaration is only letting the task DCHECK that it runs on the private encoder_queue_? I wonder if it's necessary to declare it here.
Thanks Alex. Just added some comments. https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (left): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1118: Honestly don't know how it ended up in this CL. Might be to a rebase. Will double check. https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:429: RemixAndResample(audio_data, number_of_frames, number_of_channels, My bad. That was actually my intention. I will rewrite https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/480001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:417: class ProcessAndEncodeAudioTask; Sorry but I don't understand. What do you suggest I do instead?
Good stuff! Have you measured if there is any change in latency (presumably an increase) with the task queue? I'm thinking the time from start of RecordedDataIsAvailable()/PushCaptureData() to start of Channel::SendData(), or something like that. Would be interesting to know.
I have done some extended measurements on my Pixel device. I measure the time from AudioDeviceBuffer::DeliverRecordedData() -> Channel::SendData() in both cases. Did two tests on each and the results are stable. 1) Without patch => average time ~1.78 msec. (no thread jump) 2) With patch => average time ~2.38 msec. (one thread jump) So yes, an expected increase by approx 0.5 msec in this case. But the extended headroom on the audio thread seems to be worth it imho.
PTAL My plan was to remove some logs once OK from both reviewers.
Forgot, I should also move resampling to the queue. Stay tuned.
Discussed with Alex off-line. Costs one extra copy to do resampling on the queue. Will move it back instead.
Sorry for all the rounds. Moved audio frame out from queue task. Makes code for with and without resampling more alike and there is no gain in letting the task own its frame (can't measure any gain). PTAL PS I will check the CL before landing and will remove all debug logging. Keeping them in case I get more questions about performance.
The CQ bit was checked by henrika@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_ios10_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...) ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by henrika@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...) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/24249) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/23353) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24260)
The CQ bit was checked by henrika@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.
https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { Given the simplicity, could you even use a lambda for this? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2720: uint32_t Channel::PrepareEncodeAndSend(AudioFrame* audio_input) { Could you fuse PrepareEncodeAndSend with EncodeAndSend into ProcessAndEncodeAudioOnTaskQueue? The names don't really provide me any information about what goes on anyway. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2746: uint32_t Channel::EncodeAndSend(AudioFrame* audio_input) { Remove return value - it is unused. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2749: RTC_DCHECK(audio_input->samples_per_channel_); _GT(..., 0); https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2751: audio_input->id_ = _channelId; Already did that in Channel::ProcessAndEncodeAudio()? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2765: _timeStamp += static_cast<uint32_t>(audio_input->samples_per_channel_); Can you use ACCESS_ON() in the .h, when _timeStamp is declared? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2888: if (input_file_player_->Get10msAudioFromFile(fileBuffer.get(), &fileSamples, We must still hold _fileCritSect while making this call. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2901: assert(audio_input->samples_per_channel_ == fileSamples); RTC_DCHECK https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:360: PushResampler<int16_t>* input_resampler() { return &input_resampler_; } Not needed anymore? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:389: uint32_t PrepareEncodeAndSend(AudioFrame* audio_input); Fuse them into ProcessAndEncodeAudioOnTaskQueue() https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transmit_mixer.cc:314: void TransmitMixer::ProcessAudio() { Name it ProcessAndEncodeAudio() since that's the name used elsewhere.
tommi@webrtc.org changed reviewers: + tommi@webrtc.org
I just realized I had to stop reviewing a while ago. These comments may be completely out of date but sending them just in case. Will take another pass at the latest. https://codereview.webrtc.org/2665693002/diff/260001/webrtc/base/swap_queue.h File webrtc/base/swap_queue.h (right): https://codereview.webrtc.org/2665693002/diff/260001/webrtc/base/swap_queue.h... webrtc/base/swap_queue.h:111: return num_elements_; returning a value that requires a lock? (the returned value can't be assumed to be correct, since a lock isn't held when examined). Can we instead require that the calling thread is the same thread as the value can change on? https://codereview.webrtc.org/2665693002/diff/260001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.cc (right): https://codereview.webrtc.org/2665693002/diff/260001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.cc:325: int64_t start_time = rtc::TimeMicros(); debug only? https://codereview.webrtc.org/2665693002/diff/260001/webrtc/modules/audio_dev... File webrtc/modules/audio_device/audio_device_buffer.h (right): https://codereview.webrtc.org/2665693002/diff/260001/webrtc/modules/audio_dev... webrtc/modules/audio_device/audio_device_buffer.h:255: // TODO(henrika): to be removed... (only used for tesing of this CL)! tesing? what is tesing?!?!? I'm so confused https://codereview.webrtc.org/2665693002/diff/260001/webrtc/voice_engine/audi... File webrtc/voice_engine/audio_frame_pool.h (right): https://codereview.webrtc.org/2665693002/diff/260001/webrtc/voice_engine/audi... webrtc/voice_engine/audio_frame_pool.h:36: size_t size() const { return audio_frame_queue_.Size(); } thread check?
Tommi, sorry for all the rounds but the swap queue is no longer used ;-) And the changes in audio_device_buffer will all be removed before landing. They are just added to print out time consumption in the audio callback. I have a similar patch for the reference case (without task queue) to be able to compare.
...hence, no need to review audio_device_buffer.h/.cc
A few more comments https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:465: _lastLocalTimeStamp = timeStamp; Appears unused - remove. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:466: _lastPayloadType = payloadType; Appears unused - remove. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2729: AudioFrameOperations::Mute(audio_input, previous_frame_muted_, is_muted); Declare ACCESS_ON for previous_frame_muted_ https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2731: if (_includeAudioLevelIndication) { _includeAudioLevelIndication is now potentially racy. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2736: rms_level_.AnalyzeMuted(length); Declare ACCESS_ON for rms_level_
https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { On 2017/03/28 12:57:50, the sun wrote: > Given the simplicity, could you even use a lambda for this? fyi - there's one difference between this class and a lambda and that is that lambda's don't support passing ownership (in C++ 11). What that means is that we'd need to pass the audio frame as a raw pointer to the lambda and if the lambda never runs, the frame would be leaked. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:965: if (channel_state_.Get().sending_has_been_activated) { calling Get() here grabs a lock and could actually be introducing synchronization which I'm not sure you want (especially given that it's not inside the DCHECK). Is there another way to dcheck that StopSend() has been called? Ideally one that doesn't require synchronization. Actually... I see that StopSend() is called inside Terminate(). I think that this check could actually be removed and the DCHECK for stop_send_event_.Wait(0) could be moved to Terminate(). https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1265: RTC_DCHECK_RUN_ON(encoder_queue_); nit: I don't think this is necessary. hmm... I don't see stop_send_event_ used outside of this function actually. Do you need it as a member variable? What about something like this: rtc::Event flush(false, false); encoder_queue_->PostTask([&flush](){ flush.Set(); }); flush.Wait(rtc::Event::kForever); This is furthermore an opportunity to remove the SetSending() method, or at least require that it be only set/checked on the encoder queue, and thus be able to remove the lock requirement for that flag. rtc::Event flush(false, false); encoder_queue_->PostTask([&flush, &channel_state_](){ channel_state_.SetSending(false); flush.Set(); }); flush.Wait(rtc::Event::kForever); Then change ChannelState so that it DCHECKs that this flag is only ever used on the same task queue and remove the lock. As a further step, the ownership of all the state that needs to be torn down here, could be moved into the task so that we won't have to block here. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1275: // https://code.google.com/p/webrtc/issues/detail?id=2111 . fredrik - looks like this might be ok to delete now? (as per https://bugs.chromium.org/p/webrtc/issues/detail?id=2102) https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2689: RTC_DCHECK(encoder_queue_); did you intend to dcheck that you're running on the encoder queue? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2701: RTC_DCHECK(encoder_queue_); same here... might want to do a search/replace for all RTC_DCHECK(encoder_queue_) and replace with RTC_DCHECK_RUN_ON(encoder_queue_) https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2751: audio_input->id_ = _channelId; On 2017/03/28 12:57:50, the sun wrote: > Already did that in Channel::ProcessAndEncodeAudio()? change to a dcheck_eq? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2888: if (input_file_player_->Get10msAudioFromFile(fileBuffer.get(), &fileSamples, On 2017/03/28 12:57:50, the sun wrote: > We must still hold _fileCritSect while making this call. Alternatively, we could change input_file_player_ so that it's always ACCESS_ON() the encoder queue. wdyt? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:84: class ChannelState { I don't suppose you see a way to make this class single threaded down the line? The pattern that's used is something I'd like to move away from. There's the problem of return a value that requires a lock to access but there are also multiple methods for changing and getting different state of the class, so how the class is used (5 *set* methods), does not guarantee a consistent state. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:91: bool sending_has_been_activated = false; what's the difference between this variable and sending_? Seems to be two states for the same thing? It would be good to add some documentation that explains how it's supposed to work. Also, if e.g. sending_ == false, sending_has_been_activated_ = true, should not be possible, then perhaps two bools is not what you want but rather a state variable. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:385: void ProcessAndEncodeAudioOnTaskQueue(AudioFrame* audio_input); Will the frame be modified by this function? If not, should it be passed via const&? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:33: encoder_queue_("AudioEncoderQueue", rtc::TaskQueue::Priority::HIGH) { why high priority? This doesn't work btw on Android or in Chrome. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.h (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.h:50: rtc::TaskQueue* encoder_queue() { return &encoder_queue_; } since we're adding this accessor, can we DCHECK that this accessor is always called on a known thread? Ideally the construction thread.
FYI, uploaded parts of proposed changes 1(2). Will continue tomorrow. The changes are rather extensive. Will try to cover all of them. Will ping when done ;-)
The CQ bit was checked by henrika@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/...
Launched some try bots over night to check changes done so far.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/11046) presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/15483)
The CQ bit was checked by henrika@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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/23504)
https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { On 2017/03/28 13:47:20, tommi (webrtc) wrote: > On 2017/03/28 12:57:50, the sun wrote: > > Given the simplicity, could you even use a lambda for this? > > fyi - there's one difference between this class and a lambda and that is that > lambda's don't support passing ownership (in C++ 11). What that means is that > we'd need to pass the audio frame as a raw pointer to the lambda and if the > lambda never runs, the frame would be leaked. Ah, good point. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1275: // https://code.google.com/p/webrtc/issues/detail?id=2111 . On 2017/03/28 13:47:19, tommi (webrtc) wrote: > fredrik - looks like this might be ok to delete now? > (as per https://bugs.chromium.org/p/webrtc/issues/detail?id=2102) It might - outside the scope of this CL though. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2888: if (input_file_player_->Get10msAudioFromFile(fileBuffer.get(), &fileSamples, On 2017/03/28 13:47:19, tommi (webrtc) wrote: > On 2017/03/28 12:57:50, the sun wrote: > > We must still hold _fileCritSect while making this call. > > Alternatively, we could change input_file_player_ so that it's always > ACCESS_ON() the encoder queue. wdyt? This code is being stripped away with the VoEFile interface, so I think we shouldn't jump through hoops to improve it. If you can get away with removing it right now, that's one option, but I doubt you can because of voe_auto_test. The most pragmatic approach appears to me keeping the lock as it was and wait for the reaper.
Thanks guys, done my best to comply with as many change proposals as possible. Some felt to "big" to do here. Still one major comment from Tommi remaining but I would like to make a pit stop first and run some try bots. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { Given input from Tommi I would like to skip using lambda. OK? https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { On 2017/03/28 13:47:20, tommi (webrtc) wrote: > On 2017/03/28 12:57:50, the sun wrote: > > Given the simplicity, could you even use a lambda for this? > > fyi - there's one difference between this class and a lambda and that is that > lambda's don't support passing ownership (in C++ 11). What that means is that > we'd need to pass the audio frame as a raw pointer to the lambda and if the > lambda never runs, the frame would be leaked. Acknowledged. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:420: bool Run() override { On 2017/03/28 23:05:40, the sun wrote: > On 2017/03/28 13:47:20, tommi (webrtc) wrote: > > On 2017/03/28 12:57:50, the sun wrote: > > > Given the simplicity, could you even use a lambda for this? > > > > fyi - there's one difference between this class and a lambda and that is that > > lambda's don't support passing ownership (in C++ 11). What that means is that > > we'd need to pass the audio frame as a raw pointer to the lambda and if the > > lambda never runs, the frame would be leaked. > > Ah, good point. Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:465: _lastLocalTimeStamp = timeStamp; Nice. Thanks! https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:466: _lastPayloadType = payloadType; On 2017/03/28 13:28:29, the sun wrote: > Appears unused - remove. Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:965: if (channel_state_.Get().sending_has_been_activated) { Did changes. Please check again. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1265: RTC_DCHECK_RUN_ON(encoder_queue_); Please let me wait with this larger task until the next round. Will try to fix all other parts first. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1275: // https://code.google.com/p/webrtc/issues/detail?id=2111 . Will wait for verification from Fredrik before making any changes. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1275: // https://code.google.com/p/webrtc/issues/detail?id=2111 . No action from my side in this CL. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2689: RTC_DCHECK(encoder_queue_); Yes, since it is not set at ctor. See comments below. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2701: RTC_DCHECK(encoder_queue_); Actually, looking at the TSAN issues I realize that the changes might be too large to motivate changes in this CL. Hope it is acceptable if the encoder thread pointer is injected via SetEngineInformation() as is the case for other members. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2720: uint32_t Channel::PrepareEncodeAndSend(AudioFrame* audio_input) { On 2017/03/28 12:57:50, the sun wrote: > Could you fuse PrepareEncodeAndSend with EncodeAndSend into > ProcessAndEncodeAudioOnTaskQueue? The names don't really provide me any > information about what goes on anyway. Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2729: AudioFrameOperations::Mute(audio_input, previous_frame_muted_, is_muted); On 2017/03/28 13:28:29, the sun wrote: > Declare ACCESS_ON for previous_frame_muted_ Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2731: if (_includeAudioLevelIndication) { Yes, discussed offline. No action here. Adding todo. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2736: rms_level_.AnalyzeMuted(length); On 2017/03/28 13:28:29, the sun wrote: > Declare ACCESS_ON for rms_level_ Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2746: uint32_t Channel::EncodeAndSend(AudioFrame* audio_input) { This method is now removed. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2749: RTC_DCHECK(audio_input->samples_per_channel_); On 2017/03/28 12:57:50, the sun wrote: > _GT(..., 0); Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2751: audio_input->id_ = _channelId; On 2017/03/28 12:57:50, the sun wrote: > Already did that in Channel::ProcessAndEncodeAudio()? Acknowledged. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2751: audio_input->id_ = _channelId; On 2017/03/28 13:47:20, tommi (webrtc) wrote: > On 2017/03/28 12:57:50, the sun wrote: > > Already did that in Channel::ProcessAndEncodeAudio()? > > change to a dcheck_eq? Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2765: _timeStamp += static_cast<uint32_t>(audio_input->samples_per_channel_); Should work. Yes. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2888: if (input_file_player_->Get10msAudioFromFile(fileBuffer.get(), &fileSamples, Added _fileCritSect, Lot's of changes required to remove this support. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2901: assert(audio_input->samples_per_channel_ == fileSamples); On 2017/03/28 12:57:50, the sun wrote: > RTC_DCHECK Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:360: PushResampler<int16_t>* input_resampler() { return &input_resampler_; } On 2017/03/28 12:57:50, the sun wrote: > Not needed anymore? Acknowledged. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:389: uint32_t PrepareEncodeAndSend(AudioFrame* audio_input); Great idea ;-) https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.cc:33: encoder_queue_("AudioEncoderQueue", rtc::TaskQueue::Priority::HIGH) { Encoding is important and it sounded like a suitable feature I could use. Let me answer with a question: when is is suitable to use? I will remove usage in this CL. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... File webrtc/voice_engine/shared_data.h (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/shar... webrtc/voice_engine/shared_data.h:50: rtc::TaskQueue* encoder_queue() { return &encoder_queue_; } On 2017/03/28 13:47:20, tommi (webrtc) wrote: > since we're adding this accessor, can we DCHECK that this accessor is always > called on a known thread? Ideally the construction thread. Done. https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/tran... File webrtc/voice_engine/transmit_mixer.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/tran... webrtc/voice_engine/transmit_mixer.cc:314: void TransmitMixer::ProcessAudio() { On 2017/03/28 12:57:50, the sun wrote: > Name it ProcessAndEncodeAudio() since that's the name used elsewhere. Done.
The CQ bit was checked by henrika@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.
Asking for new round even if I have not implemented all proposed changes. See motivations in review comments. PTAL https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/560001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1265: RTC_DCHECK_RUN_ON(encoder_queue_); I actually failed to do what was suggested since I could not come up with a clean solution to replace if (!channel_state_.Get().sending) { return 0; }
The CQ bit was checked by henrika@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_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/24356)
lgtm & comments https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:418: : audio_frame_(std::move(audio_frame)), channel_(channel) {} super nit: you should DCHECK(channel) here instead of before the dereference; it is part of the invariant. https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2674: RTC_DCHECK(encoder_queue_); Also RTC_DCHECK(channel_state_.Get().sending); https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2686: RTC_DCHECK(encoder_queue_); Here too: RTC_DCHECK(channel_state_.Get().sending); https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2852: rtc::CritScope cs(&_fileCritSect); fyi: you're holding the lock for longer now. don't know if it matters https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:381: void ProcessAndEncodeAudioOnTaskQueue(AudioFrame* audio_input); Move to private:
Done. Tommi: would you mind one last check ;-) https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:418: : audio_frame_(std::move(audio_frame)), channel_(channel) {} On 2017/03/30 10:13:32, the sun wrote: > super nit: you should DCHECK(channel) here instead of before the dereference; it > is part of the invariant. Done. https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2674: RTC_DCHECK(encoder_queue_); On 2017/03/30 10:13:33, the sun wrote: > Also > RTC_DCHECK(channel_state_.Get().sending); Done. https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2686: RTC_DCHECK(encoder_queue_); On 2017/03/30 10:13:33, the sun wrote: > Here too: > RTC_DCHECK(channel_state_.Get().sending); Done. https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2852: rtc::CritScope cs(&_fileCritSect); Let me fix that. https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.h (right): https://codereview.webrtc.org/2665693002/diff/640001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.h:381: void ProcessAndEncodeAudioOnTaskQueue(AudioFrame* audio_input); On 2017/03/30 10:13:33, the sun wrote: > Move to private: Done.
The CQ bit was checked by henrika@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.
https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1146: RTC_DCHECK(encoder_queue); should we also add: RTC_DCHECK(!encoder_queue_); (note: that's making sure the member variable is null) https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1245: channel_state_.SetSending(false); It's too bad that these two methods, Get() and SetSending() both grab the same lock, copy state, only for the sake of setting a bool and even so, they're prone to a race since when we call SetSending(false), we don't know if |sending| is still true[1], which indicates a bug. The four lock/unlock operations, checking the value and setting it, could all be accomplished in a single atomic operation: if (rtc::AtomicOps::CompareAndSwap(&channel_state_.sending_, 0) == 0) return; RTC_DCHECK(encoder_queue_); ... [1]: That is assuming that the lock is actually needed on this thread, in the first place. It could very well be that all modification to the |sending| flag, is done on the current thread, and if so, the current thread does not need to hold a lock to examine its value. However, there aren't thread checks or documentation that explain and verify how this works, so at the moment, it doesn't look like we really know. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2675: RTC_DCHECK(encoder_queue_); we don't DCHECK pointers that we dereference anyway https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2678: audio_frame->CopyFrom(audio_input); It feels like we should be able to avoid this. Can you add a TODO here so that we don't have to do this allocation+copy for every buffer? https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2688: RTC_DCHECK(encoder_queue_); this pointer is used anyway below, so no need to dcheck
https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1245: channel_state_.SetSending(false); On 2017/03/31 09:44:10, tommi (webrtc) wrote: > It's too bad that these two methods, Get() and SetSending() both grab the same > lock, copy state, only for the sake of setting a bool and even so, they're prone > to a race since when we call SetSending(false), we don't know if |sending| is > still true[1], which indicates a bug. > > The four lock/unlock operations, checking the value and setting it, could all be > accomplished in a single atomic operation: > > if (rtc::AtomicOps::CompareAndSwap(&channel_state_.sending_, 0) == 0) > return; > > RTC_DCHECK(encoder_queue_); > ... > I totally agree that the current construct is... terrible - I don't find any diplomatic way to put it - but resorting to a one-off solution with atomics seems like adding too much complexity for something which should be very simple. I'd use that in an inner loop, which none of the functions in voe::Channel really is (that's why we've been getting away with taking ~5 locks in GetAudioFrameWithMuted(), for instance). This CL is not making the situation worse (and StopSend() is not called that often), so I suggest we leave such cleanup for a later time. > > [1]: That is assuming that the lock is actually needed on this thread, in the > first place. It could very well be that all modification to the |sending| flag, > is done on the current thread, and if so, the current thread does not need to > hold a lock to examine its value. However, there aren't thread checks or > documentation that explain and verify how this works, so at the moment, it > doesn't look like we really know. This class is riddled with questionable design choices. That's one of the reasons Audio[Send|Receive]Stream access it via ChannelProxy - where we have added thread checks and assertions on inputs/outputs - in an attempt to lock down how voe::Channel is actually used. Once the legacy VoE APIs are gone, we'll be able to strip out everything from voe::Channel which falls outside of those assumptions. Then cleaning up and splitting the class' send/receive duties (it is duplex right now!) into the stream classes will be much easier. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2678: audio_frame->CopyFrom(audio_input); On 2017/03/31 09:44:10, tommi (webrtc) wrote: > It feels like we should be able to avoid this. Can you add a TODO here so that > we don't have to do this allocation+copy for every buffer? The allocation can easily be avoided by aggregating the frame in the task object. AFAICT the copy is needed, unless we do some major surgery and bless the ADM with a pool of buffers it can pass ownership of (something akin to how SwapQueue works - we'd need to send a prealloc:d buffer back for each one we receive).
lgtm https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2728: // Add 10ms of raw (PCM) audio data to the encoder @ 32kHz. This comment is from when channel.cc was originally imported. Do we always send data @ 32 kHz? This looks like a comment that doesn't contribute much and could get stale without anyone noticing. I suggest removing it. If 32 kHz is important, there should be a DCHECK instead. https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2848: // a shared helper. Is this done now?
https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2848: // a shared helper. On 2017/03/31 10:34:40, aleloi wrote: > Is this done now? Don't bother, file mixing will be gone soon.
sorry, I meant to hit the 'lgtm' button in the last round, so doing that now. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1245: channel_state_.SetSending(false); On 2017/03/31 10:19:37, the sun wrote: > On 2017/03/31 09:44:10, tommi (webrtc) wrote: > > It's too bad that these two methods, Get() and SetSending() both grab the same > > lock, copy state, only for the sake of setting a bool and even so, they're > prone > > to a race since when we call SetSending(false), we don't know if |sending| is > > still true[1], which indicates a bug. > > > > The four lock/unlock operations, checking the value and setting it, could all > be > > accomplished in a single atomic operation: > > > > if (rtc::AtomicOps::CompareAndSwap(&channel_state_.sending_, 0) == 0) > > return; > > > > RTC_DCHECK(encoder_queue_); > > ... > > > > I totally agree that the current construct is... terrible - I don't find any > diplomatic way to put it - but resorting to a one-off solution with atomics > seems like adding too much complexity for something which should be very simple. > I'd use that in an inner loop, which none of the functions in voe::Channel > really is (that's why we've been getting away with taking ~5 locks in > GetAudioFrameWithMuted(), for instance). > > This CL is not making the situation worse (and StopSend() is not called that > often), so I suggest we leave such cleanup for a later time. > > > > > [1]: That is assuming that the lock is actually needed on this thread, in the > > first place. It could very well be that all modification to the |sending| > flag, > > is done on the current thread, and if so, the current thread does not need to > > hold a lock to examine its value. However, there aren't thread checks or > > documentation that explain and verify how this works, so at the moment, it > > doesn't look like we really know. > > This class is riddled with questionable design choices. That's one of the > reasons Audio[Send|Receive]Stream access it via ChannelProxy - where we have > added thread checks and assertions on inputs/outputs - in an attempt to lock > down how voe::Channel is actually used. Once the legacy VoE APIs are gone, we'll > be able to strip out everything from voe::Channel which falls outside of those > assumptions. Then cleaning up and splitting the class' send/receive duties (it > is duplex right now!) into the stream classes will be much easier. Sounds good. My preference as far as the flag goes, would be that we clarify the threading model with thread checks and then take action. I suspect that a lock or atomic ops might not be needed and we can start removing much of the extra complexity. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2678: audio_frame->CopyFrom(audio_input); On 2017/03/31 10:19:37, the sun wrote: > On 2017/03/31 09:44:10, tommi (webrtc) wrote: > > It feels like we should be able to avoid this. Can you add a TODO here so that > > we don't have to do this allocation+copy for every buffer? > > The allocation can easily be avoided by aggregating the frame in the task > object. > > AFAICT the copy is needed, unless we do some major surgery and bless the ADM > with a pool of buffers it can pass ownership of (something akin to how SwapQueue > works - we'd need to send a prealloc:d buffer back for each one we receive). yes, something like that should work. A TODO or something for now, just so that we won't forget, works for me.
https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1245: channel_state_.SetSending(false); On 2017/03/31 10:58:52, tommi (webrtc) wrote: > On 2017/03/31 10:19:37, the sun wrote: > > On 2017/03/31 09:44:10, tommi (webrtc) wrote: > > > It's too bad that these two methods, Get() and SetSending() both grab the > same > > > lock, copy state, only for the sake of setting a bool and even so, they're > > prone > > > to a race since when we call SetSending(false), we don't know if |sending| > is > > > still true[1], which indicates a bug. > > > > > > The four lock/unlock operations, checking the value and setting it, could > all > > be > > > accomplished in a single atomic operation: > > > > > > if (rtc::AtomicOps::CompareAndSwap(&channel_state_.sending_, 0) == 0) > > > return; > > > > > > RTC_DCHECK(encoder_queue_); > > > ... > > > > > > > I totally agree that the current construct is... terrible - I don't find any > > diplomatic way to put it - but resorting to a one-off solution with atomics > > seems like adding too much complexity for something which should be very > simple. > > I'd use that in an inner loop, which none of the functions in voe::Channel > > really is (that's why we've been getting away with taking ~5 locks in > > GetAudioFrameWithMuted(), for instance). > > > > This CL is not making the situation worse (and StopSend() is not called that > > often), so I suggest we leave such cleanup for a later time. > > > > > > > > [1]: That is assuming that the lock is actually needed on this thread, in > the > > > first place. It could very well be that all modification to the |sending| > > flag, > > > is done on the current thread, and if so, the current thread does not need > to > > > hold a lock to examine its value. However, there aren't thread checks or > > > documentation that explain and verify how this works, so at the moment, it > > > doesn't look like we really know. > > > > This class is riddled with questionable design choices. That's one of the > > reasons Audio[Send|Receive]Stream access it via ChannelProxy - where we have > > added thread checks and assertions on inputs/outputs - in an attempt to lock > > down how voe::Channel is actually used. Once the legacy VoE APIs are gone, > we'll > > be able to strip out everything from voe::Channel which falls outside of those > > assumptions. Then cleaning up and splitting the class' send/receive duties (it > > is duplex right now!) into the stream classes will be much easier. > > Sounds good. My preference as far as the flag goes, would be that we clarify > the threading model with thread checks and then take action. I suspect that a > lock or atomic ops might not be needed and we can start removing much of the > extra complexity. I believe your suspicion is correct...
Thanks all. Landing a compromise solution. https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2728: // Add 10ms of raw (PCM) audio data to the encoder @ 32kHz. It has been removed since the comment was invalid. https://codereview.webrtc.org/2665693002/diff/660001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2848: // a shared helper. On 2017/03/31 10:49:19, the sun wrote: > On 2017/03/31 10:34:40, aleloi wrote: > > Is this done now? > > Don't bother, file mixing will be gone soon. Acknowledged. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... File webrtc/voice_engine/channel.cc (right): https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:1146: RTC_DCHECK(encoder_queue); Wow, why not actually ;-) https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2675: RTC_DCHECK(encoder_queue_); On 2017/03/31 09:44:10, tommi (webrtc) wrote: > we don't DCHECK pointers that we dereference anyway Acknowledged. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2678: audio_frame->CopyFrom(audio_input); On 2017/03/31 10:58:52, tommi (webrtc) wrote: > On 2017/03/31 10:19:37, the sun wrote: > > On 2017/03/31 09:44:10, tommi (webrtc) wrote: > > > It feels like we should be able to avoid this. Can you add a TODO here so > that > > > we don't have to do this allocation+copy for every buffer? > > > > The allocation can easily be avoided by aggregating the frame in the task > > object. > > > > AFAICT the copy is needed, unless we do some major surgery and bless the ADM > > with a pool of buffers it can pass ownership of (something akin to how > SwapQueue > > works - we'd need to send a prealloc:d buffer back for each one we receive). > > yes, something like that should work. A TODO or something for now, just so that > we won't forget, works for me. Done. https://codereview.webrtc.org/2665693002/diff/680001/webrtc/voice_engine/chan... webrtc/voice_engine/channel.cc:2688: RTC_DCHECK(encoder_queue_); On 2017/03/31 09:44:10, tommi (webrtc) wrote: > this pointer is used anyway below, so no need to dcheck Done.
The CQ bit was checked by henrika@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org, tommi@webrtc.org, aleloi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2665693002/#ps700001 (title: "Final comments from Tommi")
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": 700001, "attempt_start_ts": 1490960603181520,
"parent_rev": "36e9eb4773d1a27d67ef632b9537bae82e3d6b6b", "commit_rev":
"ec6fbd277623cc60007d6a57d91f1e6b78ad6b04"}
Message was sent while issue was closed.
Description was changed from ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng ========== to ========== Moves channel-dependent audio input processing to separate encoder task queue. First approach to remove parts of the heavy load done for encoding, and preparation for sending, from native audio thread to separate task queue. With this change we will give the native input audio thread more time to "relax" between successive audio captures. Separate profiling done on Android has verified that the change works well; the load is now redistributed and the load of the native AudioRecordThread is reduced. Similar conclusions should be valid for all other OS:es as well. BUG=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_compile_dbg,linux_android_rel_ng Review-Url: https://codereview.webrtc.org/2665693002 Cr-Commit-Position: refs/heads/master@{#17488} Committed: https://chromium.googlesource.com/external/webrtc/+/ec6fbd277623cc60007d6a57d... ==========
Message was sent while issue was closed.
Committed patchset #35 (id:700001) as https://chromium.googlesource.com/external/webrtc/+/ec6fbd277623cc60007d6a57d... |
