Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2582703003:
Audio output debug recording. (Closed)
|
Created:
4 years ago by Henrik Grunell Modified:
3 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, miu+watch_chromium.org, miu, xjz+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudio output debug recording.
Class diagram: https://docs.google.com/drawings/d/1QQwzUYpEZwB9R4kW5kPuCUxOHWjuJpo_ckaCvFj0MxI/edit?usp=sharing
Adds two new classes in media/:
* AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop.
* AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them.
Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API).
BUG=531883
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2582703003
Cr-Commit-Position: refs/heads/master@{#454218}
Committed: https://chromium.googlesource.com/chromium/src/+/086a4113b499025bdd7db2c19a10bd8cd34a5997
Patch Set 1 #
Total comments: 2
Patch Set 2 : Working (although not ready) implementation for PulseAudio. #Patch Set 3 : Control through AudioManager. Compiles on Linux, works with PulseAudio. #
Total comments: 28
Patch Set 4 : Code review (dalecurtis@ and maxmorin@). #
Total comments: 19
Patch Set 5 : Changed to record in AudioOutputResampler. #
Total comments: 1
Patch Set 6 : Using callbacks instead. #
Total comments: 9
Patch Set 7 : Reworked callbacks and interfaces. #
Total comments: 14
Patch Set 8 : Code review. #
Total comments: 31
Patch Set 9 : Code review. Rebase. #
Total comments: 29
Patch Set 10 : Code review. #
Total comments: 6
Patch Set 11 : Code review. #Patch Set 12 : Rebase. #
Total comments: 2
Patch Set 13 : FilePath string type fixes. Rebase. #Patch Set 14 : Two more fixes. #
Total comments: 4
Patch Set 15 : Code review (pfeldman@/dalecurtis@) and a fix. Rebase. #
Total comments: 2
Patch Set 16 : Changed to initialize recording in AudioManager::Create(). #Patch Set 17 : Intermediate patch set: rebase after AudioDebugFileWriter move. #Patch Set 18 : Updates after the AudioDebugFileWriter move. #Patch Set 19 : Fix in a unit test. Rebase. #
Total comments: 2
Patch Set 20 : Code review. #Messages
Total messages: 134 (60 generated)
Description was changed from ========== Audio output debug recording. BUG=531883 ========== to ========== Audio output debug recording. BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Audio output debug recording. BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Audio output debug recording. BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
grunell@chromium.org changed reviewers: + dalecurtis@google.com
Dale: this is my proposal of adding audio output debug recording, just outlined now for discussion. Ptal. We want to record the data as close to the OS/audio system as possible. This approach lets each stream implementation create and own the file writer. (Actually, I think we should inject it instead, could be a raw pointer since AudioOutputController outlives the stream.) The stream will issue Write() calls on the file writer. For Windows (and other?) we could do loopback recording. Added enable and disable to AudioOutputStream interface. https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... media/audio/audio_output_controller.h:115: static scoped_refptr<AudioOutputController> Create( Unless AudioInputDebugWriter (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), however renamed, can be moved into media/, we have to inject the file writer here. https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) {} Create and start the file writer here, if OK to move AudioInputDebugWriter (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), into media/. Otherwise, only start.
grunell@chromium.org changed reviewers: + maxmorin@chromium.org
grunell@chromium.org changed reviewers: + olka@chromium.org
Olga and Max, ptal. If we want to avoid adding the enable/disable functions to the stream interface, one option is to inject the writer, let AOC own it and start and stop it, have the stream always check WillWrite() and call Write(). On 2016/12/16 14:12:59, Henrik Grunell wrote: > Dale: this is my proposal of adding audio output debug recording, just outlined > now for discussion. Ptal. > > We want to record the data as close to the OS/audio system as possible. This > approach lets each stream implementation create and own the file writer. > (Actually, I think we should inject it instead, could be a raw pointer since > AudioOutputController outlives the stream.) The stream will issue Write() calls > on the file writer. > > For Windows (and other?) we could do loopback recording. > > Added enable and disable to AudioOutputStream interface. > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > File media/audio/audio_output_controller.h (right): > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > media/audio/audio_output_controller.h:115: static > scoped_refptr<AudioOutputController> Create( > Unless AudioInputDebugWriter > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > however renamed, can be moved into media/, we have to inject the file writer > here. > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > File media/audio/pulse/pulse_output.cc (right): > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) {} > Create and start the file writer here, if OK to move AudioInputDebugWriter > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > into media/. Otherwise, only start.
maxmorin@chromium.org changed reviewers: + dalecurtis@chromium.org - dalecurtis@google.com
-R wrong Dale, +R right Dale
On 2016/12/19 09:04:50, Henrik Grunell wrote: > Olga and Max, ptal. > > If we want to avoid adding the enable/disable functions to the stream interface, > one option is to inject the writer, let AOC own it and start and stop it, have > the stream always check WillWrite() and call Write(). > > On 2016/12/16 14:12:59, Henrik Grunell wrote: > > Dale: this is my proposal of adding audio output debug recording, just > outlined > > now for discussion. Ptal. > > > > We want to record the data as close to the OS/audio system as possible. This > > approach lets each stream implementation create and own the file writer. > > (Actually, I think we should inject it instead, could be a raw pointer since > > AudioOutputController outlives the stream.) The stream will issue Write() > calls > > on the file writer. > > > > For Windows (and other?) we could do loopback recording. > > > > Added enable and disable to AudioOutputStream interface. > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > File media/audio/audio_output_controller.h (right): > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > media/audio/audio_output_controller.h:115: static > > scoped_refptr<AudioOutputController> Create( > > Unless AudioInputDebugWriter > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > however renamed, can be moved into media/, we have to inject the file writer > > here. > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > File media/audio/pulse/pulse_output.cc (right): > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) {} > > Create and start the file writer here, if OK to move AudioInputDebugWriter > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > into media/. Otherwise, only start. This approach goes well with https://codereview.chromium.org/2578983003/
On 2016/12/19 09:23:27, o1ka OOO Dec 23 - Jan 3 wrote: > On 2016/12/19 09:04:50, Henrik Grunell wrote: > > Olga and Max, ptal. > > > > If we want to avoid adding the enable/disable functions to the stream > interface, > > one option is to inject the writer, let AOC own it and start and stop it, have > > the stream always check WillWrite() and call Write(). > > > > On 2016/12/16 14:12:59, Henrik Grunell wrote: > > > Dale: this is my proposal of adding audio output debug recording, just > > outlined > > > now for discussion. Ptal. > > > > > > We want to record the data as close to the OS/audio system as possible. This > > > approach lets each stream implementation create and own the file writer. > > > (Actually, I think we should inject it instead, could be a raw pointer since > > > AudioOutputController outlives the stream.) The stream will issue Write() > > calls > > > on the file writer. > > > > > > For Windows (and other?) we could do loopback recording. > > > > > > Added enable and disable to AudioOutputStream interface. > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > File media/audio/audio_output_controller.h (right): > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > media/audio/audio_output_controller.h:115: static > > > scoped_refptr<AudioOutputController> Create( > > > Unless AudioInputDebugWriter > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > however renamed, can be moved into media/, we have to inject the file writer > > > here. > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > File media/audio/pulse/pulse_output.cc (right): > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) {} > > > Create and start the file writer here, if OK to move AudioInputDebugWriter > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > into media/. Otherwise, only start. > > This approach goes well with https://codereview.chromium.org/2578983003/ Yes. I added a stub method for debug recording to AudioOutputDelegate in that CL. As for this CL, approach looks good except that you need the update the implementation for each platform, but if you can do that then go ahead.
Do all these layers need to know about this or should we just have an AudioManager::EnableDebugRecording() function which flips all streams into debug recording.
On 2016/12/20 17:26:47, DaleCurtis_OOO_Dec14_Jan6 wrote: > Do all these layers need to know about this or should we just have an > AudioManager::EnableDebugRecording() function which flips all streams into debug > recording. That is appealing. However, the render process id and stream id are needed to construct the filename. So that information has to come from somewhere when enabling.
On 2016/12/20 08:35:27, Max Morin OOO 22 Dec - 9 Jan wrote: > On 2016/12/19 09:23:27, o1ka OOO Dec 23 - Jan 3 wrote: > > On 2016/12/19 09:04:50, Henrik Grunell wrote: > > > Olga and Max, ptal. > > > > > > If we want to avoid adding the enable/disable functions to the stream > > interface, > > > one option is to inject the writer, let AOC own it and start and stop it, > have > > > the stream always check WillWrite() and call Write(). > > > > > > On 2016/12/16 14:12:59, Henrik Grunell wrote: > > > > Dale: this is my proposal of adding audio output debug recording, just > > > outlined > > > > now for discussion. Ptal. > > > > > > > > We want to record the data as close to the OS/audio system as possible. > This > > > > approach lets each stream implementation create and own the file writer. > > > > (Actually, I think we should inject it instead, could be a raw pointer > since > > > > AudioOutputController outlives the stream.) The stream will issue Write() > > > calls > > > > on the file writer. > > > > > > > > For Windows (and other?) we could do loopback recording. > > > > > > > > Added enable and disable to AudioOutputStream interface. > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > > File media/audio/audio_output_controller.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > > media/audio/audio_output_controller.h:115: static > > > > scoped_refptr<AudioOutputController> Create( > > > > Unless AudioInputDebugWriter > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > > however renamed, can be moved into media/, we have to inject the file > writer > > > > here. > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > > File media/audio/pulse/pulse_output.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > > media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) {} > > > > Create and start the file writer here, if OK to move AudioInputDebugWriter > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > > into media/. Otherwise, only start. > > > > This approach goes well with https://codereview.chromium.org/2578983003/ > > Yes. I added a stub method for debug recording to AudioOutputDelegate in that > CL. As for this CL, approach looks good except that you need the update the > implementation for each platform, but if you can do that then go ahead. Actually, the output controller sometimes switches output (for example on SwitchOutputDevice or when the mirroring manager is diverting). In this case, we will start a new recording (or not have a recording at all, if it bypasses OS output in situations like tab capturing). Not sure if this is a (serious) problem. You probably want to CC miu on this one.
On 2016/12/21 13:01:19, Max Morin OOO 22 Dec - 9 Jan wrote: > On 2016/12/20 08:35:27, Max Morin OOO 22 Dec - 9 Jan wrote: > > On 2016/12/19 09:23:27, o1ka OOO Dec 23 - Jan 3 wrote: > > > On 2016/12/19 09:04:50, Henrik Grunell wrote: > > > > Olga and Max, ptal. > > > > > > > > If we want to avoid adding the enable/disable functions to the stream > > > interface, > > > > one option is to inject the writer, let AOC own it and start and stop it, > > have > > > > the stream always check WillWrite() and call Write(). > > > > > > > > On 2016/12/16 14:12:59, Henrik Grunell wrote: > > > > > Dale: this is my proposal of adding audio output debug recording, just > > > > outlined > > > > > now for discussion. Ptal. > > > > > > > > > > We want to record the data as close to the OS/audio system as possible. > > This > > > > > approach lets each stream implementation create and own the file writer. > > > > > (Actually, I think we should inject it instead, could be a raw pointer > > since > > > > > AudioOutputController outlives the stream.) The stream will issue > Write() > > > > calls > > > > > on the file writer. > > > > > > > > > > For Windows (and other?) we could do loopback recording. > > > > > > > > > > Added enable and disable to AudioOutputStream interface. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > > > File media/audio/audio_output_controller.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/audio_output_co... > > > > > media/audio/audio_output_controller.h:115: static > > > > > scoped_refptr<AudioOutputController> Create( > > > > > Unless AudioInputDebugWriter > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > > > however renamed, can be moved into media/, we have to inject the file > > writer > > > > > here. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > > > File media/audio/pulse/pulse_output.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2582703003/diff/1/media/audio/pulse/pulse_out... > > > > > media/audio/pulse/pulse_output.cc:254: const base::FilePath& file_name) > {} > > > > > Create and start the file writer here, if OK to move > AudioInputDebugWriter > > > > > > > > > > > > > > > (https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi...), > > > > > into media/. Otherwise, only start. > > > > > > This approach goes well with https://codereview.chromium.org/2578983003/ > > > > Yes. I added a stub method for debug recording to AudioOutputDelegate in that > > CL. As for this CL, approach looks good except that you need the update the > > implementation for each platform, but if you can do that then go ahead. > > Actually, the output controller sometimes switches output (for example on > SwitchOutputDevice or when the mirroring manager is diverting). In this case, we > will start a new recording (or not have a recording at all, if it bypasses OS > output in situations like tab capturing). Not sure if this is a (serious) > problem. You probably want to CC miu on this one. Thanks for pointing that out. I think I commented on switching device in the new patch set, or maybe it was something else related to changing device. We would normally only want to record what's physically being played out on the machine. I'll cc miu@.
I've uploaded a new patch set. It's a poc implementation for PulseAudio, but there are things to work out: * Handle idle streams. (Re-use writer?) * Handle recreation of stream at device change. * For both above, we could inject a writer factory instead of a writer. * Threading: AudioFileWriter must be called on the same thread. OK if the stream implementation handles thread jumps? Or should we give an "output data" callback to the stream and let upper layers do that? (In this code I've disabled a thread check temporarily just for testing.) * Enable/Disable calls should not be needed. AOC can own the writer and start/stop it directly. This assumes AOC outlives the stream. * Need to start recording on newly created stream if recording is enabled. * Update unit tests. The data should be tapped close to the OS/audio system, i.e. from stream implementations. Though as much of the common code as possible should of course be in a single place. So dealing with threading should be done in that common place, hence the idea above of just giving a "output data" callback to the streams. I also renamed AudioInputWriter interface -> AudioFileWriter, and the content implementation of it. I think this should go into a separate CL to make reviewing easier - will do that after holidays.
On 2016/12/21 13:29:08, Henrik Grunell OOO back Jan 10 wrote: > I've uploaded a new patch set. It's a poc implementation for PulseAudio, but > there are things to work out: > > * Handle idle streams. (Re-use writer?) > * Handle recreation of stream at device change. > * For both above, we could inject a writer factory instead of a writer. > * Threading: AudioFileWriter must be called on the same thread. OK if the stream > implementation handles thread jumps? Or should we give an "output data" callback > to the stream and let upper layers do that? (In this code I've disabled a thread > check temporarily just for testing.) > * Enable/Disable calls should not be needed. AOC can own the writer and > start/stop it directly. This assumes AOC outlives the stream. > * Need to start recording on newly created stream if recording is enabled. > * Update unit tests. > > The data should be tapped close to the OS/audio system, i.e. from stream > implementations. Though as much of the common code as possible should of course > be in a single place. So dealing with threading should be done in that common > place, hence the idea above of just giving a "output data" callback to the > streams. > > I also renamed AudioInputWriter interface -> AudioFileWriter, and the content > implementation of it. I think this should go into a separate CL to make > reviewing easier - will do that after holidays. Wow, this looks big now. Plumbing debug recording through so many layers to get a sane file recording name looks like too much of an effort, there should be some simpler approach. I agree with Dale - low-level recording should be done at AudioManager layer. And you can reuse parts of this CL if we want to have controller-level output recording (right after IPC edge)
Sure, bypassing all the layers and controlling debug recording through AM makes sense. It won't make sense when migrating to the audio process, but I bet we're going to have to rethink the plumbing for this no matter how clever we try to be in advance :).
On 2017/01/09 13:29:55, Max Morin wrote: > Sure, bypassing all the layers and controlling debug recording through AM makes > sense. It won't make sense when migrating to the audio process, but I bet we're > going to have to rethink the plumbing for this no matter how clever we try to be > in advance :). Yes, it's preferable to use the AudioManager. The only reason for passing through as done now, is to be able to use the render process id and the stream id in the file name. The stream id is currently only used to create a unique filename, so it could be replaced. But if we want to later also record close to the browser/render process boundary, it's helpful to see which files belong together. The render process id is good to have also to see which files belong together in case of multiple processes. We could probably live without this: the typical use case (for WebRTC) is a single call, and it's also in most cases possible to listen to the contents of the files to group them afterwards. However, if multiple sources are being played out in different render processes it could be impossible to figure out afterwards. Another idea: store render process id and stream id in AudioManagerBase::DispatcherParams, but that would also add passing info just for this purpose. Note 1: The AudioManager knows about dispatchers, so that layer has to be passed through either way. Note 2: The size for the patch set 2 is partly because of the rename of AudioInputWriter. That change can be moved out to its own CL. Note 3: The stream id in the DispatcherImpl is not the same and can't be used since it's unique and internal for that dispatcher only. Thoughts?
On 2017/01/11 13:32:33, Henrik Grunell wrote: > On 2017/01/09 13:29:55, Max Morin wrote: > > Sure, bypassing all the layers and controlling debug recording through AM > makes > > sense. It won't make sense when migrating to the audio process, but I bet > we're > > going to have to rethink the plumbing for this no matter how clever we try to > be > > in advance :). > > Yes, it's preferable to use the AudioManager. The only reason for passing > through as done now, is to be able to use the render process id and the stream > id in the file name. The stream id is currently only used to create a unique > filename, so it could be replaced. But if we want to later also record close to > the browser/render process boundary, it's helpful to see which files belong > together. The render process id is good to have also to see which files belong > together in case of multiple processes. > > We could probably live without this: the typical use case (for WebRTC) is a > single call, and it's also in most cases possible to listen to the contents of > the files to group them afterwards. However, if multiple sources are being > played out in different render processes it could be impossible to figure out > afterwards. > > Another idea: store render process id and stream id in > AudioManagerBase::DispatcherParams, but that would also add passing info just > for this purpose. > > Note 1: The AudioManager knows about dispatchers, so that layer has to be passed > through either way. > > Note 2: The size for the patch set 2 is partly because of the rename of > AudioInputWriter. That change can be moved out to its own CL. > > Note 3: The stream id in the DispatcherImpl is not the same and can't be used > since it's unique and internal for that dispatcher only. > > Thoughts? Olga, Max and I discussed this offline, and came to the conclusion that we should be able to omit render process id and stream id without any large implication on analyzing the recordings. I'll put the class name changes in a separate CL, and change this one to have AM enable recording on all streams, and any newly created if recording is ongoing.
Patchset #3 (id:40001) has been deleted
Now controlling output debug recording through AudioManager. Ptal. Remaining: * Decide on filename. Discussion ongoing over mail about unique naming. * Implementation for other streams than PulseAudio. * Tests * Unit, integration, manual on all platforms. * Both webrtc-internals and the extension API. * Verify that re-use of streams works.
Went through about half so far. https://codereview.chromium.org/2582703003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:112: media::AudioManager::Get()->GetTaskRunner()->PostTask( Just call Get() once and reuse value? https://codereview.chromium.org/2582703003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2582703003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.cc:67: void Write(const char (&data)[4]) { This is a strange set of methods. Why not have a general StringPiece writer? https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:337: media::AudioManager::Get()->GetTaskRunner()->PostTask( Ditto on reuse. https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:511: media::AudioManager::Get()->GetTaskRunner()->PostTask( Ditto. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:47: if (WillWrite()) { early return instead. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:30: base::SingleThreadTaskRunner* task_runner); scoped_refptr https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:55: }; DISALLOW https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... media/audio/audio_io.h:110: // Enable debug recording. A file type extension is added to |file_name|. Hmm this is an API only AudioManager should use, so I don't think it belongs on this interface. Instead we might want a new AudioDebugRecording interface that output streams also implement. Or some way to avoid this API at all. Potentially by using the AudioOutputResampler to trigger this recording instead (it intercepts the audio data).
Thanks Dale. One reply to the interface comment. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... media/audio/audio_io.h:110: // Enable debug recording. A file type extension is added to |file_name|. On 2017/01/19 17:44:34, DaleCurtis wrote: > Hmm this is an API only AudioManager should use, so I don't think it belongs on > this interface. Instead we might want a new AudioDebugRecording interface that > output streams also implement. Or some way to avoid this API at all. Potentially > by using the AudioOutputResampler to trigger this recording instead (it > intercepts the audio data). How could AudioOutputResampler do this without an API? I guess I'm not familiar with the interception mechanism and how that can be used. Also, the AOR isn't always used, right? An AudioOutputDispatcherImpl could be used directly afaik.
https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... media/audio/audio_io.h:110: // Enable debug recording. A file type extension is added to |file_name|. On 2017/01/19 at 20:32:39, Henrik Grunell wrote: > On 2017/01/19 17:44:34, DaleCurtis wrote: > > Hmm this is an API only AudioManager should use, so I don't think it belongs on > > this interface. Instead we might want a new AudioDebugRecording interface that > > output streams also implement. Or some way to avoid this API at all. Potentially > > by using the AudioOutputResampler to trigger this recording instead (it > > intercepts the audio data). > > How could AudioOutputResampler do this without an API? I guess I'm not familiar with the interception mechanism and how that can be used. Also, the AOR isn't always used, right? An AudioOutputDispatcherImpl could be used directly afaik. AODI is only used for fake streams these days, so it would miss some. AM also uses AOR directly, so no interface API is needed, just some direct methods on AOR.
Just some minor comments. WDYT about landing support for (e.g.) pulse only in this CL, and do the rest in smaller CLs? https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... media/audio/audio_manager.cc:48: // Dummy fucntion for creating debug writer. function https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... media/audio/audio_manager_base.cc:94: CHECK(!create_audio_file_writer_callback_.Equals( is it enough with CHECK(create_audio_file_writer_callback_) or CHECK(!create_audio_file_writer_callback_.is_null())? https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_proxy.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_proxy.cc:78: // Must only be called by AudoiManager directly on physical streams. Audio https://codereview.chromium.org/2582703003/diff/60001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:173: debug_recording_helper_->MaybeWrite(audio_bus_.get()); Move this below call to PA, to reduce risk of timeouts?
https://codereview.chromium.org/2582703003/diff/60001/chrome/browser/media/we... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/60001/chrome/browser/media/we... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:112: media::AudioManager::Get()->GetTaskRunner()->PostTask( On 2017/01/19 17:44:33, DaleCurtis wrote: > Just call Get() once and reuse value? Done. https://codereview.chromium.org/2582703003/diff/60001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2582703003/diff/60001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.cc:67: void Write(const char (&data)[4]) { On 2017/01/19 17:44:33, DaleCurtis wrote: > This is a strange set of methods. Why not have a general StringPiece writer? This is just the work of git cl format, and I haven't written the code. So I don't know either. https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:337: media::AudioManager::Get()->GetTaskRunner()->PostTask( On 2017/01/19 17:44:34, DaleCurtis wrote: > Ditto on reuse. Done. https://codereview.chromium.org/2582703003/diff/60001/content/browser/webrtc/... content/browser/webrtc/webrtc_internals.cc:511: media::AudioManager::Get()->GetTaskRunner()->PostTask( On 2017/01/19 17:44:34, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:47: if (WillWrite()) { On 2017/01/19 17:44:34, DaleCurtis wrote: > early return instead. Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:30: base::SingleThreadTaskRunner* task_runner); On 2017/01/19 17:44:34, DaleCurtis wrote: > scoped_refptr Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:55: }; On 2017/01/19 17:44:34, DaleCurtis wrote: > DISALLOW Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... media/audio/audio_io.h:110: // Enable debug recording. A file type extension is added to |file_name|. On 2017/01/19 22:37:29, DaleCurtis wrote: > On 2017/01/19 at 20:32:39, Henrik Grunell wrote: > > On 2017/01/19 17:44:34, DaleCurtis wrote: > > > Hmm this is an API only AudioManager should use, so I don't think it belongs > on > > > this interface. Instead we might want a new AudioDebugRecording interface > that > > > output streams also implement. Or some way to avoid this API at all. > Potentially > > > by using the AudioOutputResampler to trigger this recording instead (it > > > intercepts the audio data). > > > > How could AudioOutputResampler do this without an API? I guess I'm not > familiar with the interception mechanism and how that can be used. Also, the AOR > isn't always used, right? An AudioOutputDispatcherImpl could be used directly > afaik. > > AODI is only used for fake streams these days, so it would miss some. AM also > uses AOR directly, so no interface API is needed, just some direct methods on > AOR. Oh, I think you mean that recording is done in AOR. (I was thinking AOR somehow would inform the stream.) That's an option of course. The reason I want to do it in the stream is to have bisect point as close to the Chrome border as possible. There's for example re-buffering on Mac in the stream implementation. I'd really like the recording point to be after that and the other code in the stream implementations. Have streams implement another interface sgtm. I'm thinking that those who implement it could call back to AM to register themselves. I could accept doing it AOR; one benefit is obviously a single place for the recording, and it would still give us valuable information. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... File media/audio/audio_manager.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... media/audio/audio_manager.cc:48: // Dummy fucntion for creating debug writer. On 2017/01/20 07:49:09, Max Morin wrote: > function Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... media/audio/audio_manager_base.cc:94: CHECK(!create_audio_file_writer_callback_.Equals( On 2017/01/20 07:49:09, Max Morin wrote: > is it enough with CHECK(create_audio_file_writer_callback_) or No, it can still be "empty". > CHECK(!create_audio_file_writer_callback_.is_null())? There's no is_null() function. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_outpu... File media/audio/audio_output_proxy.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_outpu... media/audio/audio_output_proxy.cc:78: // Must only be called by AudoiManager directly on physical streams. On 2017/01/20 07:49:09, Max Morin wrote: > Audio Done. https://codereview.chromium.org/2582703003/diff/60001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:173: debug_recording_helper_->MaybeWrite(audio_bus_.get()); On 2017/01/20 07:49:09, Max Morin wrote: > Move this below call to PA, to reduce risk of timeouts? Good point. Done.
https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.cc (left): https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.cc:190: DCHECK_EQ(params.bits_per_sample(), kBytesPerSample * 8); Have you fixed the logic to deal with other bytes-per-sample values? Have you added unit tests for that? https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.h:33: explicit AudioDebugFileWriter(const media::AudioParameters& params); protected? (since you have Create... method now). https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:31: debug_writer_ = audio_manager_->CreateAudioFileWriter(params); This is an unfortunate dependency on AudioManager. Creating file writer has nothing to do with audio system. Could you pass a callback in AudioDebugRecordingHelper constructor instead? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:59: weak_factory_.GetWeakPtr(), base::Passed(&audio_bus_copy))); Can you have one pre-created weak pointer and reuse it all over? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:63: return debug_writer_ && debug_writer_->WillWrite(); Add a comment on why it's thread-safe enough? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:27: class AudioDebugRecordingHelper { Switch AudioInputController to using this as well? (Otherwise we have code duplication.) https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:53: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; Add a comment what |task_runner_| is for? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_file_... File media/audio/audio_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_file_... media/audio/audio_file_writer.h:48: base::Callback<std::unique_ptr<AudioFileWriter>(const AudioParameters&)>; This should be base::RepeatingCallback, shouldn't it? (https://cs.chromium.org/chromium/src/docs/callback.md?q=OnceCallback&sq=packa...) https://codereview.chromium.org/2582703003/diff/80001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:66: debug_recording_helper_(base::MakeUnique<AudioDebugRecordingHelper>( Set it to nullptr for non-webrtc-enabled builds?
Defer to your team on the decision, but I think AOR would simplify and provide you all the data you're like to need. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... media/audio/audio_io.h:110: // Enable debug recording. A file type extension is added to |file_name|. On 2017/01/20 at 10:38:56, Henrik Grunell wrote: > On 2017/01/19 22:37:29, DaleCurtis wrote: > > On 2017/01/19 at 20:32:39, Henrik Grunell wrote: > > > On 2017/01/19 17:44:34, DaleCurtis wrote: > > > > Hmm this is an API only AudioManager should use, so I don't think it belongs > > on > > > > this interface. Instead we might want a new AudioDebugRecording interface > > that > > > > output streams also implement. Or some way to avoid this API at all. > > Potentially > > > > by using the AudioOutputResampler to trigger this recording instead (it > > > > intercepts the audio data). > > > > > > How could AudioOutputResampler do this without an API? I guess I'm not > > familiar with the interception mechanism and how that can be used. Also, the AOR > > isn't always used, right? An AudioOutputDispatcherImpl could be used directly > > afaik. > > > > AODI is only used for fake streams these days, so it would miss some. AM also > > uses AOR directly, so no interface API is needed, just some direct methods on > > AOR. > > Oh, I think you mean that recording is done in AOR. (I was thinking AOR somehow would inform the stream.) That's an option of course. The reason I want to do it in the stream is to have bisect point as close to the Chrome border as possible. There's for example re-buffering on Mac in the stream implementation. I'd really like the recording point to be after that and the other code in the stream implementations. > > Have streams implement another interface sgtm. I'm thinking that those who implement it could call back to AM to register themselves. > > I could accept doing it AOR; one benefit is obviously a single place for the recording, and it would still give us valuable information. Yeah, stuff like the rebuffering in OSX wouldn't be caught, but otherwise AOR is the last line before streams, and generally the per-stream stuff should be minor. I think between AOR and the output stream you're very unlikely to get useful information.
Patchset #5 (id:100001) has been deleted
On 2017/01/20 17:52:20, DaleCurtis wrote: > Defer to your team on the decision, but I think AOR would simplify and provide > you all the data you're like to need. > > https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h > File media/audio/audio_io.h (right): > > https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_io.h#... > media/audio/audio_io.h:110: // Enable debug recording. A file type extension is > added to |file_name|. > On 2017/01/20 at 10:38:56, Henrik Grunell wrote: > > On 2017/01/19 22:37:29, DaleCurtis wrote: > > > On 2017/01/19 at 20:32:39, Henrik Grunell wrote: > > > > On 2017/01/19 17:44:34, DaleCurtis wrote: > > > > > Hmm this is an API only AudioManager should use, so I don't think it > belongs > > > on > > > > > this interface. Instead we might want a new AudioDebugRecording > interface > > > that > > > > > output streams also implement. Or some way to avoid this API at all. > > > Potentially > > > > > by using the AudioOutputResampler to trigger this recording instead (it > > > > > intercepts the audio data). > > > > > > > > How could AudioOutputResampler do this without an API? I guess I'm not > > > familiar with the interception mechanism and how that can be used. Also, the > AOR > > > isn't always used, right? An AudioOutputDispatcherImpl could be used > directly > > > afaik. > > > > > > AODI is only used for fake streams these days, so it would miss some. AM > also > > > uses AOR directly, so no interface API is needed, just some direct methods > on > > > AOR. > > > > Oh, I think you mean that recording is done in AOR. (I was thinking AOR > somehow would inform the stream.) That's an option of course. The reason I want > to do it in the stream is to have bisect point as close to the Chrome border as > possible. There's for example re-buffering on Mac in the stream implementation. > I'd really like the recording point to be after that and the other code in the > stream implementations. > > > > Have streams implement another interface sgtm. I'm thinking that those who > implement it could call back to AM to register themselves. > > > > I could accept doing it AOR; one benefit is obviously a single place for the > recording, and it would still give us valuable information. > > Yeah, stuff like the rebuffering in OSX wouldn't be caught, but otherwise AOR is > the last line before streams, and generally the per-stream stuff should be > minor. I think between AOR and the output stream you're very unlikely to get > useful information. Ptal. I decided to change to record in AudioOutputResampler. There could be a few comments from the previous round I didn't have time to fix today, will go over tomorrow. Tests also remain and update CL description.
https://codereview.chromium.org/2582703003/diff/120001/media/audio/audio_outp... File media/audio/audio_output_resampler.h (right): https://codereview.chromium.org/2582703003/diff/120001/media/audio/audio_outp... media/audio/audio_output_resampler.h:51: // Controls debug recording. It seems like this should just be something like SetDebugRecordingCallback() and then the caller to that maintains the complexity for setting up the debug recorders in the right way. It's a bit odd to have AOR/OMDC setting up and managing file names, etc. I realize it's a many-to-one relationship though, so perhaps the callback can take an opaque stream id as well.
Now using callbacks instead. I use register and unregister callbacks apart from the data callback to be able to setup and take down recording, and do that on the right thread. https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/60001/media/audio/audio_manag... media/audio/audio_manager_base.cc:94: CHECK(!create_audio_file_writer_callback_.Equals( On 2017/01/20 10:38:56, Henrik Grunell wrote: > On 2017/01/20 07:49:09, Max Morin wrote: > > is it enough with CHECK(create_audio_file_writer_callback_) or > > No, it can still be "empty". > > > CHECK(!create_audio_file_writer_callback_.is_null())? > > There's no is_null() function. Oh, I missed the base class. Changed. https://codereview.chromium.org/2582703003/diff/140001/media/audio/fake_audio... File media/audio/fake_audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/140001/media/audio/fake_audio... media/audio/fake_audio_manager.h:23: CreateAudioFileWriterCallback create_audio_file_writer_callback); I will have to update loads of unit tests in content/ too due to this change. We could keep the ctor signature and have this class always use a FakeAudioFileWriter. I doubt any user of this class wants else. Wdyt?
Sorry, I find the current version a bit complicated - very difficult to read and to reason about. We need cleaner interfaces, more descriptive naming and good comments on what is going on. https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:27: class AudioDebugRecordingHelper { On 2017/01/20 11:36:26, o1ka wrote: > Switch AudioInputController to using this as well? (Otherwise we have code > duplication.) Ping? https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... media/audio/audio_manager_base.cc:483: void AudioManagerBase::RegisterDebugRecordingSource( If keeping this logic, could you move it into a helper object? AudioManager takes care of too much stuff. https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... media/audio/audio_manager_base.cc:507: // here is safe. This does not looks like a good separation of concerns to me. Could AudioManager create a debug recorder object which takes care of everything and pass it into resampler? https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:372: resampler_callback->SetDebugRecordingCallback(debug_recording_callback_); Sorry I can't process it :) Why can't you still call it EnableDebugRecording? https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:431: register_debug_recording_source_callback.Run(it.second->id(), Way too many callbacks :( the code is unreadable. I find the previous version much cleaner. To separate debug recording logic you can have a helper object, for example.
Awesome - I had the exact same concerns yesterday evening at home in retrospect. Move out code from AudioManagerBase and change the many callback to an interface. So I'll do that. olka@ and I discussed offline and I'll also move ownership of the helper to the converter. I've addressed some older comments but not uploaded those yet. Three old comments are yet to be done, I have a note of those. https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.cc (left): https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.cc:190: DCHECK_EQ(params.bits_per_sample(), kBytesPerSample * 8); On 2017/01/20 11:36:25, o1ka wrote: > Have you fixed the logic to deal with other bytes-per-sample values? Have you > added unit tests for that? It will convert to signed int 16 in Write(). I'll update unit test. https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_debug_file_writer.h:33: explicit AudioDebugFileWriter(const media::AudioParameters& params); On 2017/01/20 11:36:26, o1ka wrote: > protected? (since you have Create... method now). Hmm, that doesn't work since MakeUnique can't create then. Does anyone know the recommended way? Use WrapUnique instead? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:31: debug_writer_ = audio_manager_->CreateAudioFileWriter(params); On 2017/01/20 11:36:26, o1ka wrote: > This is an unfortunate dependency on AudioManager. Creating file writer has > nothing to do with audio system. Could you pass a callback in > AudioDebugRecordingHelper constructor instead? Done. https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:59: weak_factory_.GetWeakPtr(), base::Passed(&audio_bus_copy))); On 2017/01/20 11:36:26, o1ka wrote: > Can you have one pre-created weak pointer and reuse it all over? I haven't seen it used that way, have you? Doesn't Bind take ownership and release the reference? https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.cc:63: return debug_writer_ && debug_writer_->WillWrite(); On 2017/01/20 11:36:26, o1ka wrote: > Add a comment on why it's thread-safe enough? Done. Both here and in header. https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:27: class AudioDebugRecordingHelper { On 2017/01/20 11:36:26, o1ka wrote: > Switch AudioInputController to using this as well? (Otherwise we have code > duplication.) Good point. I'll do that. https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_debug... media/audio/audio_debug_recording_helper.h:53: scoped_refptr<base::SingleThreadTaskRunner> task_runner_; On 2017/01/20 11:36:26, o1ka wrote: > Add a comment what |task_runner_| is for? Done. https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_file_... File media/audio/audio_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/audio_file_... media/audio/audio_file_writer.h:48: base::Callback<std::unique_ptr<AudioFileWriter>(const AudioParameters&)>; On 2017/01/20 11:36:26, o1ka wrote: > This should be base::RepeatingCallback, shouldn't it? > (https://cs.chromium.org/chromium/src/docs/callback.md?q=OnceCallback&sq=packa...) Callbacks will be replaced by an object. https://codereview.chromium.org/2582703003/diff/80001/media/audio/pulse/pulse... File media/audio/pulse/pulse_output.cc (right): https://codereview.chromium.org/2582703003/diff/80001/media/audio/pulse/pulse... media/audio/pulse/pulse_output.cc:66: debug_recording_helper_(base::MakeUnique<AudioDebugRecordingHelper>( On 2017/01/20 11:36:26, o1ka wrote: > Set it to nullptr for non-webrtc-enabled builds? Good point. That should be the right things to do. Right now, recording can only be enabled in WebRTC-enabled builds. Will do that. https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... media/audio/audio_manager_base.cc:483: void AudioManagerBase::RegisterDebugRecordingSource( On 2017/01/25 17:47:00, o1ka wrote: > If keeping this logic, could you move it into a helper object? AudioManager > takes care of too much stuff. Yep, agree. https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_mana... media/audio/audio_manager_base.cc:507: // here is safe. On 2017/01/25 17:47:00, o1ka wrote: > This does not looks like a good separation of concerns to me. Could AudioManager > create a debug recorder object which takes care of everything and pass it into > resampler? Yes, will do. https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:372: resampler_callback->SetDebugRecordingCallback(debug_recording_callback_); On 2017/01/25 17:47:00, o1ka wrote: > Sorry I can't process it :) Why can't you still call it EnableDebugRecording? This only sets the callback to forward the data. The recording may or may not be enabled. https://codereview.chromium.org/2582703003/diff/140001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:431: register_debug_recording_source_callback.Run(it.second->id(), On 2017/01/25 17:47:00, o1ka wrote: > Way too many callbacks :( the code is unreadable. > > I find the previous version much cleaner. To separate debug recording logic you > can have a helper object, for example. Yes, exactly what I thought myself later yesterday.
Dale: ptal. I've reworked the callbacks and interfaces. There's only a register callback now, passed in the AudioOutputResampler ctor. The code managing the AudioDebugRecordingHelpers is moved to a new class, AudioDebugRecordingManager. When registering a new source (i.e. OnMoreDataConverter) with the AudioDebugRecordingManager, an AudioRecorder (see below) object is returned, wrapped in a struct. The struct handles automatic unregistration when deleted. OMDC owns the wrapper. Olga proposed the automatic unregistration and I like it but the way I did it adds a bunch of new types which clutters a bit. I'm not all happy with it and would like fresh eyes and input on that. (I put some comments in the code.) AudioRecorder is an interface implemented by AudioDebugRecordingHelper and only contains one function - OnData(). OnData() is called by OMDC to provide data.
With risk of sounding like Olga: I think a diagram would help (displaying relations between classes) :). https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:31: using UnregisterAudioDebugRecorderCallback = base::Callback<void()>; Should probably be a OnceCallback (but some technical limitations might make it impossible right now). https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:62: using AudioDebugRecorderWrapperUniquePtr = I'm reading this on the train so I may be completely off, but both wrapper and uniqueptr seems excessive, the unique ptr with deleter is itself a wrapper. Maybe all functionality can be in the deleter and this can just be std::unique_ptr<AudioDebugRecorder, AudioDebugRecorderDeleter> and a factory function somewhere that puts the callback in the deleter? Actually, can't it just do it in the destructor? https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:98: // Unregisters a source. Stops recording on it's recorder. its
Overall looks reasonable aside from the new constructor argument. I leave the specifics of the writer review to olka@ since I'm not familiar with that code. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... File media/audio/audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... media/audio/audio_manager.h:68: CreateAudioFileWriterCallback create_audio_file_writer_callback); Do you really need to inject this here? This causes complexity for all AudioManager implementations for something that can be handled in the base implementation.
I could write a diagram, that's probably useful. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... File media/audio/audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... media/audio/audio_manager.h:68: CreateAudioFileWriterCallback create_audio_file_writer_callback); On 2017/01/26 17:53:18, DaleCurtis wrote: > Do you really need to inject this here? This causes complexity for all > AudioManager implementations for something that can be handled in the base > implementation. I know, it's annoying. The content layer passes the callback, that's why. And the reason for writer implementation not being in media is that it handles file writing afaik. Any suggestions to avoid this?
https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... File media/audio/audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... media/audio/audio_manager.h:68: CreateAudioFileWriterCallback create_audio_file_writer_callback); On 2017/01/26 at 19:56:22, Henrik Grunell wrote: > On 2017/01/26 17:53:18, DaleCurtis wrote: > > Do you really need to inject this here? This causes complexity for all > > AudioManager implementations for something that can be handled in the base > > implementation. > > I know, it's annoying. The content layer passes the callback, that's why. And the reason for writer implementation not being in media is that it handles file writing afaik. Any suggestions to avoid this? Just make the caller of Enable() deal with it? That code is already in content. Doesn't seem to be any reason it couldn't be passed in every time.
https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:94: it->second->DisableDebugRecording(); The code implies that the helper is still alive. Does it really? There is no guarantee. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:62: using AudioDebugRecorderWrapperUniquePtr = On 2017/01/26 17:49:43, Max Morin wrote: > I'm reading this on the train so I may be completely off, but both wrapper and > uniqueptr seems excessive, the unique ptr with deleter is itself a wrapper. > Maybe all functionality can be in the deleter and this can just be > std::unique_ptr<AudioDebugRecorder, AudioDebugRecorderDeleter> and a factory > function somewhere that puts the callback in the deleter? > > Actually, can't it just do it in the destructor? Agree. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... File media/audio/audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... media/audio/audio_manager.h:68: CreateAudioFileWriterCallback create_audio_file_writer_callback); On 2017/01/26 20:00:13, DaleCurtis wrote: > On 2017/01/26 at 19:56:22, Henrik Grunell wrote: > > On 2017/01/26 17:53:18, DaleCurtis wrote: > > > Do you really need to inject this here? This causes complexity for all > > > AudioManager implementations for something that can be handled in the base > > > implementation. > > > > I know, it's annoying. The content layer passes the callback, that's why. And > the reason for writer implementation not being in media is that it handles file > writing afaik. Any suggestions to avoid this? > > Just make the caller of Enable() deal with it? That code is already in content. > Doesn't seem to be any reason it couldn't be passed in every time. This is a pretty neat idea.
New patch set. * Removed the argument in AudioManager ctor. Initialize function instead - see comment. * Removed the recorder wrapper. * Other code review fixes. I've waited with tests until design and implementation is settled. I left the fake writer around although it's not used in this patch set, in case some test would make use of it in coming patch sets. No need to review it now. I'll remove it later if it's not used. To do is also use the helper in AudioInputController. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:94: it->second->DisableDebugRecording(); On 2017/01/27 09:25:26, o1ka wrote: > The code implies that the helper is still alive. Does it really? There is no > guarantee. I removed this, it's not necessary since unregistration is done in conjunction deletion time. (Good point that this may not be safe.) https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:31: using UnregisterAudioDebugRecorderCallback = base::Callback<void()>; On 2017/01/26 17:49:42, Max Morin wrote: > Should probably be a OnceCallback (but some technical limitations might make it > impossible right now). Good point. I'm not sure actually about limitations. It's "under development", but still new code should use Once and Repeatable versions according to documentation. I'm going to do that. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:62: using AudioDebugRecorderWrapperUniquePtr = On 2017/01/27 09:25:26, o1ka wrote: > On 2017/01/26 17:49:43, Max Morin wrote: > > I'm reading this on the train so I may be completely off, but both wrapper and > > uniqueptr seems excessive, the unique ptr with deleter is itself a wrapper. > > Maybe all functionality can be in the deleter and this can just be > > std::unique_ptr<AudioDebugRecorder, AudioDebugRecorderDeleter> and a factory > > function somewhere that puts the callback in the deleter? > > > > Actually, can't it just do it in the destructor? > > Agree. > Removed the wrapper. The helper takes an on destruction closure now. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:98: // Unregisters a source. Stops recording on it's recorder. On 2017/01/26 17:49:43, Max Morin wrote: > its Removed that sentence. https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... File media/audio/audio_manager.h (right): https://codereview.chromium.org/2582703003/diff/160001/media/audio/audio_mana... media/audio/audio_manager.h:68: CreateAudioFileWriterCallback create_audio_file_writer_callback); On 2017/01/27 09:25:26, o1ka wrote: > On 2017/01/26 20:00:13, DaleCurtis wrote: > > On 2017/01/26 at 19:56:22, Henrik Grunell wrote: > > > On 2017/01/26 17:53:18, DaleCurtis wrote: > > > > Do you really need to inject this here? This causes complexity for all > > > > AudioManager implementations for something that can be handled in the base > > > > implementation. > > > > > > I know, it's annoying. The content layer passes the callback, that's why. > And > > the reason for writer implementation not being in media is that it handles > file > > writing afaik. Any suggestions to avoid this? > > > > Just make the caller of Enable() deal with it? That code is already in > content. > > Doesn't seem to be any reason it couldn't be passed in every time. > > This is a pretty neat idea. You're right, that would work since the create callback is not used until enabling. However, it's also enabled from chrome/ where the create function is not available. Instead I added an init function that sets the callback and call this after creating.
api surface lgtm, defer to olka for formal review.
https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:112: media::AudioManager* audio_manager = media::AudioManager::Get(); The AudioManager::Get() pointer is nulled on the UI thread, before the AudioManager is actually deleted. Are you sure you can't get null here? (The lifetime of this object isn't obvious to me). https://codereview.chromium.org/2582703003/diff/180001/content/browser/webrtc... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2582703003/diff/180001/content/browser/webrtc... content/browser/webrtc/webrtc_internals.cc:337: media::AudioManager* audio_manager = media::AudioManager::Get(); ditto
Olga, please review also. https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:112: media::AudioManager* audio_manager = media::AudioManager::Get(); On 2017/01/30 10:24:01, Max Morin wrote: > The AudioManager::Get() pointer is nulled on the UI thread, before the > AudioManager is actually deleted. Are you sure you can't get null here? (The > lifetime of this object isn't obvious to me). I'll double check that tomorrow.
https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:111: // this object, so it's safe to post unretained. Add why media::AudioManager::Get() is safe (or better use a pointer instead)? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:39: // The debug writer writes in wave format. How do we know it here? It's just some audio file writer. The extension should be set externally by someone who knows what file writer it is. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:53: // AudioFileWriter::WillWrite() can be called on any thread. Please explain why this method is ok to call from any thread, though it's not thread-safe. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:58: if (!WillWrite()) Why is it safe to call it here? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:62: // AudioBuses. See also AudioInputController. What exactly in AudioInputController? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.h:55: bool WillWrite(); Private? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:66: base::Unretained(this), id)); Please specify why Unretained() is safe. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:68: if (!debug_recording_base_file_name_.empty()) { Then you need to check it in Enable/Disbale() as well https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.cc:292: base::Unretained(output_debug_recording_manager_.get()))); Check if |output_debug_recording_manager_| is initialized. otherwise pass a no-op callback? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.cc:451: output_debug_recording_manager_->EnableDebugRecording(base_file_name); Check if |output_debug_recording_manager_| is initialized https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.h:203: std::unique_ptr<AudioDebugRecordingManager> output_debug_recording_manager_; Why calling it "output"? Won't it work for inputs just as well? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:427: // register again at start. Seems like converters are re-used. Please figure this out. Now we register recording on each start, but never unregister it. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:480: if (debug_recorder_) If you are going to unregistered in Stop, there may be a race here.
Patchset #9 (id:200001) has been deleted
* Wrote two unit tests. * Updated and improved one browser test. * Code review fixes. * Some other fixes. * Rebased. I've started on a diagram, and will add that when finished. If you prefer to have it before reviewing again, it's fine with me to wait. Also, I think since there can be a quite a streams now written we should fix the heap allocation at every data callback. Probably better to do in a separate CL, before or after this. Wdyt? https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... File chrome/browser/media/webrtc/audio_debug_recordings_handler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:111: // this object, so it's safe to post unretained. On 2017/01/31 11:00:11, o1ka (CET) wrote: > Add why media::AudioManager::Get() is safe (or better use a pointer instead)? Changed to a pointer injected. https://codereview.chromium.org/2582703003/diff/180001/chrome/browser/media/w... chrome/browser/media/webrtc/audio_debug_recordings_handler.cc:112: media::AudioManager* audio_manager = media::AudioManager::Get(); On 2017/01/30 14:11:26, Henrik Grunell wrote: > On 2017/01/30 10:24:01, Max Morin wrote: > > The AudioManager::Get() pointer is nulled on the UI thread, before the > > AudioManager is actually deleted. Are you sure you can't get null here? (The > > lifetime of this object isn't obvious to me). > > I'll double check that tomorrow. This class lives on the UI thread, which is implicit. See the PostDelayedTask below - it posts on the UI thread to run DoStopAudioDebugRecordings(), which uses the class wide thread checker. I've changed to check the UI thread explicitly in all functions instead which makes that clearer. After doing that, I also changed to inject a pointer instead based on Olga's comment above. https://codereview.chromium.org/2582703003/diff/180001/content/browser/webrtc... File content/browser/webrtc/webrtc_internals.cc (right): https://codereview.chromium.org/2582703003/diff/180001/content/browser/webrtc... content/browser/webrtc/webrtc_internals.cc:337: media::AudioManager* audio_manager = media::AudioManager::Get(); On 2017/01/30 10:24:01, Max Morin wrote: > ditto And same here. Accessing on UI thread is safe. Updated the comments in this class. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:39: // The debug writer writes in wave format. On 2017/01/31 11:00:11, o1ka (CET) wrote: > How do we know it here? It's just some audio file writer. The extension should > be set externally by someone who knows what file writer it is. Agree. Added AudioFileWriter::GetExtension(), and implementation of it in AudioDebugFileWriter. Currently, it's only the writer that knows about that. I also updated the documentaion of AudioDebugFileWriter to say that it records a 16 bit PCM WAVE file. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:53: // AudioFileWriter::WillWrite() can be called on any thread. On 2017/01/31 11:00:11, o1ka (CET) wrote: > Please explain why this method is ok to call from any thread, though it's not > thread-safe. Removed this function. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:58: if (!WillWrite()) On 2017/01/31 11:00:11, o1ka (CET) wrote: > Why is it safe to call it here? WillWrite() has been removed, and I just check |debug_writer_| here instead because WillWrite() is unecessary anyway. (It would have been safe but even safer this way for the future.) Added comment about this. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:62: // AudioBuses. See also AudioInputController. On 2017/01/31 11:00:11, o1ka (CET) wrote: > What exactly in AudioInputController? Updated comment. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.h (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.h:55: bool WillWrite(); On 2017/01/31 11:00:11, o1ka (CET) wrote: > Private? I removed it. It's only used in one place internally. (Yes, it should have been private if kept). https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:66: base::Unretained(this), id)); On 2017/01/31 11:00:11, o1ka (CET) wrote: > Please specify why Unretained() is safe. Changed it to use a weak pointer. Unretained() was safe since the manager outlived the resampler, but to not require this I changed to weak ptr. Added comment. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:68: if (!debug_recording_base_file_name_.empty()) { On 2017/01/31 11:00:11, o1ka (CET) wrote: > Then you need to check it in Enable/Disbale() as well I'm not sure what you mean. Enable() takes the file name as an argument, I added a DCHECK there which makes sense. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.cc:292: base::Unretained(output_debug_recording_manager_.get()))); On 2017/01/31 11:00:11, o1ka (CET) wrote: > Check if |output_debug_recording_manager_| is initialized. otherwise pass a > no-op callback? Yes, done. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.cc:451: output_debug_recording_manager_->EnableDebugRecording(base_file_name); On 2017/01/31 11:00:11, o1ka (CET) wrote: > Check if |output_debug_recording_manager_| is initialized Enable() should not be called if Initialize() hasn't been called. Thus it should be a DCHECK, which is redundant since it'll crash on deref. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_mana... media/audio/audio_manager_base.h:203: std::unique_ptr<AudioDebugRecordingManager> output_debug_recording_manager_; On 2017/01/31 11:00:11, o1ka (CET) wrote: > Why calling it "output"? Won't it work for inputs just as well? Good point. The "output" filename extension is given in the constructor, global for the recording manager, but I changed it to be given per stream instead. In the future, apart from input we'll likely also want output recording at another point with another extension. One effect of this, just to be aware of, is that the unique running ID will be global for all these streams. I think that's fine. So, after that change, I also changed this variable. However, I think it makes sense to keep the naming of functions above since it currently only controls output. https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:427: // register again at start. Seems like converters are re-used. On 2017/01/31 11:00:11, o1ka (CET) wrote: > Please figure this out. Now we register recording on each start, but never > unregister it. I've looked into this now. We should unregister when closing stream as done now (when the converter is destroyed) because Start() and Stop() is used to pause and restart a stream. I think the semantics makes sense too; a stream is connected to a recorder. As long as a stream is alive, the data going through it is fed to the recorder. Wdyt? https://codereview.chromium.org/2582703003/diff/180001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:480: if (debug_recorder_) On 2017/01/31 11:00:11, o1ka (CET) wrote: > If you are going to unregistered in Stop, there may be a race here. See above comment - won't unregister in Stop().
lgtm with comments addressed. https://codereview.chromium.org/2582703003/diff/220001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/browse... content/browser/browser_main_loop.cc:1809: base::BindRepeating(&AudioDebugFileWriter::Create))); Just use base::MakeUnique<media::AudioFileWriter>, then you can get rid of the ADFW::Create method. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:59: // whether started or not, it's not needed either. Could you introduce an atomic flag instead?
Description was changed from ========== Audio output debug recording. BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Updated CL description and added link to class diagram.
Good job with updating the tests! https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.cc:315: std::string AudioDebugFileWriter::GetExtension() { const std::string& maybe? Do you anticipate any usecase what const ref wouldn't be enough? https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.h:46: std::string GetExtension() override; GetFileNameExtension() would be more descriptive. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:59: // whether started or not, it's not needed either. On 2017/02/09 09:47:26, Max Morin wrote: > Could you introduce an atomic flag instead? This is good enough, isn't it? What would we gain having a flag? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:85: &AudioDebugRecordingManager::UnregisterDebugRecordingSource, Is it necessary to pass a pointer to AudioDebugRecordingManager::UnregisterDebugRecordingSource into CreateAudioDebugRecordingHelper? It is known there, isn't it? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:88: if (!debug_recording_base_file_name_.empty()) { This needs a comment (or better a helper function IsDebugRecordingEnabled()) https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:37: class MEDIA_EXPORT AudioDebugRecordingManager { Can we have a description or pseudo-graphic of the whole debug recording infrastructure and objects ownership here? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_file... File media/audio/audio_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_file... media/audio/audio_file_writer.h:53: base::RepeatingCallback<std::unique_ptr<AudioFileWriter>( Declare in within AudioFileWriter scope and call it |CreateCallback| ? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:300: register_debug_recording_source_callback); just (debug_recording_manager_) ? base::BindRepeating( &AudioDebugRecordingManager::RegisterDebugRecordingSource, base::Unretained(debug_recording_manager_.get()), "output") : ... ? it can also be a helper static of AudioDebugRecordingManager. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: debug_recording_manager_->EnableDebugRecording(base_file_name); if (debug_recording_manager_)? Looks like unit test is needed. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.h:79: void DisableOutputDebugRecording() override; All three "final"? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:362: std::move(debug_recorder)); Just OnMoreDataConverter(params_, output_params_, base::WrapUnique<AudioDebugRecorder>(register_debug_recording_source_callback_ ? register_debug_recording_source_callback_.Run(...) : nullptr)); ? To simplify it further, we can make |register_debug_recording_source_callback_| to always be non-empty and just return null if no debug recording.
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
Code review fixes. Added test case for new functions in AudioManager. https://codereview.chromium.org/2582703003/diff/220001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/browse... content/browser/browser_main_loop.cc:1809: base::BindRepeating(&AudioDebugFileWriter::Create))); On 2017/02/09 09:47:26, Max Morin wrote: > Just use base::MakeUnique<media::AudioFileWriter>, then you can get rid of the > ADFW::Create method. It would need to be base::MakeUnique<media::AudioDebugFileWriter> (i.e. the implementation), and that conversion makes Bind() cry. Afaik, it can't be done. https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.cc (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.cc:315: std::string AudioDebugFileWriter::GetExtension() { On 2017/02/09 13:04:02, o1ka (CET) wrote: > const std::string& maybe? Do you anticipate any usecase what const ref wouldn't > be enough? That would be fine, but the constant string would need to be stored as an std::string member too, which is unnecessary. I changed it to const char* though (same as for e.g. AudioManager::GetName()). We may not get rid of the extra copy; I'm not familiar with details of BasicStringPieceType which is the argument of FilePath::AddExtension(). I think that's anyway better than increasing the memory usage. https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/220001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.h:46: std::string GetExtension() override; On 2017/02/09 13:04:02, o1ka (CET) wrote: > GetFileNameExtension() would be more descriptive. Agree, changed. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:59: // whether started or not, it's not needed either. On 2017/02/09 13:04:02, o1ka (CET) wrote: > On 2017/02/09 09:47:26, Max Morin wrote: > > Could you introduce an atomic flag instead? > This is good enough, isn't it? What would we gain having a flag? After discussing offline, I decided to use an atomic flag. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:85: &AudioDebugRecordingManager::UnregisterDebugRecordingSource, On 2017/02/09 13:04:03, o1ka (CET) wrote: > Is it necessary to pass a pointer to > AudioDebugRecordingManager::UnregisterDebugRecordingSource into > CreateAudioDebugRecordingHelper? It is known there, isn't it? The helper doesn't know about the manager. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:88: if (!debug_recording_base_file_name_.empty()) { On 2017/02/09 13:04:03, o1ka (CET) wrote: > This needs a comment (or better a helper function IsDebugRecordingEnabled()) Yep, a helper is better. Done. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:37: class MEDIA_EXPORT AudioDebugRecordingManager { On 2017/02/09 13:04:03, o1ka (CET) wrote: > Can we have a description or pseudo-graphic of the whole debug recording > infrastructure and objects ownership here? That's a good idea. I created a diagram doc to assist the CL description, but it's of course best to have maintainable documentation close to the source code. I wrote up a simplified diagram. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_file... File media/audio/audio_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_file... media/audio/audio_file_writer.h:53: base::RepeatingCallback<std::unique_ptr<AudioFileWriter>( On 2017/02/09 13:04:03, o1ka (CET) wrote: > Declare in within AudioFileWriter scope and call it |CreateCallback| ? Sounds good, done. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:300: register_debug_recording_source_callback); On 2017/02/09 13:04:03, o1ka (CET) wrote: > just (debug_recording_manager_) ? base::BindRepeating( > &AudioDebugRecordingManager::RegisterDebugRecordingSource, > base::Unretained(debug_recording_manager_.get()), "output") : ... ? Done. > it can also be a helper static of AudioDebugRecordingManager. Not sure how you mean. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: debug_recording_manager_->EnableDebugRecording(base_file_name); On 2017/02/09 13:04:03, o1ka (CET) wrote: > if (debug_recording_manager_)? > Looks like unit test is needed. I wrote answer in a previous comment answer. :) The intention is that InitializeOutputDebugRecording() must be called before Enable(). I.e. crashing is intentional. Anyway, added unit test case for these functions. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.h (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.h:79: void DisableOutputDebugRecording() override; On 2017/02/09 13:04:03, o1ka (CET) wrote: > All three "final"? Good point, should be final. Done. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:362: std::move(debug_recorder)); On 2017/02/09 13:04:03, o1ka (CET) wrote: > Just > OnMoreDataConverter(params_, output_params_, > base::WrapUnique<AudioDebugRecorder>(register_debug_recording_source_callback_ ? > register_debug_recording_source_callback_.Run(...) : nullptr)); ? > > To simplify it further, we can make |register_debug_recording_source_callback_| > to always be non-empty and just return null if no debug recording. Sounds good, changed to always running the callback.
And thanks for the thorough review!
lgtm with comments addressed. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:85: &AudioDebugRecordingManager::UnregisterDebugRecordingSource, On 2017/02/10 09:00:56, Henrik Grunell wrote: > On 2017/02/09 13:04:03, o1ka (CET) wrote: > > Is it necessary to pass a pointer to > > AudioDebugRecordingManager::UnregisterDebugRecordingSource into > > CreateAudioDebugRecordingHelper? It is known there, isn't it? > > The helper doesn't know about the manager. CreateAudioDebugRecordingHelper does. Why to pass UnregisterDebugRecordingSource into it? https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: debug_recording_manager_->EnableDebugRecording(base_file_name); On 2017/02/10 09:00:56, Henrik Grunell wrote: > On 2017/02/09 13:04:03, o1ka (CET) wrote: > > if (debug_recording_manager_)? > > Looks like unit test is needed. > > I wrote answer in a previous comment answer. :) The intention is that > InitializeOutputDebugRecording() must be called before Enable(). I.e. crashing > is intentional. > > Anyway, added unit test case for these functions. This intention is not obvious at all. DCHECK(debug_recording_manager_) << "InitializeOutputDebugRecording() must be called before Enable()" ? https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:43: base::subtle::Release_Store(&recording_enabled_, 1); Let's do NoBarrier everywhere as per offline discussion, and please add a comment explaining why we are doing ti this way. https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:53: // This is awesome. Could you also indicate which of owned instances are null when recording is disabled? https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:253: : base::BindRepeating(&GetNullptrAudioDebugRecorder)), Pass this into constructor instead? Why to deffer the decision to AudioOutputResampler, when everything is know before its creation?
grunell@chromium.org changed reviewers: + guidou@chromium.org, pfeldman@chromium.org
Adding owner reviewers: guidou@: content/browser/webrtc/* (olka@ has lgtm'd these so feel free to rubber stamp) pfeldman@: content/browser/browser_main_loop.cc
Fixed olka@'s final comments. (Feel free to read the added code comments.) https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.cc:85: &AudioDebugRecordingManager::UnregisterDebugRecordingSource, On 2017/02/10 15:21:53, o1ka (CET) wrote: > On 2017/02/10 09:00:56, Henrik Grunell wrote: > > On 2017/02/09 13:04:03, o1ka (CET) wrote: > > > Is it necessary to pass a pointer to > > > AudioDebugRecordingManager::UnregisterDebugRecordingSource into > > > CreateAudioDebugRecordingHelper? It is known there, isn't it? > > > > The helper doesn't know about the manager. > > CreateAudioDebugRecordingHelper does. Why to pass UnregisterDebugRecordingSource > into it? Oh sorry, I misread, I thought you meant AudioDebugRecordingHelper. Yes it's known in CreateAudioDebugRecordingHelper. The reason for doing it this way is that CreateAudioDebugRecordingHelper is overridden in the unit test to create a mock instead and it should be as lightweight as possible. Otherwise the test class' function needs to re-implement code (binding the right function) that we actually want to test. https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/220001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: debug_recording_manager_->EnableDebugRecording(base_file_name); On 2017/02/10 15:21:53, o1ka (CET) wrote: > On 2017/02/10 09:00:56, Henrik Grunell wrote: > > On 2017/02/09 13:04:03, o1ka (CET) wrote: > > > if (debug_recording_manager_)? > > > Looks like unit test is needed. > > > > I wrote answer in a previous comment answer. :) The intention is that > > InitializeOutputDebugRecording() must be called before Enable(). I.e. crashing > > is intentional. > > > > Anyway, added unit test case for these functions. > > This intention is not obvious at all. > DCHECK(debug_recording_manager_) << "InitializeOutputDebugRecording() must be > called before Enable()" > ? Yeah, it's only in the header comment in AudioManager (fixed a missing word there now). Added the dcheck here. https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... File media/audio/audio_debug_recording_helper.cc (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... media/audio/audio_debug_recording_helper.cc:43: base::subtle::Release_Store(&recording_enabled_, 1); On 2017/02/10 15:21:53, o1ka wrote: > Let's do NoBarrier everywhere as per offline discussion, and please add a > comment explaining why we are doing ti this way. Done. https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... File media/audio/audio_debug_recording_manager.h (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_debu... media/audio/audio_debug_recording_manager.h:53: // On 2017/02/10 15:21:53, o1ka (CET) wrote: > This is awesome. Could you also indicate which of owned instances are null when > recording is disabled? Thanks. Sounds good, done. It involves what content/ is doing, but since it's the main user I think that's fine. I added a reference to the content code. https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_outp... File media/audio/audio_output_resampler.cc (right): https://codereview.chromium.org/2582703003/diff/240001/media/audio/audio_outp... media/audio/audio_output_resampler.cc:253: : base::BindRepeating(&GetNullptrAudioDebugRecorder)), On 2017/02/10 15:21:53, o1ka (CET) wrote: > Pass this into constructor instead? Why to deffer the decision to > AudioOutputResampler, when everything is know before its creation? Absolutely, done.
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
content/browser/renderer_host/media/ and content/browser/webrtc lgtm % comment.
https://codereview.chromium.org/2582703003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.h:33: explicit AudioDebugFileWriter(const media::AudioParameters& params); Should this constructor become private?
https://codereview.chromium.org/2582703003/diff/280001/content/browser/render... File content/browser/renderer_host/media/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/280001/content/browser/render... content/browser/renderer_host/media/audio_debug_file_writer.h:33: explicit AudioDebugFileWriter(const media::AudioParameters& params); On 2017/02/14 10:03:42, Guido Urdaneta wrote: > Should this constructor become private? The Create() function's purpose is to be passed as a callback, and it returns an AudioFileWriter intentionally so it's doesn't quite replace the constructor. There's no particular reason for hiding the ctor either.
I changed AudioManager::GetFileNameExtension() to return const base::FilePath::CharType* instead of char* so that it plays well with FilePath::AddExtension() which it will mainly be used with.
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Patchset #15 (id:340001) has been deleted
The CQ bit was checked by grunell@chromium.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.chromium.or...
On 2017/02/14 11:31:53, Henrik Grunell wrote: > I changed AudioManager::GetFileNameExtension() to return const > base::FilePath::CharType* instead of char* so that it plays well with > FilePath::AddExtension() which it will mainly be used with. Olga: I've made a bunch of compile fixes since patch set 10, mainly string type for FilePath. I think it would be good if you reviewed the diff from that patch set - that should be quick.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... content/browser/browser_main_loop.cc:1802: audio_manager_->GetTaskRunner()->PostTask( Why is this here, but not in the media::AudioManager::Create?
https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... content/browser/browser_main_loop.cc:1802: audio_manager_->GetTaskRunner()->PostTask( On 2017/02/14 19:44:28, pfeldman wrote: > Why is this here, but not in the media::AudioManager::Create? That's a good point. There was a discussion earlier in the code review to avoid passing the callback to the ctor, but it should be possible to move this post in AM::Create(). I don't see any problem doing that. Dale: wdyt about that? It would add one parameter to AM::Create (only).
https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... content/browser/browser_main_loop.cc:1802: audio_manager_->GetTaskRunner()->PostTask( On 2017/02/14 at 20:22:17, Henrik Grunell wrote: > On 2017/02/14 19:44:28, pfeldman wrote: > > Why is this here, but not in the media::AudioManager::Create? > > That's a good point. There was a discussion earlier in the code review to avoid passing the callback to the ctor, but it should be possible to move this post in AM::Create(). I don't see any problem doing that. > > Dale: wdyt about that? It would add one parameter to AM::Create (only). Don't want this added as a parameter; instead I'd argue that InitializeOutputDebugRecording() should PostTask if on the wrong thread.
The CQ bit was checked by grunell@chromium.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.chromium.or...
Addressed comment, made a fix, and rebased. Olga: ptal now too. https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/2582703003/diff/320001/content/browser/browse... content/browser/browser_main_loop.cc:1802: audio_manager_->GetTaskRunner()->PostTask( On 2017/02/14 20:30:29, DaleCurtis wrote: > On 2017/02/14 at 20:22:17, Henrik Grunell wrote: > > On 2017/02/14 19:44:28, pfeldman wrote: > > > Why is this here, but not in the media::AudioManager::Create? > > > > That's a good point. There was a discussion earlier in the code review to > avoid passing the callback to the ctor, but it should be possible to move this > post in AM::Create(). I don't see any problem doing that. > > > > Dale: wdyt about that? It would add one parameter to AM::Create (only). > > Don't want this added as a parameter; instead I'd argue that > InitializeOutputDebugRecording() should PostTask if on the wrong thread. That sounds like a reasonable option, I've changed to doing that. pfeldman@ ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2582703003/diff/360001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/360001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: base::Unretained(this), create_audio_file_writer_callback)); move the callback?
I'm still uncomfortable with your making content/browser/browser_main_loop.cc aware of media's internal business. What is happening under that macro seems entirely within the media's scope, so it is unclear to me as a reader, why main_loop needs to get involved beyond creating the object.
Patchset #16 (id:380001) has been deleted
On 2017/02/15 22:29:21, pfeldman wrote: > I'm still uncomfortable with your making content/browser/browser_main_loop.cc > aware of media's internal business. What is happening under that macro seems > entirely within the media's scope, so it is unclear to me as a reader, why > main_loop needs to get involved beyond creating the object. I wrote the alternative, calling initialize in AudioManager::Create(). Patch set 16. I'll let you two settle on which option to go for. (15 or 16)
https://codereview.chromium.org/2582703003/diff/360001/media/audio/audio_mana... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2582703003/diff/360001/media/audio/audio_mana... media/audio/audio_manager_base.cc:458: base::Unretained(this), create_audio_file_writer_callback)); On 2017/02/15 14:13:12, o1ka wrote: > move the callback? Done.
> I wrote the alternative, calling initialize in AudioManager::Create(). Patch set > 16. > > I'll let you two settle on which option to go for. (15 or 16) But why are you still passing parameter? If you leave audio business to audio manager, you would not even need content/ owner review!
On 2017/02/16 21:30:03, pfeldman wrote: > > I wrote the alternative, calling initialize in AudioManager::Create(). Patch > set > > 16. > > > > I'll let you two settle on which option to go for. (15 or 16) > > But why are you still passing parameter? If you leave audio business to audio > manager, you would not even need content/ owner review! This is because the file writer implementation is in content/ (content/browser/renderer_host/media/audio_debug_file_writer.h) and we need some connection there. media/ runs the injected callback to get a file writer created in content/. This is previously done similarly for the audio input side, but a file writer is injected instead, and it's not handled by the AudioManager as for output in this CL. https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?ty... I think the (sole) reason for having the implementation in content/ is that it does thread hops to the FILE thread. It seems like the FILE task runner could be handed to media/ and the implementation can be moved there. I see there are a few places in media/ file access is currently done. (Or would anything be violated doing like so, Dale?)
If you can pass the file_task_runner in that's probably fine. I thought you needed to be in content for more than just the ::FILE thread though.
On 2017/02/17 18:10:24, DaleCurtis wrote: > If you can pass the file_task_runner in that's probably fine. I thought you > needed to be in content for more than just the ::FILE thread though. I thought so too, but I can't find any other reason actually. Wrote a CL that moves it to media/ and updates the code that deals with the existing input recording: https://codereview.chromium.org/2702323002/
Patchset #17 (id:420001) has been deleted
AudioDebugFileWriter has been moved from content/ to media/ in another CL. I have rebased and updated this CL. (Patch set 17 is the rebase only kept for reference.) Ptal: Olga: overall Dale: media/ interfaces. Pavel: content/browser/browser_main_loop.cc Diff patch set 16 to 18 for the changes since latest review round.
The CQ bit was checked by grunell@chromium.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.chromium.or...
Description was changed from ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #18 (id:460001) has been deleted
The CQ bit was checked by grunell@chromium.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.chromium.or...
content/browser/browser_main_loop.cc lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/27 14:59:20, Henrik Grunell OOO back Mar 2 wrote: > AudioDebugFileWriter has been moved from content/ to media/ in another CL. I > have rebased and updated this CL. (Patch set 17 is the rebase only kept for > reference.) > > Ptal: > > Olga: overall > Dale: media/ interfaces. > Pavel: content/browser/browser_main_loop.cc > > Diff patch set 16 to 18 for the changes since latest review round. New patch with fix for a unit test. Try bots are now green.
lgtm https://codereview.chromium.org/2582703003/diff/500001/media/audio/audio_debu... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/500001/media/audio/audio_debu... media/audio/audio_debug_file_writer.h:35: virtual ~AudioDebugFileWriter(); It's inheritable for mocking only, right? Having interface/implementation would be cleaner, but it's not a show-stopper. Could you add a comment on that though?
Dale: friendly ping to review this again as well. https://codereview.chromium.org/2582703003/diff/500001/media/audio/audio_debu... File media/audio/audio_debug_file_writer.h (right): https://codereview.chromium.org/2582703003/diff/500001/media/audio/audio_debu... media/audio/audio_debug_file_writer.h:35: virtual ~AudioDebugFileWriter(); On 2017/03/01 18:36:23, o1ka wrote: > It's inheritable for mocking only, right? > Having interface/implementation would be cleaner, but it's not a show-stopper. > Could you add a comment on that though? Yes. Added a comment.
The CQ bit was checked by grunell@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/01 19:52:54, Henrik Grunell wrote: > Dale: friendly ping to review this again as well. Dale, I haven't heard anything for three days. I'm going to go ahead and land this anyway based on: * Olga and Pavel have given (new) lgtms. * Previous lgtm. * Changes to media/ surface since then is replacing a callback with a file task runner as parameter. * Previous comment "If you can pass the file_task_runner in that's probably fine." * I can create a follow-up CL or revert if necessary. (Still, please review the changes since patch set 16.)
The CQ bit was checked by grunell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, maxmorin@chromium.org, guidou@chromium.org, pfeldman@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2582703003/#ps520001 (title: "Code review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 520001, "attempt_start_ts": 1488446304803390,
"parent_rev": "020397fbdf70bb4398dab310bccb437ba5b2377a", "commit_rev":
"086a4113b499025bdd7db2c19a10bd8cd34a5997"}
Message was sent while issue was closed.
Description was changed from ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2582703003 Cr-Commit-Position: refs/heads/master@{#454218} Committed: https://chromium.googlesource.com/chromium/src/+/086a4113b499025bdd7db2c19a10... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:520001) as https://chromium.googlesource.com/chromium/src/+/086a4113b499025bdd7db2c19a10...
Message was sent while issue was closed.
Description was changed from ========== Audio output debug recording. Class diagram: https://docs.google.com/a/google.com/drawings/d/1qD-gv02z1vFxNDfB_exAbMA2ugDG... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2582703003 Cr-Commit-Position: refs/heads/master@{#454218} Committed: https://chromium.googlesource.com/chromium/src/+/086a4113b499025bdd7db2c19a10... ========== to ========== Audio output debug recording. Class diagram: https://docs.google.com/drawings/d/1QQwzUYpEZwB9R4kW5kPuCUxOHWjuJpo_ckaCvFj0M... Adds two new classes in media/: * AudioDebugRecordingHelper - wraps an AudioDebugFileWriter and handles data copying and thread hop. * AudioDebugRecordingManager owned by AudioManagerBase and responsible for keeping track of AudioDebugRecordingHelpers and enable/disable on them. Three new functions on AudioManager to initialize, enable and disable. Recordings are enabled/disabled from WebRTCInternals (chrome://webrtc-internals) and AudioDebugRecordingsHandler (from WebRTC logging extension API). BUG=531883 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2582703003 Cr-Commit-Position: refs/heads/master@{#454218} Committed: https://chromium.googlesource.com/chromium/src/+/086a4113b499025bdd7db2c19a10... ==========
Message was sent while issue was closed.
Whoops, sorry I missed your pings. Feel free to send me a direct e-mail or chat if I don't respond in 24 hours. In any case, I had been following along, just thought my previous approval was fine. So lgtm!
Message was sent while issue was closed.
On 2017/03/02 17:27:41, DaleCurtis wrote: > Whoops, sorry I missed your pings. Feel free to send me a direct e-mail or chat > if I don't respond in 24 hours. In any case, I had been following along, just > thought my previous approval was fine. So lgtm! OK, cool. |
