Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2865113002:
AecDump implementation. (Closed)
|
Created:
3 years, 7 months ago by aleloi Modified:
3 years, 7 months ago Reviewers:
peah-webrtc CC:
webrtc-reviews_webrtc.org, AleBzk, peah-webrtc, Andrew MacDonald, aleloi, tterriberry_mozilla.com, audio-team_agora.io, hlundin-webrtc, kwiberg-webrtc, minyue-webrtc, the sun, aluebs-webrtc, bjornv1 Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAecDump implementation.
This CL implements webrtc::AecDump, which is an interface defined in
https://codereview.webrtc.org/2778783002.
This AudioProcessing submodule writes audio and APM state to a
file. The file writing is done by posting IO tasks
(write_to_file_task.h) on an rtc::TaskQueue. There is an existing
implementation for this through AudioProcessing::StartDebugRecording()
and AudioProcessing::StopDebugRecording(). This implementation still
works, and is used as the default until this dependent CL:
https://codereview.webrtc.org/2896813002/.
To be able to build webrtc without protobuf support, the interface is
isolated from protobuf types. Audio data from AudioProcessing is
passed to AecDumpImpl through the AecDump interface. There it is
stored in protobuf objects, which are posted on the task queue.
This functionality is verified correct by the CL
https://codereview.webrtc.org/2864373002, which enables this recording
submodule in APM tests.
BUG=webrtc:7404
Review-Url: https://codereview.webrtc.org/2865113002
Cr-Commit-Position: refs/heads/master@{#18241}
Committed: https://chromium.googlesource.com/external/webrtc/+/06013e99ac3f2526784627ca5e0eea78c617db87
Patch Set 1 : Clean up of impl code. #
Total comments: 11
Patch Set 2 : Rebase on small fixes. #
Total comments: 21
Patch Set 3 : Comments, factory methods, one lock less. #
Total comments: 19
Patch Set 4 : Minor issues; MakeUnique & updated unit test. #Patch Set 5 : Add event type for Capture Stream. #Patch Set 6 : Rebase & remove 'forced' functionality. #Patch Set 7 : Use ProtoString and fix protobuf utils deps for internal project. #Patch Set 8 : Removed extra build deps from dependent CL. #Depends on Patchset: Messages
Total messages: 67 (56 generated)
The CQ bit was checked by aleloi@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_memcheck on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_memcheck/builds/7794)
The CQ bit was checked by aleloi@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 checked by aleloi@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/21331)
Description was changed from ========== New version of AecDump implementation without polymorphism. BUG=Add later. ========== to ========== AecDump implementation without polymorphism. This CL implements the AecDump, which was declared in https://codereview.webrtc.org/2778783002. This APM submodule posts file IO tasks (write_to_file_task.h) on a Task Queue. In an upcoming CL the defalut AecDump recording of APM will change to this implementation. The CL https://codereview.webrtc.org/2864373002 enables this recording submodule in APM tests. TODO(aleloi): expand, elaborate BUG=webrtc:7404 ==========
aleloi@webrtc.org changed reviewers: + peah@webrtc.org
PTAL on the (non-virtual) AecDump implementation!
The CQ bit was checked by aleloi@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.
Hi, Thanks for the new non-polymorphic variant of the AecDump implementation. It avoids the need for virtual member calls, but I'm concerned that there is still quite a lot of object creation required during runtime. The main motivation for moving the AecDump functionality from the AudioProcessingImpl class is to reduce the file-write overhead when making aecdumps, in order to ensure that these have a minimum impact on the performance of the real-time behavior. For the same purpose, the current aecdump recording functionality avoids doing runtime-creation of objects related to the recording by caching the Event objects for the render and capture on the state (the AudioProcessingImpl object). The new AecDump recording functionality introduces some further dynamic creation of objects during runtime that I don't think are necessary, but rather is an effect of the way the AecDump, CaptureStreamInfo and FileWriteTask classes are implemented. In the current CL, each capture frame requires three objects to be dynamically created: CaptureStreamInfo, Event and FileWriteTaskm and each render frame requires two objects to be dynamically created: Event and a FileWriteTask. I don't see any easy way of avoiding FileWriteTask objects to be created, but avoiding the creation of Event and CaptureStreamInfo objects is definitely doable and should be straightforward. https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec_dump/aec_dump.cc (right): https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:68: thread_sync_event.Wait(rtc::Event::kForever); What does this do? Can this wait forever in the destructor? https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:105: void AecDump::WriteRenderStreamMessage(FloatAudioFrame src) { Instead of passing a FloatAudioFrame as aec_dump_->WriteRenderStreamMessage(FloatAudioFrame(src, num_channels, channel_size)); have you considered changing the call to aec_dump_->WriteRenderStreamMessage(src, num_channels, channel_size); That would eliminate the need for FloatAudioFrame. The for loop would then instead be for (size_t i = 0; i < num_channels; ++i) { const auto& channel_view = src.channel(i); msg->add_channel(&src[i], sizeof(float) * channel_size); } WDYT? https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:159: auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event()); I'd prefer to make this part of the WriteToFileTask. As it is now, it is injected into WriteToFileTask but due to that construction it needs to be created separately. Would it be possible to do this as the following instead. std::unique_ptr<rtc::QueuedTask> task(new WriteToFileTask(debug_file_.get(), &num_bytes_left_for_log_))); audioproc::Event* = task.GetEvent(); .... [update the fields of Event] .... worker_queue_->PostTask(std::move(task)); That approach would eliminate the need for a separate newing up of Event(). Furthermore, the postTask method would not be needed. WDYT? https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec_dump/aec_dump_no_protobuf.cc (right): https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump_no_protobuf.cc:40: void AecDump::CaptureStreamInfo::AddInput(FloatAudioFrame src) {} Would it make sense to put RTC_NOTREACHED() here? Is there any scenario where we would allow this code to be run used when no aecdump recording is built in? https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc:25: : event_(new audioproc::Event()) { I think the separation of the capture stream information writing into a separate class is a bit unfortunate. Sorry for not flagging for this before but it was not clear to me when reviewing the code for the generic aec_dump interface. My concerns are the following: -The separation of this functionality from AecDump is the only thing that enforces creation of separate Events. In the current implementation, the events for render and capture are cached in order to avoid having to create new events for each frame. If creation of new Event objects for each frame is not needed, I think it should not be introduced in this CL (which has the main purpose of reducing the cost of writing AECdumps). -As it is now, all the writing of the capture information is abstracted to this class while the writing of the render information is still in the aec_dump.cc. -The usage of CaptureStreamInfo requires creation of a new CaptureStreamInfo object for each capture frame, right? (as GetEventMessage moves the ownership of event_) https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc:87: std::unique_ptr<audioproc::Event> AecDump::CaptureStreamInfo::GetEventMsg() { This part of the class is a bit error prone. It means that none of methods in the class can be used after the GetEventMsg() method has been called.
The CQ bit was checked by aleloi@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/...
PTAL at new patch! https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec_dump/aec_dump.cc (right): https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:68: thread_sync_event.Wait(rtc::Event::kForever); On 2017/05/09 07:14:57, peah-webrtc wrote: > What does this do? Can this wait forever in the destructor? It pauses execution until 'thread_sync_event.Set()' in the lambda function on line 67 has been run. This is to ensure the task queue has pointers to an AecDump object from inside pending tasks when the d-tor has finished. It shouldn't wait forever. The same pattern is used in AudioSendStream, VideoSendStream and rtc event logging. I've added a comment. https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:105: void AecDump::WriteRenderStreamMessage(FloatAudioFrame src) { On 2017/05/09 07:14:57, peah-webrtc wrote: > Instead of passing a FloatAudioFrame as > aec_dump_->WriteRenderStreamMessage(FloatAudioFrame(src, num_channels, > channel_size)); > have you considered changing the call to > aec_dump_->WriteRenderStreamMessage(src, num_channels, channel_size); > > That would eliminate the need for FloatAudioFrame. > The for loop would then instead be > for (size_t i = 0; i < num_channels; ++i) { > const auto& channel_view = src.channel(i); > msg->add_channel(&src[i], sizeof(float) * channel_size); > } > > WDYT? I have a mild preference for leaving FloatAudioFrame. If we were to change the interface to (float** data, num_channels, channel_size), there would be three arguments for what is conceptually the same thing. FloadAudioFrame helps to use the interface correctly by showing how the data will be used through the public method channel(idx). It also helps in the implementation of an AecDump subclass by showing how the data is supposed to be used. https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/aec_dump.cc:159: auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event()); On 2017/05/09 07:14:57, peah-webrtc wrote: > I'd prefer to make this part of the WriteToFileTask. As it is now, it is > injected into WriteToFileTask but due to that construction it needs to be > created separately. > > Would it be possible to do this as the following instead. > > std::unique_ptr<rtc::QueuedTask> task(new WriteToFileTask(debug_file_.get(), > &num_bytes_left_for_log_))); > audioproc::Event* = task.GetEvent(); > .... > [update the fields of Event] > .... > worker_queue_->PostTask(std::move(task)); > > That approach would eliminate the need for a separate newing up of Event(). > Furthermore, the postTask method would not be needed. > > WDYT? That makes everything a little simpler. I like it! https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... File webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc:25: : event_(new audioproc::Event()) { On 2017/05/09 07:14:57, peah-webrtc wrote: > I think the separation of the capture stream information writing into a separate > class is a bit unfortunate. Sorry for not flagging for this before but it was > not clear to me when reviewing the code for the generic aec_dump interface. > > My concerns are the following: > -The separation of this functionality from AecDump is the only thing that > enforces creation of separate Events. In the current implementation, the events > for render and capture are cached in order to avoid having to create new events > for each frame. If creation of new Event objects for each frame is not needed, I > think it should not be introduced in this CL (which has the main purpose of > reducing the cost of writing AECdumps). > > -As it is now, all the writing of the capture information is abstracted to this > class while the writing of the render information is still in the aec_dump.cc. > > -The usage of CaptureStreamInfo requires creation of a new CaptureStreamInfo > object for each capture frame, right? (as GetEventMessage moves the ownership of > event_) Changed after offline discussion. To avoid creating new CaptureStreamInfos, AecDump holds a *single* CaptureStreamInfoImpl. A CaptureStreamInfoImpl holds a WriteToFileTask, which has an audioproc::Event. APM gets a pointer (with CaptureStreamInfo interface). Then APM calls aec_dump_->LogCaptureStream() or aec_dump_->LogCaptureStream(CaptureStreamInfo*). Exact interface is TBD. This calls AecDumpImpl to send the WriteToFileTask owned by its CaptureStreamInfoImpl instance to the task queue. A new WriteToFileTask owned by CaptureStreamInfoImpl is created. CaptureStreamInfoImpl holds the state needed to group all the capture stream data that is logged in one protobuf message. https://codereview.webrtc.org/2865113002/diff/80001/webrtc/modules/audio_proc... webrtc/modules/audio_processing/aec_dump/capture_stream_info_impl.cc:87: std::unique_ptr<audioproc::Event> AecDump::CaptureStreamInfo::GetEventMsg() { On 2017/05/09 07:14:57, peah-webrtc wrote: > This part of the class is a bit error prone. It means that none of methods in > the class can be used after the GetEventMsg() method has been called. The code is rather different now after the CaptureStreamInfo changes.
The CQ bit was checked by aleloi@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_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/13688)
Patchset #6 (id:100001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #3 (id:160001) has been deleted
Description was changed from ========== AecDump implementation without polymorphism. This CL implements the AecDump, which was declared in https://codereview.webrtc.org/2778783002. This APM submodule posts file IO tasks (write_to_file_task.h) on a Task Queue. In an upcoming CL the defalut AecDump recording of APM will change to this implementation. The CL https://codereview.webrtc.org/2864373002 enables this recording submodule in APM tests. TODO(aleloi): expand, elaborate BUG=webrtc:7404 ========== to ========== AecDump implementation. This CL implements the AecDump, which was declared in https://codereview.webrtc.org/2778783002. This APM submodule posts file IO tasks (write_to_file_task.h) on a Task Queue. In an upcoming CL the defalut AecDump recording of APM will change to this implementation. The CL https://codereview.webrtc.org/2864373002 enables this recording submodule in APM tests. TODO(aleloi): expand, elaborate BUG=webrtc:7404 ==========
PTAL on the latest changes to interface and implementation!
Hi, Thanks for the update! It looks great, but I have some comments. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:34: // Closes associated file. Messages waiting to be written to file What does this comment refer to? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:63: void AecDumpImpl::AddCaptureStreamInput(const FloatAudioFrame& src) { You could probably put these (63-79) in the header file if you want. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:124: // auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event()); Is line 124 intended to be commented out? I guess it should be deleted, right? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:144: void CopyFromConfigToEvent(const webrtc::InternalAPMConfig& config, Can this method be put in the anonymous namespace? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:183: rtc::CritScope cs(&config_string_lock_); I don't see that locking is needed for the config_string. It is only accessed in WriteConfig, right? And WriteConfig is in the usage of AecDump only called one at a time. This is a tricky part when having an interface that can be accessed concurrently, but in a way such that the state used by the different thread accesses is separated so that no locks are needed. In cases like this I like the approach of separating the members of the class into nested classes, where each contain only the data that it can operate on. That makes it easier to see that the code is threadsafe by examining the code. But in this case the separation is so trivial so it probably does not make sense, at least for the render part. Another thing thing to do would be to add thread sanitizers/checkers. Are there any such that you can use instead? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h:48: AecDumpImpl(rtc::PlatformFile file, Do these constructors need to be public? If we instead of using a separate factory and put the create methods as part of AecDump, it should be possible to make the constructors private, right? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h:86: rtc::CriticalSection config_string_lock_; I'm not convinced that this lock is really needed. Or am I wrong in that? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc:46: audioproc::Stream* stream = task_->GetEvent()->mutable_stream(); auto* to conform to 24 and 35? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc:54: audioproc::Stream* stream = task_->GetEvent()->mutable_stream(); auto* to conform to 24 and 35? https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/capture_stream_info.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.h:54: RTC_DCHECK(!task_); You probably should add RTC_DCHECK(task); as well.
The CQ bit was checked by aleloi@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_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/20323) linux_more_configs on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_more_configs/buil...) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
The CQ bit was checked by aleloi@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 checked by aleloi@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/...
Please take a look on the latest pset! I've made changes in response to comments, and a small interface change I noticed when updating the comments to the factory: It was not clear what happened when files could not be open. Now the contract is that the factories return nullptr. This required some changes to the constructors of AecDumpImpl. Another issue: we can maybe remove one more lock by using another kind of file handling object than FileWrapper. It has an internal lock, which we don't need, because the task queue runs single-threaded. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:34: // Closes associated file. Messages waiting to be written to file On 2017/05/16 06:30:37, peah-webrtc wrote: > What does this comment refer to? Now updated. I've went through the other comments as well. They should be updated now. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:63: void AecDumpImpl::AddCaptureStreamInput(const FloatAudioFrame& src) { On 2017/05/16 06:30:37, peah-webrtc wrote: > You could probably put these (63-79) in the header file if you want. The chromium-style plugin disagrees: virtual methods with non-empty bodies shouldn't be declared inline. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:124: // auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event()); On 2017/05/16 06:30:37, peah-webrtc wrote: > Is line 124 intended to be commented out? I guess it should be deleted, right? Done. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:144: void CopyFromConfigToEvent(const webrtc::InternalAPMConfig& config, On 2017/05/16 06:30:37, peah-webrtc wrote: > Can this method be put in the anonymous namespace? Done. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:183: rtc::CritScope cs(&config_string_lock_); On 2017/05/16 06:30:38, peah-webrtc wrote: > I don't see that locking is needed for the config_string. It is only accessed in > WriteConfig, right? And WriteConfig is in the usage of AecDump only called one > at a time. > > This is a tricky part when having an interface that can be accessed > concurrently, but in a way such that the state used by the different thread > accesses is separated so that no locks are needed. > > In cases like this I like the approach of separating the members of the class > into nested classes, where each contain only the data that it can operate on. > That makes it easier to see that the code is threadsafe by examining the code. > But in this case the separation is so trivial so it probably does not make > sense, at least for the render part. > > Another thing thing to do would be to add thread sanitizers/checkers. Are there > any such that you can use instead? There was something detected by the tsan bots that led me to add the locks. I can't think of why they would be needed now; maybe the threading model has changed. I've replaced the lock with a race checker. I'll run the new patch on the bots. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h:48: AecDumpImpl(rtc::PlatformFile file, On 2017/05/16 06:30:38, peah-webrtc wrote: > Do these constructors need to be public? If we instead of using a separate > factory and put the create methods as part of AecDump, it should be possible to > make the constructors private, right? It would expose some implementation in the interface. As it is now, the interface contains only what is needed for AudioProcessingImpl to log to it. I'd rather make the factory header a friend class. Another way is to make the AecDumpImpl header 'invisible' through GN. In the build file, only the factory is made visible to outside targets. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h:86: rtc::CriticalSection config_string_lock_; On 2017/05/16 06:30:38, peah-webrtc wrote: > I'm not convinced that this lock is really needed. Or am I wrong in that? I also can't see the need for it. It was definitely needed in an earlier version. Well spotted! https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc:46: audioproc::Stream* stream = task_->GetEvent()->mutable_stream(); On 2017/05/16 06:30:38, peah-webrtc wrote: > auto* to conform to 24 and 35? Done. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.cc:54: audioproc::Stream* stream = task_->GetEvent()->mutable_stream(); On 2017/05/16 06:30:38, peah-webrtc wrote: > auto* to conform to 24 and 35? Done. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/capture_stream_info.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/capture_stream_info.h:54: RTC_DCHECK(!task_); On 2017/05/16 06:30:38, peah-webrtc wrote: > You probably should add > RTC_DCHECK(task); > as well. Done.
Patchset #5 (id:220001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #2 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great! Thanks for the new patch! I don't have that many more comments apart from one major one which concerns the shared resources between the WriteToFileTask and the AecDump instances. It would be really good to avoid that. As it is now, the requirements that one outlive the other is extremely hard to test for, and the result if that is not fulfilled could be really fatal. I threw out some ideas about solving that using reference counted pointers. It would be great to have your input on that. https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h (right): https://codereview.webrtc.org/2865113002/diff/180001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.h:48: AecDumpImpl(rtc::PlatformFile file, On 2017/05/16 20:10:17, aleloi wrote: > On 2017/05/16 06:30:38, peah-webrtc wrote: > > Do these constructors need to be public? If we instead of using a separate > > factory and put the create methods as part of AecDump, it should be possible > to > > make the constructors private, right? > > It would expose some implementation in the interface. As it is now, the > interface contains only what is needed for AudioProcessingImpl to log to it. I'd > rather make the factory header a friend class. > > Another way is to make the AecDumpImpl header 'invisible' through GN. In the > build file, only the factory is made visible to outside targets. Good point! Lets keep the factories :-) https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:28: // The |worker_queue| may not be null and must outlive the created Regarding this: I know we discussed it but is there no way of avoid this requirement? What if log_size_bytes is created dynamically, and both the file-pointer and log_size_bytes are made reference counted? Then the worker thread would not need to outlive the AecDump instance as no explicit synchronization, right? Furthermore, the AecDump instance does not need to outlive the last posted dumped packet, as the reference counted pointer will pass ownership to the Tasks once the AecDump instance is destroyed, right? It would simplify the use of this a lot if we don't have to have the requirement that one of these need to outlive the others which seems as something that definitely will go wrong at some point. WDYT? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:65: // Block until all tasks have finished running. It would be so nice if this could be avoided. Please consider the idea of reference counted pointers for that. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:114: worker_queue_->PostTask(std::unique_ptr<rtc::QueuedTask>(std::move(task))); Just wondering, is it necessary to explicitly create a new unique_ptr of type rtc::QueuedTask here? Cannot that happen automagically? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:186: return std::unique_ptr<AecDumpImpl>( MakeUnique? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:198: return std::unique_ptr<AecDumpImpl>( MakeUnique? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:211: return std::unique_ptr<AecDumpImpl>( MakeUnique? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:20: // Note order of initialization: factory first (has event pool), I don't really follow the comments about the event pools? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:28: // TODO(aleloi): wait a while and check that file1 is Something to address before landing this? https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:34: // TODO(aleloi): open from handle. Something to address before landing this?
The CQ bit was checked by aleloi@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/...
Thanks for the comments! I've mostly done small fixes and not updated the threading/memory/ownership much except for removing a lock. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_factory.h:28: // The |worker_queue| may not be null and must outlive the created On 2017/05/17 05:36:32, peah-webrtc wrote: > Regarding this: I know we discussed it but is there no way of avoid this > requirement? What if > log_size_bytes is created dynamically, and both the file-pointer and > log_size_bytes are made reference counted? > Then the worker thread would not need to outlive the AecDump instance as no > explicit synchronization, right? Furthermore, the AecDump instance does not need > to outlive the last posted dumped packet, as the reference counted pointer will > pass ownership to the Tasks once the AecDump instance is destroyed, right? > > It would simplify the use of this a lot if we don't have to have the requirement > that one of these need to outlive the others which seems as something that > definitely will go wrong at some point. > > WDYT? Acknowledged. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:65: // Block until all tasks have finished running. On 2017/05/17 05:36:33, peah-webrtc wrote: > It would be so nice if this could be avoided. Please consider the idea of > reference counted pointers for that. Acknowledged. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:114: worker_queue_->PostTask(std::unique_ptr<rtc::QueuedTask>(std::move(task))); On 2017/05/17 05:36:33, peah-webrtc wrote: > Just wondering, is it necessary to explicitly create a new unique_ptr of type > rtc::QueuedTask here? Cannot that happen automagically? I think it should work, but it doesn't. It seems to be somehow related to template resolution. The TQ has two relevant methods: void PostTask(std::unique_ptr<QueuedTask> task); template <class Closure> void PostTask(const Closure& closure) { PostTask(std::unique_ptr<QueuedTask>(new ClosureTask<Closure>(closure))); } The compiler thinks that #2 fits better (because the types match exactly without having to do a conversion unique_ptr<WriteToFileTask> -> unique_ptr<QueuedTask), and tries to call the 2:nd method with Closure = unique_ptr<WriteToFileTask>. That fails in the next compilation step. That could be fixed by making the template<Closure> version reject a unique_ptr right away. This works: template<class Closure> typename std::enable_if<std::is_copy_constructible<Closure>::value>::type void PostTask(const Closure& closure) { PostTask(std::unique_ptr<QueuedTask>(new ClosureTask<Closure>(closure))); } I'm not sure we want to make the TaskQueue interface less readable this way, though. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:186: return std::unique_ptr<AecDumpImpl>( On 2017/05/17 05:36:32, peah-webrtc wrote: > MakeUnique? Done. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:198: return std::unique_ptr<AecDumpImpl>( On 2017/05/17 05:36:33, peah-webrtc wrote: > MakeUnique? Done. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:211: return std::unique_ptr<AecDumpImpl>( On 2017/05/17 05:36:33, peah-webrtc wrote: > MakeUnique? Done. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:20: // Note order of initialization: factory first (has event pool), On 2017/05/17 05:36:33, peah-webrtc wrote: > I don't really follow the comments about the event pools? That was some kind of merge issue. The version of aec_dump_unittest.cc in the first patch set is updated, while this one is not. There hasn't been an event pool for a long while... https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:28: // TODO(aleloi): wait a while and check that file1 is On 2017/05/17 05:36:33, peah-webrtc wrote: > Something to address before landing this? Same as above, this file somehow reverted to an earlier version. https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_unittest.cc:34: // TODO(aleloi): open from handle. On 2017/05/17 05:36:33, peah-webrtc wrote: > Something to address before landing this? Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Awesome! Great work! lgtm https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... File webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc (right): https://codereview.webrtc.org/2865113002/diff/240001/webrtc/modules/audio_pro... webrtc/modules/audio_processing/aec_dump/aec_dump_impl.cc:114: worker_queue_->PostTask(std::unique_ptr<rtc::QueuedTask>(std::move(task))); On 2017/05/17 10:49:55, aleloi wrote: > On 2017/05/17 05:36:33, peah-webrtc wrote: > > Just wondering, is it necessary to explicitly create a new unique_ptr of type > > rtc::QueuedTask here? Cannot that happen automagically? > > I think it should work, but it doesn't. It seems to be somehow related to > template resolution. The TQ has two relevant methods: > > void PostTask(std::unique_ptr<QueuedTask> task); > > template <class Closure> > void PostTask(const Closure& closure) { > PostTask(std::unique_ptr<QueuedTask>(new ClosureTask<Closure>(closure))); > } > > The compiler thinks that #2 fits better (because the types match exactly without > having to do a conversion unique_ptr<WriteToFileTask> -> unique_ptr<QueuedTask), > and tries to call the 2:nd method with Closure = unique_ptr<WriteToFileTask>. > That fails in the next compilation step. > > That could be fixed by making the template<Closure> version reject a unique_ptr > right away. This works: > > template<class Closure> > typename std::enable_if<std::is_copy_constructible<Closure>::value>::type > void PostTask(const Closure& closure) { > PostTask(std::unique_ptr<QueuedTask>(new ClosureTask<Closure>(closure))); > } > > > I'm not sure we want to make the TaskQueue interface less readable this way, > though. No, I don't think that makes sense either. Let's keep it as it is.
The CQ bit was checked by aleloi@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.
Description was changed from ========== AecDump implementation. This CL implements the AecDump, which was declared in https://codereview.webrtc.org/2778783002. This APM submodule posts file IO tasks (write_to_file_task.h) on a Task Queue. In an upcoming CL the defalut AecDump recording of APM will change to this implementation. The CL https://codereview.webrtc.org/2864373002 enables this recording submodule in APM tests. TODO(aleloi): expand, elaborate BUG=webrtc:7404 ========== to ========== AecDump implementation. This CL implements webrtc::AecDump, which is an interface defined in https://codereview.webrtc.org/2778783002. This AudioProcessing submodule writes audio and APM state to a file. The file writing is done by posting IO tasks (write_to_file_task.h) on an rtc::TaskQueue. There is an existing implementation for this through AudioProcessing::StartDebugRecording() and AudioProcessing::StopDebugRecording(). This implementation still works, and is used as the default until this dependent CL: https://codereview.webrtc.org/2896813002/. To be able to build webrtc without protobuf support, the interface is isolated from protobuf types. Audio data from AudioProcessing is passed to AecDumpImpl through the AecDump interface. There it is stored in protobuf objects, which are posted on the task queue. This functionality is verified correct by the CL https://codereview.webrtc.org/2864373002, which enables this recording submodule in APM tests. BUG=webrtc:7404 ==========
The CQ bit was checked by aleloi@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org Link to the patchset: https://codereview.webrtc.org/2865113002/#ps340001 (title: "Removed extra build deps from dependent CL.")
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": 340001, "attempt_start_ts": 1495551810563670,
"parent_rev": "f27c5b8d6eed929eae066c417856eb379779f0c0", "commit_rev":
"06013e99ac3f2526784627ca5e0eea78c617db87"}
Message was sent while issue was closed.
Description was changed from ========== AecDump implementation. This CL implements webrtc::AecDump, which is an interface defined in https://codereview.webrtc.org/2778783002. This AudioProcessing submodule writes audio and APM state to a file. The file writing is done by posting IO tasks (write_to_file_task.h) on an rtc::TaskQueue. There is an existing implementation for this through AudioProcessing::StartDebugRecording() and AudioProcessing::StopDebugRecording(). This implementation still works, and is used as the default until this dependent CL: https://codereview.webrtc.org/2896813002/. To be able to build webrtc without protobuf support, the interface is isolated from protobuf types. Audio data from AudioProcessing is passed to AecDumpImpl through the AecDump interface. There it is stored in protobuf objects, which are posted on the task queue. This functionality is verified correct by the CL https://codereview.webrtc.org/2864373002, which enables this recording submodule in APM tests. BUG=webrtc:7404 ========== to ========== AecDump implementation. This CL implements webrtc::AecDump, which is an interface defined in https://codereview.webrtc.org/2778783002. This AudioProcessing submodule writes audio and APM state to a file. The file writing is done by posting IO tasks (write_to_file_task.h) on an rtc::TaskQueue. There is an existing implementation for this through AudioProcessing::StartDebugRecording() and AudioProcessing::StopDebugRecording(). This implementation still works, and is used as the default until this dependent CL: https://codereview.webrtc.org/2896813002/. To be able to build webrtc without protobuf support, the interface is isolated from protobuf types. Audio data from AudioProcessing is passed to AecDumpImpl through the AecDump interface. There it is stored in protobuf objects, which are posted on the task queue. This functionality is verified correct by the CL https://codereview.webrtc.org/2864373002, which enables this recording submodule in APM tests. BUG=webrtc:7404 Review-Url: https://codereview.webrtc.org/2865113002 Cr-Commit-Position: refs/heads/master@{#18241} Committed: https://chromium.googlesource.com/external/webrtc/+/06013e99ac3f2526784627ca5... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:340001) as https://chromium.googlesource.com/external/webrtc/+/06013e99ac3f2526784627ca5... |
