Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624403002:
Refactor AudioInputController and related interfaces. (Closed)
|
Created:
3 years, 11 months ago by tommi (sloooow) - chröme Modified:
3 years, 11 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, ossu-chromium, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor AudioInputController and related interfaces.
* Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied.
* Made methods and variables private to keep test code separate and force use of ForTesting methods.
* Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback.
* Moved inheritance of AudioInputCallback into a subclass.
* An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations.
* Made several member variable const, as their value must never change.
* Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit.
* Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path).
* Added a new UMA stat to track how often we get errors reported from the audio layer during capture.
BUG=681150, 681152
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/2624403002
Cr-Commit-Position: refs/heads/master@{#443925}
Committed: https://chromium.googlesource.com/chromium/src/+/ce9a2515fc22794d7beb2268eaae525279a0c3b1
Patch Set 1 #
Total comments: 41
Patch Set 2 : Address comments #
Total comments: 2
Patch Set 3 : Update the other speech_recognition_browsertest.cc #Patch Set 4 : Update comments #
Total comments: 6
Patch Set 5 : Don't report errors that occur after (or rather while) closing the stream #
Total comments: 2
Patch Set 6 : Remove superfluous DCHECK #Patch Set 7 : Use a weak pointer factory when posting from other threads than the main thread. #Patch Set 8 : Rebase #
Total comments: 14
Patch Set 9 : Address comments #Messages
Total messages: 71 (29 generated)
Description was changed from ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG= ========== to ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG= 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 ==========
The CQ bit was checked by tommi@chromium.org to run a CQ dry run
tommi@chromium.org changed reviewers: + maxmorin@chromium.org, olka@chromium.org
Max and Olga - can you take a look please?
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm! Nice cleanup. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:411: DCHECK(!audio_callback_); DoRecord might be called multiple times, so it should probably just return if audio_callback_. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:442: base::StringPrintf("AIC::DoClose: stream duration="); Unnecessary StringPrintf (or crate the entire log_string here). https://codereview.chromium.org/2624403002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2624403002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:24043: +<histogram name="Media.Audio.Capture.CallbackError" enum="Boolean"> BooleanError maybe?
That's a very handy change for our audio process work! Seems a bit racy as it is now though. https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_renderer_host.h (left): https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_renderer_host.h:111: void OnData(media::AudioInputController* controller, \o/ that's just what we need for audio process work! https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_renderer_host.h:239: media::UserInputMonitor* user_input_monitor_; Should this be const? https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:93: bool error_during_callback() const { return error_during_callback_; } Similar data race (see AudioInputController comment) for these two members: they are updated on callback thread; but we read them on audio thread in AudioInputController::DoClose() right after we called Stop() on the stream (i.e. while there may be in progress/pending callbacks). https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:116: // Called on the audio callback thread while recording is enabled. In other places it's called "hw callback thread" (I like "audio" version more). https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:125: // Might require some modifications to AudioInputDebugWriter though. "Requires some refactoring since both AudioInputController::WriteInputDataForDebugging() and AudioInputDebugWriter::Write() are not thread-safe" - it's not immediately obvious, and having a comment may prevent a quick dangerous change in the future. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:127: // AudioInputWriter... remove the interface? The interface is in media together with controller, and the impl is in content - this seems to be the reason for having it. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:438: base::TimeDelta duration = const? https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:442: base::StringPrintf("AIC::DoClose: stream duration="); On 2017/01/12 09:31:10, Max Morin wrote: > or crate the entire log_string here - then can be const as well. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:620: bool key_pressed = current_count != prev_key_down_count_; both const? https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:638: kPowerMonitorLogIntervalSeconds) { Can be early return instead. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:643: const int mic_volume_percent = static_cast<int>(100.0 * volume); Is "floor" operation intentional? We won't distinguish between 0.9% and 1% - not sure if it matters though. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:651: last_audio_level_log_time_ = base::TimeTicks::Now(); This is a rather costly call, so I would make it once before l.637. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:348: // Enabled in DoCrete() but not in DoCreateForStream(). This comment is not quite clear (I know it's an old one). And I would add "is never modified after that, thus safe to access on audio(or hw?) callback thread. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:372: // The value of this pointer is only set and read on the audio thread. It is also accessed on audio callback thread. Doesn't it mean that we have a data race if callback execution is in progress on callback thread while |audio_callback_| is reset on audio thread in AudioInputController::DoClose()? See coment to AudioInputStream::Stop() (https://cs.chromium.org/chromium/src/media/audio/audio_io.h?sq=package:chromi...): "// Stops recording audio. Effect might not be instantaneous as there could be // pending audio callbacks in the queue which will be issued first before // recording stops."
Address comments
tommi@chromium.org changed reviewers: + henrika@chromium.org
+Henrik - can you take a look over the comments? There are a couple of things that I think you can provide some background to or confirm/deny if making a change (e.g. floor) is what we want. https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_renderer_host.h (left): https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_renderer_host.h:111: void OnData(media::AudioInputController* controller, On 2017/01/12 12:07:38, o1ka wrote: > \o/ that's just what we need for audio process work! :D https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/media/audio_input_renderer_host.h (right): https://codereview.chromium.org/2624403002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/media/audio_input_renderer_host.h:239: media::UserInputMonitor* user_input_monitor_; On 2017/01/12 12:07:38, o1ka wrote: > Should this be const? I haven't dug into this class that much yet as far as that goes but yes looks like it. Done. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:93: bool error_during_callback() const { return error_during_callback_; } On 2017/01/12 12:07:38, o1ka wrote: > Similar data race (see AudioInputController comment) for these two members: they > are updated on callback thread; but we read them on audio thread in > AudioInputController::DoClose() right after we called Stop() on the stream (i.e. > while there may be in progress/pending callbacks). Yes, that pattern is safe. It's just that the documentation for Stop() was incorrect. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:116: // Called on the audio callback thread while recording is enabled. On 2017/01/12 12:07:38, o1ka wrote: > In other places it's called "hw callback thread" (I like "audio" version more). hmm... changed to "audio/hw callback thread" :-| https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:125: // Might require some modifications to AudioInputDebugWriter though. On 2017/01/12 12:07:38, o1ka wrote: > "Requires some refactoring since both > AudioInputController::WriteInputDataForDebugging() and > AudioInputDebugWriter::Write() are not thread-safe" - it's not immediately > obvious, and having a comment may prevent a quick dangerous change in the > future. Comment updated. WriteInputDataForDebugging itself is safe as is, but with this refactoring, it would be going away anyway. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:127: // AudioInputWriter... remove the interface? On 2017/01/12 12:07:38, o1ka wrote: > The interface is in media together with controller, and the impl is in content - > this seems to be the reason for having it. ah, makes sense. removed this part of the comment. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:411: DCHECK(!audio_callback_); On 2017/01/12 09:31:10, Max Morin wrote: > DoRecord might be called multiple times, so it should probably just return if > audio_callback_. Thanks. Done. Do you know in what situation that happens? https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:442: base::StringPrintf("AIC::DoClose: stream duration="); On 2017/01/12 12:07:38, o1ka wrote: > On 2017/01/12 09:31:10, Max Morin wrote: > > or crate the entire log_string here > - then can be const as well. > ah, sorry I missed this somehow (this is moved code). Made const and reduced the string copying+conversion inefficiencies. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:620: bool key_pressed = current_count != prev_key_down_count_; On 2017/01/12 12:07:38, o1ka wrote: > both const? Done. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:638: kPowerMonitorLogIntervalSeconds) { On 2017/01/12 12:07:38, o1ka wrote: > Can be early return instead. Done. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:643: const int mic_volume_percent = static_cast<int>(100.0 * volume); On 2017/01/12 12:07:38, o1ka wrote: > Is "floor" operation intentional? We won't distinguish between 0.9% and 1% - not > sure if it matters though. I think it's better that henrika@ explains the thinking. I'll add him to the review. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:651: last_audio_level_log_time_ = base::TimeTicks::Now(); On 2017/01/12 12:07:39, o1ka wrote: > This is a rather costly call, so I would make it once before l.637. Agreed and likely more accurate too. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:348: // Enabled in DoCrete() but not in DoCreateForStream(). On 2017/01/12 12:07:39, o1ka wrote: > This comment is not quite clear (I know it's an old one). > And I would add "is never modified after that, thus safe to access on audio(or > hw?) callback thread. Yeah it could be more clear :) I looked at the implementation and rewrote the comment, hopefully making it slightly more clear. However, I don't know why this is turned on only if the AGC is turned on. I think henrika@ can probably explain that and I think it might be in order to document what power monitoring actually is in this context. This feature is not related to the PowerMonitor class for example, which Chromium developers might assume when reading. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:372: // The value of this pointer is only set and read on the audio thread. On 2017/01/12 12:07:39, o1ka wrote: > It is also accessed on audio callback thread. Doesn't it mean that we have a > data race if callback execution is in progress on callback thread while > |audio_callback_| is reset on audio thread in AudioInputController::DoClose()? > See coment to AudioInputStream::Stop() > (https://cs.chromium.org/chromium/src/media/audio/audio_io.h?sq=package:chromi...): > "// Stops recording audio. Effect might not be instantaneous as there could be > // pending audio callbacks in the queue which will be issued first before > // recording stops." Thanks for pointing out that documentation. It's wrong :) It was also written 8 years ago it seems. It was correct for a long while but we had a lot of issues (crashes) with that behavior and made all implementations of Stop() complete synchronously. I've updated the comment in audio_io.h. https://codereview.chromium.org/2624403002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2624403002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:24043: +<histogram name="Media.Audio.Capture.CallbackError" enum="Boolean"> On 2017/01/12 09:31:10, Max Morin wrote: > BooleanError maybe? Done.
Superb stuff Tommi ;-) Added some more details trying to answer the outstanding questions. Sorry if I don't remember the exact details. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:643: const int mic_volume_percent = static_cast<int>(100.0 * volume); Good comment. But it does not matter. The point of the stat was to be able to look for "very low values" to see if the volume control level could be causing a claimed low volume. Guess we could to static_cast<int>(100 * volume + 0.5) to be "more correct". https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:348: // Enabled in DoCrete() but not in DoCreateForStream(). When we first added the power monitor, the goal we had was to ensure that it only was activated for WebRTC clients. I don't remember now who uses DoCreateForStream() instead (is it Cast), but it is not called by WebRTC and that was the main reason. Same for AGC. Only WebRTC streams has AGC enabled and that ensured that the stats was more focuses on "our" clients.
https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:411: DCHECK(!audio_callback_); On 2017/01/12 14:21:00, tommi (chrömium) wrote: > On 2017/01/12 09:31:10, Max Morin wrote: > > DoRecord might be called multiple times, so it should probably just return if > > audio_callback_. > > Thanks. Done. Do you know in what situation that happens? It would only happen with a buggy/misbehaving renderer.
Thank you and lgtm! https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:93: bool error_during_callback() const { return error_during_callback_; } On 2017/01/12 14:21:00, tommi (chrömium) wrote: > On 2017/01/12 12:07:38, o1ka wrote: > > Similar data race (see AudioInputController comment) for these two members: > they > > are updated on callback thread; but we read them on audio thread in > > AudioInputController::DoClose() right after we called Stop() on the stream > (i.e. > > while there may be in progress/pending callbacks). > > Yes, that pattern is safe. It's just that the documentation for Stop() was > incorrect. Acknowledged. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:643: const int mic_volume_percent = static_cast<int>(100.0 * volume); On 2017/01/12 15:16:14, henrika wrote: > Good comment. But it does not matter. The point of the stat was to be able to > look for "very low values" to see if the volume control level could be causing a > claimed low volume. > > Guess we could to static_cast<int>(100 * volume + 0.5) to be "more correct". Acknowledged. https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:348: // Enabled in DoCrete() but not in DoCreateForStream(). On 2017/01/12 15:16:14, henrika wrote: > When we first added the power monitor, the goal we had was to ensure that it > only was activated for WebRTC clients. I don't remember now who uses > DoCreateForStream() instead (is it Cast), but it is not called by WebRTC and > that was the main reason. Same for AGC. Only WebRTC streams has AGC enabled and > that ensured that the stats was more focuses on "our" clients. Thank you both for clarification! https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:372: // The value of this pointer is only set and read on the audio thread. > Thanks for pointing out that documentation. It's wrong :) > > It was also written 8 years ago it seems. It was correct for a long while but > we had a lot of issues (crashes) with that behavior and made all implementations > of Stop() complete synchronously. I've updated the comment in audio_io.h. It's not obvious from platform-specific implementations at all, and there is no single comment on it in that code. So I blindly trust you :) Anyways, if such race is possible the current version seems to have it, too. Would be nice to mention that it is accessed on hw callback thread and must be valid between stream_::Start() and stream_::Stop(). https://codereview.chromium.org/2624403002/diff/20001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/20001/media/audio/audio_input... media/audio/audio_input_controller.cc:118: // Called on the audio/hw callback thread while recording is enabled. It's still named "hw callback thread" in other places; in this case my strong believe is that consistence is more important that correctness or beauty :)
Update the other speech_recognition_browsertest.cc
The CQ bit was checked by tommi@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: + holte@chromium.org - henrika@chromium.org
+holte for histograms.xml
Update comments
olka, maxmorin - I missed fixing the test in speech_recognition_browsertest.cc previously. So there are now three new files in the cl (small changes) that need reviewing. sorry about that: content/browser/speech/speech_recognition_browsertest.cc media/audio/test_audio_input_controller_factory.h media/audio/test_audio_input_controller_factory.cc https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.cc:438: base::TimeDelta duration = On 2017/01/12 12:07:38, o1ka wrote: > const? Done. (forgot to ack in the patch set that I made it const) https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/1/media/audio/audio_input_con... media/audio/audio_input_controller.h:372: // The value of this pointer is only set and read on the audio thread. On 2017/01/12 17:27:15, o1ka wrote: > > Thanks for pointing out that documentation. It's wrong :) > > > > It was also written 8 years ago it seems. It was correct for a long while but > > we had a lot of issues (crashes) with that behavior and made all > implementations > > of Stop() complete synchronously. I've updated the comment in audio_io.h. > > It's not obvious from platform-specific implementations at all, and there is no > single comment on it in that code. So I blindly trust you :) > Anyways, if such race is possible the current version seems to have it, too. > > Would be nice to mention that it is accessed on hw callback thread and must be > valid between stream_::Start() and stream_::Stop(). I was trying to convey that with "Valid only while 'recording' (aka capturing). The value of this pointer is only set and read on the audio thread." but since that wasn't obvious, I guess I failed :) Updated now. https://codereview.chromium.org/2624403002/diff/20001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/20001/media/audio/audio_input... media/audio/audio_input_controller.cc:118: // Called on the audio/hw callback thread while recording is enabled. On 2017/01/12 17:27:15, o1ka wrote: > It's still named "hw callback thread" in other places; in this case my strong > believe is that consistence is more important that correctness or beauty :) ack. Changed to simply "hw callback thread".
FYI, my comments earlier felt a bit fuzzy. Basically, all the logging (power level, mic level etc.) were done with focus only on WebRTC clients. I ran tests with the most popular clients and tried to ensure that logging and stats only was added when WebRTC was involved. It was difficult to document the exact reason why things were done in this way.
LGTM
On 2017/01/12 19:47:25, henrika wrote: > FYI, my comments earlier felt a bit fuzzy. Basically, all the logging > (power level, mic level etc.) were done with focus only on WebRTC clients. > I ran tests with the most popular clients and tried to ensure that logging > and stats only was added when WebRTC was involved. It was difficult to > document the exact reason why things were done in this way. The logging isn't free as far as cycles goes. Should it be disabled in official builds or is it something that's needed for webrtc logs?
histograms lgtm
The CQ bit was checked by tommi@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
still lgtm
The failures on Mac seem to be related to the CL as well, don't they? (some nits I missed in previous round - sorry) https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.cc:414: DCHECK(!audio_callback_); No need, we return on l.411 https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.h:268: const scoped_refptr<base::SingleThreadTaskRunner>& GetTaskRunnerForTesting() protected? https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.h:273: EventHandler* GetHandlerForTesting() const { return handler_; } protected? (and then both can be normal non-for-testing getters)
On 2017/01/13 10:07:07, o1ka wrote: > The failures on Mac seem to be related to the CL as well, don't they? I'm checking. If you look at the log, it appears that the bot has issues with the default audio device so maybe there is not new behavior here but still something to examine. > (some nits I missed in previous round - sorry) > > https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... > File media/audio/audio_input_controller.cc (right): > > https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... > media/audio/audio_input_controller.cc:414: DCHECK(!audio_callback_); > No need, we return on l.411 > > https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... > File media/audio/audio_input_controller.h (right): > > https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... > media/audio/audio_input_controller.h:268: const > scoped_refptr<base::SingleThreadTaskRunner>& GetTaskRunnerForTesting() > protected? > > https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... > media/audio/audio_input_controller.h:273: EventHandler* GetHandlerForTesting() > const { return handler_; } > protected? > > (and then both can be normal non-for-testing getters)
https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.h:273: EventHandler* GetHandlerForTesting() const { return handler_; } On 2017/01/13 10:07:07, o1ka wrote: > protected? > > (and then both can be normal non-for-testing getters) I'm not following... Can you clarify the question? These methods are protected to allow access to subclasses as are the constructor and constructor.
lgtm if mac failures are unrelated. https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.h:273: EventHandler* GetHandlerForTesting() const { return handler_; } On 2017/01/13 10:31:51, tommi (chrømium) wrote: > On 2017/01/13 10:07:07, o1ka wrote: > > protected? > > > > (and then both can be normal non-for-testing getters) > > I'm not following... Can you clarify the question? These methods are protected > to allow access to subclasses as are the constructor and constructor. Oh, sorry - I misread these two as public somehow.
Don't report errors that occur after (or rather while) closing the stream
The CQ bit was checked by tommi@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...
I found and fixed the error that was happening on the mac bot. The issue was with how the audio source in those tests handles shutdown. It will report an error on the hw callback thread while we're closing the stream. When the AudioInputController then reports the error, the Close() callback has already been executed and the |entry| deleted (which is what the CHECK() was about).
Remove superfluous DCHECK
https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/60001/media/audio/audio_input... media/audio/audio_input_controller.cc:414: DCHECK(!audio_callback_); On 2017/01/13 10:07:07, o1ka wrote: > No need, we return on l.411 Done.
On 2017/01/13 11:51:54, tommi (chrømium) wrote: > I found and fixed the error that was happening on the mac bot. The issue was > with how the audio source in those tests handles shutdown. It will report an > error on the hw callback thread while we're closing the stream. When the > AudioInputController then reports the error, the Close() callback has already > been executed and the |entry| deleted (which is what the CHECK() was about). Won't we have the same problem with any other queued notification, like OnCreated() for example? As far as I remember, that's why there were all those "if (handler_)" checks, and |handler_| was reset on closing (https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?ty...).
On 2017/01/13 12:14:24, o1ka wrote: > On 2017/01/13 11:51:54, tommi (chrømium) wrote: > > I found and fixed the error that was happening on the mac bot. The issue was > > with how the audio source in those tests handles shutdown. It will report an > > error on the hw callback thread while we're closing the stream. When the > > AudioInputController then reports the error, the Close() callback has already > > been executed and the |entry| deleted (which is what the CHECK() was about). > > Won't we have the same problem with any other queued notification, like > OnCreated() for example? > As far as I remember, that's why there were all those "if (handler_)" checks, > and |handler_| was reset on closing > (https://cs.chromium.org/chromium/src/media/audio/audio_input_controller.cc?ty...). That's a good question. Let me take a look, maybe it's time to change the controller to not be refcounted and instead use a weak pointer for posting these tasks.
Use a weak pointer factory when posting from other threads than the main thread.
The CQ bit was checked by tommi@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...
OK, so in the latest patch set, I changed it so that we avoid adding a reference to the input controller from the hw callback thread. I thought we might be posting from the audio thread too and by doing that adding a reference, but it doesn't look like it. This makes the lifetime guarantees better and moves us towards not needing ref counting. However, as the input controller still straddles two threads, it needs to continue to be ref counted. Now, by using the weak pointer factory, we can get rid of potentially pending callbacks on the audio thread by invalidating the weak pointers, and no "if (handler_)" or "if (audio_callback_)" checks are needed. I also changed the unit tests to actually use a separate thread for the audio thread. This makes the testing more correct (would e.g. catch race issues on asan bots etc) and helped while changing the approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG= 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 ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG=681150,681152 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 ==========
Rebase
The CQ bit was checked by tommi@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.
still lgtm. https://codereview.chromium.org/2624403002/diff/80001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/80001/media/audio/audio_input... media/audio/audio_input_controller.cc:485: return; Is it possible to add a unit test for this rather than relying on the browser tests? https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.h:401: private: Unnecessary?
I like the approach, maybe we can simplify object responsibilities a bit? (would like to double-check a couple more places regarding objects lifetime yet) https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:143: bool key_pressed = controller_->CheckForKeyboardInput(); maxmorin@: here is another dependency on browser process we missed. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:144: controller_->sync_writer_->Write(source, volume, key_pressed, I feel like relationships between AIC and AIC::Callback are too complicated now. What if AIC::Callback will own |sync_writer|? And what if all the "log audio levels" logic will be an AIC method? And same with PerformOptionalDebugRecording(): what if it will be an AIC method? And then we provide AIC::ReportError() and AIC::Callback() does not need |weak_controller_|.
https://codereview.chromium.org/2624403002/diff/80001/media/audio/audio_input... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/80001/media/audio/audio_input... media/audio/audio_input_controller.cc:485: return; On 2017/01/16 10:02:14, Max Morin wrote: > Is it possible to add a unit test for this rather than relying on the browser > tests? Possibly, but it's a bit tricky to do correctly and reliably. Btw, in the latest patch set, the approach for this callback is different, so the if() statement is gone. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:144: controller_->sync_writer_->Write(source, volume, key_pressed, On 2017/01/16 10:23:31, o1ka wrote: > I feel like relationships between AIC and AIC::Callback are too complicated now. Before, they were all in the same class. With this change, I find that it's clearer that the lifetime of the functionality that goes together with the Callback class, is shorter than what goes for the AIC. > What if AIC::Callback will own |sync_writer|? I considered that but didn't move ownership to here because of the variable in the parent class. However, considering that there's no "StopRecording" method, only Close() and now that I look at the implementation here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... It looks like the sync writer ownership can in fact be moved to this class. In practice it doesn't make a huge difference though because sync_writer_ is a const variable in the controller and Close() will be done at the same time and on the same thread. > And what if all the "log audio levels" logic will be an AIC method? The PostTask call must be done in the Callback class because we're on the hw callback thread and must use the weak pointer. The weak pointer is also constructed on the audio manager's thread, so creating a new weak pointer inside of CheckAudioPower, is not possible. > And same with PerformOptionalDebugRecording(): what if it will be an AIC method? > And then we provide AIC::ReportError() and AIC::Callback() does not need > |weak_controller_|. The AIC would then just need its own weak_controller_ and that would be confusing. The key thing is that adding a reference count to the AIC from the hw callback thread, is something we need to avoid. Since the hw callback thread belongs to the Callback thread, I prefer that all PostTask calls from there, be done within that class and use the same weak_controller_ variable. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.h:401: private: On 2017/01/16 10:02:14, Max Morin wrote: > Unnecessary? Done.
> > What if AIC::Callback will own |sync_writer|? > > I considered that but didn't move ownership to here because of the variable in > the parent class. However, considering that there's no "StopRecording" method, > only Close() and now that I look at the implementation here: > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... > > It looks like the sync writer ownership can in fact be moved to this class. In > practice it doesn't make a huge difference though because sync_writer_ is a > const variable in the controller and Close() will be done at the same time and > on the same thread. > Looking at this again, I'm not sure it's worth it. As is, there's one member variable in the AIC: SyncWriter* const sync_writer_; It's nice that it's const, which means it must be set in the constructor and never change after that. So I'd like to not change that. "Moving" the pointer to the Callback class (and set the original pointer to nullptr), then isn't possible. Copying the pointer value is of course possible. This adds a second SyncWriter pointer to a private subclass of the AIC that already has a pointer to the AIC. Having two pointers to the same thing seems like something we should avoid. So I don't think it's something we should do.
lgtm % a couple of questions. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:87: class AudioInputController::AudioCallback Could you add a comment explaining the reasons why functionality is split between AIC and AIC::Callback the way it is? https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:144: controller_->sync_writer_->Write(source, volume, key_pressed, On 2017/01/16 12:34:30, tommi (chrømium) wrote: > On 2017/01/16 10:23:31, o1ka wrote: > > I feel like relationships between AIC and AIC::Callback are too complicated > now. > > Before, they were all in the same class. With this change, I find that it's > clearer that the lifetime of the functionality that goes together with the > Callback class, is shorter than what goes for the AIC. > > > What if AIC::Callback will own |sync_writer|? > > I considered that but didn't move ownership to here because of the variable in > the parent class. However, considering that there's no "StopRecording" method, > only Close() and now that I look at the implementation here: > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... > > It looks like the sync writer ownership can in fact be moved to this class. In > practice it doesn't make a huge difference though because sync_writer_ is a > const variable in the controller and Close() will be done at the same time and > on the same thread. > > > And what if all the "log audio levels" logic will be an AIC method? > > The PostTask call must be done in the Callback class because we're on the hw > callback thread and must use the weak pointer. The weak pointer is also > constructed on the audio manager's thread, so creating a new weak pointer inside > of CheckAudioPower, is not possible. > > > And same with PerformOptionalDebugRecording(): what if it will be an AIC > method? > > And then we provide AIC::ReportError() and AIC::Callback() does not need > > |weak_controller_|. > > The AIC would then just need its own weak_controller_ and that would be > confusing. The key thing is that adding a reference count to the AIC from the > hw callback thread, is something we need to avoid. Since the hw callback thread > belongs to the Callback thread, I prefer that all PostTask calls from there, be > done within that class and use the same weak_controller_ variable. > Agree that moving ownership does not make sense. And I like that we use aggregation instead of inheritance here. But I don't like how we split controller logic, including private member access, between two classes - I find it not very readable. Why CheckForKeyboardInput() logic and |prev_key_down_count_| is in controller? Same for |power_measurement_is_enabled_| - why can't it be a member of AIC::Callback()? The edge between objects looks a bit random and too complicated to me. However, it may be a personal preference, and we can refactor it later if we need ;) so - up to you. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:415: prev_key_down_count_ = user_input_monitor_->GetKeyPressCount(); Shouldn't we init it right before starting the stream at l.434? Otherwise the first callback will count all key presses since the controller was created, and I believe we are interested in key presses happened while the input data buffer was captured. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:489: weak_ptr_factory_.InvalidateWeakPtrs(); We can do it first thing after l.442 to avoid posting to this thread while DoClose() is being executed: anyways all the pointers will be invalidated by the moment we exit it.
https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:489: weak_ptr_factory_.InvalidateWeakPtrs(); On 2017/01/16 15:04:52, o1ka wrote: > We can do it first thing after l.442 to avoid posting to this thread while > DoClose() is being executed: anyways all the pointers will be invalidated by the > moment we exit it. Ah, disregard this, I forgot that post does not check the pointer.
All in all, very nice improvement! Thank you!!
Address comments
https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:87: class AudioInputController::AudioCallback On 2017/01/16 15:04:52, o1ka wrote: > Could you add a comment explaining the reasons why functionality is split > between AIC and AIC::Callback the way it is? Done. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:144: controller_->sync_writer_->Write(source, volume, key_pressed, On 2017/01/16 15:04:52, o1ka wrote: > On 2017/01/16 12:34:30, tommi (chrømium) wrote: > > On 2017/01/16 10:23:31, o1ka wrote: > > > I feel like relationships between AIC and AIC::Callback are too complicated > > now. > > > > Before, they were all in the same class. With this change, I find that it's > > clearer that the lifetime of the functionality that goes together with the > > Callback class, is shorter than what goes for the AIC. > > > > > What if AIC::Callback will own |sync_writer|? > > > > I considered that but didn't move ownership to here because of the variable in > > the parent class. However, considering that there's no "StopRecording" > method, > > only Close() and now that I look at the implementation here: > > > > > https://cs.chromium.org/chromium/src/content/browser/renderer_host/media/audi... > > > > It looks like the sync writer ownership can in fact be moved to this class. > In > > practice it doesn't make a huge difference though because sync_writer_ is a > > const variable in the controller and Close() will be done at the same time and > > on the same thread. > > > > > And what if all the "log audio levels" logic will be an AIC method? > > > > The PostTask call must be done in the Callback class because we're on the hw > > callback thread and must use the weak pointer. The weak pointer is also > > constructed on the audio manager's thread, so creating a new weak pointer > inside > > of CheckAudioPower, is not possible. > > > > > And same with PerformOptionalDebugRecording(): what if it will be an AIC > > method? > > > And then we provide AIC::ReportError() and AIC::Callback() does not need > > > |weak_controller_|. > > > > The AIC would then just need its own weak_controller_ and that would be > > confusing. The key thing is that adding a reference count to the AIC from the > > hw callback thread, is something we need to avoid. Since the hw callback > thread > > belongs to the Callback thread, I prefer that all PostTask calls from there, > be > > done within that class and use the same weak_controller_ variable. > > > > Agree that moving ownership does not make sense. And I like that we use > aggregation instead of inheritance here. But I don't like how we split > controller logic, including private member access, between two classes - I find > it not very readable. > Why CheckForKeyboardInput() logic and |prev_key_down_count_| is in controller? > Same for |power_measurement_is_enabled_| - why can't it be a member of > AIC::Callback()? The edge between objects looks a bit random and too complicated > to me. > However, it may be a personal preference, and we can refactor it later if we > need ;) so - up to you. I agree that it's not exactly elegant. I added a comment that explains why we do this here and that it can be improved as far as readability goes. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:415: prev_key_down_count_ = user_input_monitor_->GetKeyPressCount(); On 2017/01/16 15:04:52, o1ka wrote: > Shouldn't we init it right before starting the stream at l.434? > Otherwise the first callback will count all key presses since the controller was > created, and I believe we are interested in key presses happened while the input > data buffer was captured. Good point, moved. In general I think this isn't very clear in the class. E.g. we measure "duration" from create to close, and not capture to stop capture. https://codereview.chromium.org/2624403002/diff/140001/media/audio/audio_inpu... media/audio/audio_input_controller.cc:489: weak_ptr_factory_.InvalidateWeakPtrs(); On 2017/01/16 15:08:51, o1ka wrote: > On 2017/01/16 15:04:52, o1ka wrote: > > We can do it first thing after l.442 to avoid posting to this thread while > > DoClose() is being executed: anyways all the pointers will be invalidated by > the > > moment we exit it. > > Ah, disregard this, I forgot that post does not check the pointer. Acknowledged.
The CQ bit was checked by tommi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, henrika@chromium.org, maxmorin@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/2624403002/#ps160001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/16 15:32:13, o1ka wrote: > All in all, very nice improvement! Thank you!! cc: ossu@ FYI
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484586172604880,
"parent_rev": "da0a11b3af6c553a39250e6c9cec38b29084776a", "commit_rev":
"ce9a2515fc22794d7beb2268eaae525279a0c3b1"}
Message was sent while issue was closed.
Description was changed from ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG=681150,681152 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 ========== Refactor AudioInputController and related interfaces. * Remove special callback code (OnData callback) for Speech and instead use the callback interface that's already supplied. * Made methods and variables private to keep test code separate and force use of ForTesting methods. * Removed state_ and lock_ variables from AudioInputController and removed the need for acquiring said lock in the audio callback. * Moved inheritance of AudioInputCallback into a subclass. * An instance of the callback subclass only exists while the callback is active. This makes the threading model clearer and allows us to avoid using locks and atomic operations. * Made several member variable const, as their value must never change. * Made supplying a handler_ non-optional as well as sync_writer_. This also simplifies the code quite a bit. * Changed the way we report UMA stats. We don't use a reference counter anymore or atomic operations. I suspect that we've been getting incorrect stats reported due to issues with how we were previously doing this (in addition to slowing down the audio path). * Added a new UMA stat to track how often we get errors reported from the audio layer during capture. BUG=681150,681152 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/2624403002 Cr-Commit-Position: refs/heads/master@{#443925} Committed: https://chromium.googlesource.com/chromium/src/+/ce9a2515fc22794d7beb2268eaae... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/ce9a2515fc22794d7beb2268eaae... |
