Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
486633003:
Only log audio input silence state UMA stats for low latency mode and if not fake device. (Closed)
|
Created:
6 years, 4 months ago by Henrik Grunell Modified:
6 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, henrika (OOO until Aug 14) Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionOnly log audio input silence state UMA stats for low latency mode and if not fake device.
The stats is intended for the low latency case, hence removing other cases.
NOTRY=true
BUG=410694
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291402
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 12
Patch Set 3 : Code review and a fix. #
Total comments: 5
Patch Set 4 : Code review + rebase. #Messages
Total messages: 15 (0 generated)
driven-by https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:291: no_data_timer_.reset(new base::Timer( could you please also change the code here, we would like to start the timer only if the controller for low latency stream. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:168: static scoped_refptr<AudioInputController> Create( FYI, this method is used by speech. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:185: static scoped_refptr<AudioInputController> CreateLowLatency( I think you should do the logic control here, and set a member flag to indicate the input controller is created for low latency stream. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:194: // Factory method for creating an AudioInputController for low-latency mode, :( confusing comment. It is definitely not the scope of this CL, would you mind adding a TODO and assign it to miu or simply changing it? https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:199: static scoped_refptr<AudioInputController> CreateForStream( FYI, this create method is used by cast. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:277: void DoCreateForLowLatency(AudioManager* audio_manager, Please see comments above, I don't think you need DoCreateForLowLatency().
Also did a fix. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:291: no_data_timer_.reset(new base::Timer( On 2014/08/21 09:11:56, xians1 wrote: > could you please also change the code here, we would like to start the timer > only if the controller for low latency stream. I think that's outside the scope of this CL. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:168: static scoped_refptr<AudioInputController> Create( On 2014/08/21 09:11:56, xians1 wrote: > FYI, this method is used by speech. Acknowledged. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:185: static scoped_refptr<AudioInputController> CreateLowLatency( On 2014/08/21 09:11:56, xians1 wrote: > I think you should do the logic control here, and set a member flag to indicate > the input controller is created for low latency stream. Not possible; it's a static function. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:194: // Factory method for creating an AudioInputController for low-latency mode, On 2014/08/21 09:11:56, xians1 wrote: > :( confusing comment. It is definitely not the scope of this CL, would you mind > adding a TODO and assign it to miu or simply changing it? Changed. I hope it's clearer. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:199: static scoped_refptr<AudioInputController> CreateForStream( On 2014/08/21 09:11:56, xians1 wrote: > FYI, this create method is used by cast. Acknowledged. https://codereview.chromium.org/486633003/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:277: void DoCreateForLowLatency(AudioManager* audio_manager, On 2014/08/21 09:11:56, xians1 wrote: > Please see comments above, I don't think you need DoCreateForLowLatency(). See comment above.
https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:243: void AudioInputController::DoCreateForLowLatency(AudioManager* audio_manager, why do we need a new function? Can't we just add the params.format() check to DoCreate instead? https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:567: DCHECK(silence_state_ == SILENCE_STATE_ONLY_SILENCE || {} (and subsequently add {} to all cases)
https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:243: void AudioInputController::DoCreateForLowLatency(AudioManager* audio_manager, On 2014/08/21 18:06:58, tommi wrote: > why do we need a new function? Can't we just add the params.format() check to > DoCreate instead? So that it's only logged in the low latency case. I think we want to exclude e.g. web speech. I don't see a way to tell the difference in DoCreate, it seems to only be the sync writer that differs in the two CreateXyz functions. https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:567: DCHECK(silence_state_ == SILENCE_STATE_ONLY_SILENCE || On 2014/08/21 18:06:58, tommi wrote: > {} (and subsequently add {} to all cases) Done.
https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/486633003/diff/40001/media/audio/audio_input_... media/audio/audio_input_controller.cc:243: void AudioInputController::DoCreateForLowLatency(AudioManager* audio_manager, On 2014/08/22 08:24:09, Henrik Grunell wrote: > On 2014/08/21 18:06:58, tommi wrote: > > why do we need a new function? Can't we just add the params.format() check to > > DoCreate instead? > > So that it's only logged in the low latency case. I think we want to exclude > e.g. web speech. I don't see a way to tell the difference in DoCreate, it seems > to only be the sync writer that differs in the two CreateXyz functions. Ah, now I chatted with Shijing offline and I understand that there's a low latency mode in the params, which yields 4 combinations (together with syncwriter or not). I wasn't aware of this before. So it's a bit more complicated. I think we should only exclude fake devices then. SG Tommi?
sg. lgtm.
On 2014/08/22 09:55:41, tommi wrote: > sg. lgtm. This refers to our offline discussion, were we decided to exclude the fake device and non-syncwriter cases. I.e. keep the CL as in patch set #4.
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/486633003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by grunell@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grunell@chromium.org/486633003/60001
Message was sent while issue was closed.
Committed patchset #4 (60001) as 291402 |
