Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390153006:
Audio input debug recording refactoring to reduce thread hops and simplify object ownership (Closed)
|
Created:
4 years, 2 months ago by o1ka Modified:
4 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, miu+watch_chromium.org, Max Morin, audio-mojo-cl_google.com, tommi (sloooow) - chröme, chcunningham Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAudio input debug recording refactoring to reduce thread hops and simplify object ownership
AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL:
* Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus:
* AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more.
* AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer.
See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain.
So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController.
BUG=586431, 653861
TESTING=updated unit tests + manual debug recording
Committed: https://crrev.com/6b2e94b644dc909c077430e3ae6fcec341ccf5c6
Cr-Commit-Position: refs/heads/master@{#425360}
Patch Set 1 #
Total comments: 30
Patch Set 2 : review comments addressed #
Total comments: 23
Patch Set 3 : nit fixes + AudioFileWriter deletion changed #Patch Set 4 : cleaning up file writer deletion on FILE thread #
Total comments: 5
Patch Set 5 : Ending a comment with a dot. #Patch Set 6 : Rebase #Messages
Total messages: 56 (25 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership BUG= ========== to ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ==========
The CQ bit was checked by olka@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 input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ========== to ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue errors remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ==========
olka@chromium.org changed reviewers: + guidou@chromium.org
Guido, could you PTAL?
olka@chromium.org changed reviewers: + tommi@chromium.org
+ tommi@chromium.org as an owner of everything - and a reviewer if you have time :)
Description was changed from ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue errors remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ========== to ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
olka@chromium.org changed reviewers: + grunell@chromium.org
+grunell@ as the author of the original code (since he's back now)
https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:260: // non-nestable to make sure it is executed after all the writes are Actually, I'm not quite sure I interpreted this correctly. Should it be non-nestable?
https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:165: int interleaved_data_size_; nit: should this be size_t? https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:173: // base::Unretained is safe, because destructor is called on FILE thread. nit: It would be good to clarify that all other operations also run on the FILE thread, so that no operations will work with a partially constructed AFW. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:243: // be written to the file. This also won't be reflect in IsRecprding(). It's typo: IsRecprding -> IsRecording https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:258: // Callback takes ownership of |file_writer_|, so it will be deleted after It would be good to make it clearer what "Callback" is. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:260: // non-nestable to make sure it is executed after all the writes are On 2016/10/10 08:44:58, o1ka wrote: > Actually, I'm not quite sure I interpreted this correctly. Should it be > non-nestable? It's also not clear to me. Why would it be incorrect if you use PostTask? Same below. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:289: // Posting fix comment flow (A whole line just for Posting). https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:422: if (input_writer_ && input_writer_->IsRecording()) { Does the extra overhead of calling IsRecording() have any impact on performance?
https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:259: // Close() is executed or when FILE thread message loop is destroyed. Posting This could mean that the header isn't updated at a browser shutdown. This is likely not going to happen often; I think we can live with that. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:260: // non-nestable to make sure it is executed after all the writes are On 2016/10/10 10:09:23, Guido Urdaneta wrote: > On 2016/10/10 08:44:58, o1ka wrote: > > Actually, I'm not quite sure I interpreted this correctly. Should it be > > non-nestable? > > It's also not clear to me. Why would it be incorrect if you use PostTask? > Same below. It should only be needed if the writes call any task-running method. But it should also be OK to do this as a guarantee, to increase readability and avoid regression. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.h:32: AudioInputDebugWriter(const media::AudioParameters& params); explicit https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:422: if (input_writer_ && input_writer_->IsRecording()) { It's still should only accessed on the audio thread, right? (No locks?) Is it guaranteed that |input_writer_| isn't deleted while IsRecording() is executed? (Comment on that since it's not clear.) Even if it is now, this is prone to future bug introduction. Maybe comment on the declaration that it must live until AIC is deleted, or whatever guarantees that. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:188: std::unique_ptr<AudioInputWriter> input_writer, Nit: Maybe a comment on this? Could be confusing what the |sync_writer| and |input_writer| are. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:237: // Disbale debug recording of audio input. Nit: Disbale -> Disable https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:397: // Audio parameters for the debug recorting, Fix comment. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:398: const AudioParameters params_; Nit: Should the name reflect that it's for debug recording? E.g. |debug_recording_params_|.
https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:165: int interleaved_data_size_; On 2016/10/10 10:09:24, Guido Urdaneta wrote: > nit: should this be size_t? We get the value by multiplying two ints (AudioBus::frames() * AudioBus::channels()). Don't think we want type conversion. WDYT? https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:173: // base::Unretained is safe, because destructor is called on FILE thread. On 2016/10/10 10:09:24, Guido Urdaneta wrote: > nit: It would be good to clarify that all other operations also run on the FILE > thread, so that no operations will work with a partially constructed AFW. Well... Actually it will :) Fixing it. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:243: // be written to the file. This also won't be reflect in IsRecprding(). It's On 2016/10/10 10:09:23, Guido Urdaneta wrote: > typo: IsRecprding -> IsRecording Done. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:258: // Callback takes ownership of |file_writer_|, so it will be deleted after On 2016/10/10 10:09:23, Guido Urdaneta wrote: > It would be good to make it clearer what "Callback" is. Done. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:260: // non-nestable to make sure it is executed after all the writes are On 2016/10/10 10:56:59, Henrik Grunell wrote: > On 2016/10/10 10:09:23, Guido Urdaneta wrote: > > On 2016/10/10 08:44:58, o1ka wrote: > > > Actually, I'm not quite sure I interpreted this correctly. Should it be > > > non-nestable? > > > > It's also not clear to me. Why would it be incorrect if you use PostTask? > > Same below. > > It should only be needed if the writes call any task-running method. But it > should also be OK to do this as a guarantee, to increase readability and avoid > regression. I don't really understand what is behind this. But I see that Sequenced task runner posts non-nestable task for destructor. https://cs.chromium.org/chromium/src/base/sequenced_task_runner.h?q=DeleteSoo... Should we post all the writes as non-nestable to avoid races in file access? https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.h:32: AudioInputDebugWriter(const media::AudioParameters& params); On 2016/10/10 10:56:59, Henrik Grunell wrote: > explicit Done. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:422: if (input_writer_ && input_writer_->IsRecording()) { On 2016/10/10 10:09:24, Guido Urdaneta wrote: > Does the extra overhead of calling IsRecording() have any impact on performance? This is one bool check under a lock. And it's held for only that bool update on start/stop debug recording. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:422: if (input_writer_ && input_writer_->IsRecording()) { On 2016/10/10 10:56:59, Henrik Grunell wrote: > It's still should only accessed on the audio thread, right? (No locks?) Is it > guaranteed that |input_writer_| isn't deleted while IsRecording() is executed? > (Comment on that since it's not clear.) Even if it is now, this is prone to > future bug introduction. Maybe comment on the declaration that it must live > until AIC is deleted, or whatever guarantees that. |input_writer_| is a const unique_ptr, I fixed the declaration. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:188: std::unique_ptr<AudioInputWriter> input_writer, On 2016/10/10 10:56:59, Henrik Grunell wrote: > Nit: Maybe a comment on this? Could be confusing what the |sync_writer| and > |input_writer| are. It's how they are named in the class. Renamed all. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:237: // Disbale debug recording of audio input. On 2016/10/10 10:56:59, Henrik Grunell wrote: > Nit: Disbale -> Disable Done. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.h:398: const AudioParameters params_; On 2016/10/10 10:56:59, Henrik Grunell wrote: > Nit: Should the name reflect that it's for debug recording? E.g. > |debug_recording_params_|. This is left-over which is not needed
https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:165: int interleaved_data_size_; On 2016/10/10 13:48:01, o1ka wrote: > On 2016/10/10 10:09:24, Guido Urdaneta wrote: > > nit: should this be size_t? > > We get the value by multiplying two ints (AudioBus::frames() * > AudioBus::channels()). Don't think we want type conversion. WDYT? leave as int then
Patchset #2 (id:60001) has been deleted
PTAL at the new version. https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/40001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:289: // Posting On 2016/10/10 10:09:24, Guido Urdaneta wrote: > fix comment flow (A whole line just for Posting). Done. https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2390153006/diff/40001/media/audio/audio_input... media/audio/audio_input_controller.cc:422: if (input_writer_ && input_writer_->IsRecording()) { On 2016/10/10 13:48:01, o1ka wrote: > On 2016/10/10 10:09:24, Guido Urdaneta wrote: > > Does the extra overhead of calling IsRecording() have any impact on > performance? > > This is one bool check under a lock. And it's held for only that bool update on > start/stop debug recording. Removed locking. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:138: ~AudioFileWriter(); The question I have not found an answer for so far: if the message loop is being destroyed, which thread unexecuted callbacks are delete on? I.e., if I have a thread check in a destructor of an object, and this object is passed to a callback as base::Owned() - will this thread check fire on callback deletion? https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:292: // non-nestable just like in SequencedTaskRunner::DeleteSoon(). I still do not quite understand this "nestability". https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:318: // |client_sequence_checker_| will fix (git cl format did not do a perfect job here)
https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:263: // be written to the file. This also won't be reflect in WillWrite(). It's nit: reflect -> reflected https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:265: // happen in case of success anyways. This allow us to save on thread hops for nit: allow -> allows https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:320: // for |file_writer_|. So, we are not very precies here, but it's fine: we can nit: precies -> precise
https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:143: // Must be called on FILE thread. Nit: Does this class live only on the FILE thread? If so, you should have a class comment for that and remove these. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:194: AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() { Nit: DCHECK correct thread? https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:292: // non-nestable just like in SequencedTaskRunner::DeleteSoon(). On 2016/10/11 12:11:24, o1ka wrote: > I still do not quite understand this "nestability". Afaict, if you want to guarantee it's run after any currently running tasks, you need that. And not have to make assumptions on what the currently running tasks are doing. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:318: // |client_sequence_checker_| On 2016/10/11 12:11:24, o1ka wrote: > will fix (git cl format did not do a perfect job here) git cl format doesn't format comments afaik. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.h:38: bool WillWrite() override; Comment on this - not clear what it does. https://codereview.chromium.org/2390153006/diff/80001/media/audio/audio_input... File media/audio/audio_input_writer.h (right): https://codereview.chromium.org/2390153006/diff/80001/media/audio/audio_input... media/audio/audio_input_writer.h:32: // Returns true if it makes sense to schedule Write() calls. Can be called And when does it make sense? :) Describe, so that the user can understand how to use this function.
PTAL again https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:143: // Must be called on FILE thread. On 2016/10/11 13:07:24, Henrik Grunell wrote: > Nit: Does this class live only on the FILE thread? If so, you should have a > class comment for that and remove these. Done. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:194: AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() { On 2016/10/11 13:07:24, Henrik Grunell wrote: > Nit: DCHECK correct thread? Seem my question above :) But I changed the code a little bit now. From what I see in other places now it's fine to have a DCHECK. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:263: // be written to the file. This also won't be reflect in WillWrite(). It's On 2016/10/11 12:39:25, Guido Urdaneta wrote: > nit: reflect -> reflected Done. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:265: // happen in case of success anyways. This allow us to save on thread hops for On 2016/10/11 12:39:25, Guido Urdaneta wrote: > nit: allow -> allows Done. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:292: // non-nestable just like in SequencedTaskRunner::DeleteSoon(). On 2016/10/11 13:07:24, Henrik Grunell wrote: > On 2016/10/11 12:11:24, o1ka wrote: > > I still do not quite understand this "nestability". > > Afaict, if you want to guarantee it's run after any currently running tasks, you > need that. And not have to make assumptions on what the currently running tasks > are doing. Ok, looks like non-netsable post is needed if I want to serialize tasks which run other tasks, which is not our case. Removing it. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:318: // |client_sequence_checker_| On 2016/10/11 13:07:24, Henrik Grunell wrote: > On 2016/10/11 12:11:24, o1ka wrote: > > will fix (git cl format did not do a perfect job here) > > git cl format doesn't format comments afaik. it wraps lines. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:320: // for |file_writer_|. So, we are not very precies here, but it's fine: we can On 2016/10/11 12:39:25, Guido Urdaneta wrote: > nit: precies -> precise Done. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.h:38: bool WillWrite() override; On 2016/10/11 13:07:24, Henrik Grunell wrote: > Comment on this - not clear what it does. See base class https://codereview.chromium.org/2390153006/diff/80001/media/audio/audio_input... File media/audio/audio_input_writer.h (right): https://codereview.chromium.org/2390153006/diff/80001/media/audio/audio_input... media/audio/audio_input_writer.h:32: // Returns true if it makes sense to schedule Write() calls. Can be called On 2016/10/11 13:07:24, Henrik Grunell wrote: > And when does it make sense? :) Describe, so that the user can understand how to > use this function. Done.
Hold on! found a problem
PTAL now
lgtm But consider my comment about creating and passing the writer when enabling. This object is normally never used, so it would be good to create it lazily. https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.cc:194: AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() { On 2016/10/11 14:04:37, o1ka wrote: > On 2016/10/11 13:07:24, Henrik Grunell wrote: > > Nit: DCHECK correct thread? > > Seem my question above :) But I changed the code a little bit now. From what I > see in other places now it's fine to have a DCHECK. Great! https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... content/browser/renderer_host/media/audio_input_debug_writer.h:38: bool WillWrite() override; On 2016/10/11 14:04:37, o1ka wrote: > On 2016/10/11 13:07:24, Henrik Grunell wrote: > > Comment on this - not clear what it does. > > See base class Acknowledged. https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_debug_writer.h (right): https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_debug_writer.h:29: // non-blocking Nit: end sentence with . https://codereview.chromium.org/2390153006/diff/120001/media/audio/audio_inpu... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2390153006/diff/120001/media/audio/audio_inpu... media/audio/audio_input_controller.h:188: std::unique_ptr<AudioInputWriter> debug_writer, It seems unnecessary to always pass a debug writer. Could it be created and passed when enabling as before?
Thanks for the review! Re: creating the writer on demand, see https://bugs.chromium.org/p/chromium/issues/detail?id=586431. On 2016/10/13 07:26:11, Henrik Grunell wrote: > lgtm > > But consider my comment about creating and passing the writer when enabling. > This object is normally never used, so it would be good to create it lazily. > > https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... > File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): > > https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... > content/browser/renderer_host/media/audio_input_debug_writer.cc:194: > AudioInputDebugWriter::AudioFileWriter::~AudioFileWriter() { > On 2016/10/11 14:04:37, o1ka wrote: > > On 2016/10/11 13:07:24, Henrik Grunell wrote: > > > Nit: DCHECK correct thread? > > > > Seem my question above :) But I changed the code a little bit now. From what I > > see in other places now it's fine to have a DCHECK. > > Great! > > https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... > File content/browser/renderer_host/media/audio_input_debug_writer.h (right): > > https://codereview.chromium.org/2390153006/diff/80001/content/browser/rendere... > content/browser/renderer_host/media/audio_input_debug_writer.h:38: bool > WillWrite() override; > On 2016/10/11 14:04:37, o1ka wrote: > > On 2016/10/11 13:07:24, Henrik Grunell wrote: > > > Comment on this - not clear what it does. > > > > See base class > > Acknowledged. > > https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... > File content/browser/renderer_host/media/audio_input_debug_writer.h (right): > > https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... > content/browser/renderer_host/media/audio_input_debug_writer.h:29: // > non-blocking > Nit: end sentence with . > > https://codereview.chromium.org/2390153006/diff/120001/media/audio/audio_inpu... > File media/audio/audio_input_controller.h (right): > > https://codereview.chromium.org/2390153006/diff/120001/media/audio/audio_inpu... > media/audio/audio_input_controller.h:188: std::unique_ptr<AudioInputWriter> > debug_writer, > It seems unnecessary to always pass a debug writer. Could it be created and > passed when enabling as before?
I know it's kind of the point with the change. If there's no reasonable way fulfilling both, then that's fine. On 2016/10/13 07:46:23, o1ka wrote: > Thanks for the review! Re: creating the writer on demand, see > https://bugs.chromium.org/p/chromium/issues/detail?id=586431. > > > On 2016/10/13 07:26:11, Henrik Grunell wrote: > > lgtm > > > > But consider my comment about creating and passing the writer when enabling. > > This object is normally never used, so it would be good to create it lazily.
I'm open for suggestions :) On 2016/10/13 07:56:09, Henrik Grunell wrote: > I know it's kind of the point with the change. If there's no reasonable way > fulfilling both, then that's fine. > > On 2016/10/13 07:46:23, o1ka wrote: > > Thanks for the review! Re: creating the writer on demand, see > > https://bugs.chromium.org/p/chromium/issues/detail?id=586431. > > > > > > On 2016/10/13 07:26:11, Henrik Grunell wrote: > > > lgtm > > > > > > But consider my comment about creating and passing the writer when enabling. > > > This object is normally never used, so it would be good to create it lazily.
https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_debug_writer.cc:268: // |file_writer_| will be deleted on FILE thread. Why not put a DCHECK here to verify that?
https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_debug_writer.cc:268: // |file_writer_| will be deleted on FILE thread. On 2016/10/13 08:30:28, Guido Urdaneta wrote: > Why not put a DCHECK here to verify that? Not sure what do you mean. DCHECK is in AudioFileWriter destrcutor.
lgtm
https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... File content/browser/renderer_host/media/audio_input_debug_writer.cc (right): https://codereview.chromium.org/2390153006/diff/120001/content/browser/render... content/browser/renderer_host/media/audio_input_debug_writer.cc:268: // |file_writer_| will be deleted on FILE thread. On 2016/10/13 09:13:34, o1ka wrote: > On 2016/10/13 08:30:28, Guido Urdaneta wrote: > > Why not put a DCHECK here to verify that? > > Not sure what do you mean. DCHECK is in AudioFileWriter destrcutor. Acknowledged. Maybe remove this comment.
olka@chromium.org changed reviewers: + chcunningham@chromium.org - tommi@chromium.org
Changing an OWNER, since Tommi is travelling. chcunningham@ - PTAL (or just RS)
The CQ bit was checked by olka@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...
tommi@chromium.org changed reviewers: + tommi@chromium.org
rs lgtm
olka@chromium.org changed reviewers: - chcunningham@chromium.org
Apologies for spam, switching OWNERs back since Tommi had a chance to look at the CL.
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, guidou@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2390153006/#ps140001 (title: "Ending a comment with a dot.")
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
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, guidou@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2390153006/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ========== to ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording ========== to ========== Audio input debug recording refactoring to reduce thread hops and simplify object ownership AudioInputRendererHost has to do too many thread hops to handle debug recording enable/disable. This CL: * Passes recording file management to AudioInputDebugWriter, which takes care of file operations executed on the correct thread. Now it can be deleted on any thread, and thus: * AudioInputDebugWriter ownership is passed to AudioInputController, AIRH does not care about its lifetime any more. * AudioEntry does not store audio parameters (required for debug recording only) any more. AudioInputDebugWriter holds them, and recording is controlled with Start()/Stop() instead of creation/deletion of the writer. See the diagram: https://drive.google.com/open?id=0Bw1QgdA2sQtMbDFXeUdvUnZzTE0. Red arrows are going away, blue arrows remain. So, now AIRH only (1) communicates with UI thread to get debug recording settings and (2) issues enable/disable recording calls for each registered AudioInputController. BUG=586431,653861 TESTING=updated unit tests + manual debug recording Committed: https://crrev.com/6b2e94b644dc909c077430e3ae6fcec341ccf5c6 Cr-Commit-Position: refs/heads/master@{#425360} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6b2e94b644dc909c077430e3ae6fcec341ccf5c6 Cr-Commit-Position: refs/heads/master@{#425360} |
