Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
271093002:
Add AudioInputControllerCaptureStartupSuccess UMA (Closed)
|
Created:
6 years, 7 months ago by no longer working on chromium Modified:
6 years, 7 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd AudioInputControllerCaptureStartupSuccess UMA.
This logs:
a boolean telling the AudioInputController is getting the recording data after an input stream was started.
This is symmetric to AudioOutputControllerPlaybackStartupSuccess on the output side.
NOTRY=true
BUG=357501
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269589
Patch Set 1 : #
Total comments: 10
Patch Set 2 : addressed the comments. #
Messages
Total messages: 22 (0 generated)
Tommi and Alexie, could you please review this CL? SX
lgtm
Thanks Alex. + Henrik in case Tommi does not have time.
https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:212: enable_nodata_timer = true; When was this guy enabled again? I had disabled it to avoid issues on Mac and now it is on again. What has happened?
On 2014/05/09 13:15:06, henrika wrote: > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > File media/audio/audio_input_controller.cc (right): > > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > media/audio/audio_input_controller.cc:212: enable_nodata_timer = true; > When was this guy enabled again? I had disabled it to avoid issues on Mac and > now it is on again. What has happened? It was modified to be used only for logging, and it does not shutdown the stream any more. For details, please look at: https://codereview.chromium.org/229573003 Or bug https://code.google.com/p/chromium/issues/detail?id=360756
To be specific, previously the timer would shutdown the input stream when detecting a no data error, and you disabled it for M35, Jiayang enabled it for M36 for pure logging, to allow us to know what happens if the device does not deliver recording data. And I am going to add a UMA stat here to know how often it happens. FYI, on the output side we have similar thing called AudioOutputControllerPlaybackStartupSuccess, it is also a timer.
https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:212: enable_nodata_timer = true; On 2014/05/09 13:15:07, henrika wrote: > When was this guy enabled again? I had disabled it to avoid issues on Mac and > now it is on again. What has happened? This was changed about a month ago: https://codereview.chromium.org/229573003 https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:324: GetDataIsActive()); How do you know that SetDataIsActive has been called at this point?
https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:324: GetDataIsActive()); On 2014/05/09 13:40:43, tommi wrote: > How do you know that SetDataIsActive has been called at this point? OnData() calls SetDataIsActive(true) when getting recording data. So if OnData() is called before the check, GetDataIsActive() return a true, which means the capture starts successfully. If GetDataIsActive() is a false, that means we do not get any OnData() after the stream has started. Let me know if I misunderstand things here.
I am perhaps missing something here but it feels as you have bot changed anything really. If so, can you clarify what is meant by First. Thanks. https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:256: // Method to check if getting recording data after a stream was started, Perhaps change to "Method which checks if we get recorded data ..."
https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:324: GetDataIsActive()); On 2014/05/09 13:48:22, xians1 wrote: > On 2014/05/09 13:40:43, tommi wrote: > > How do you know that SetDataIsActive has been called at this point? > > OnData() calls SetDataIsActive(true) when getting recording data. So if OnData() > is called before the check, GetDataIsActive() return a true, which means the > capture starts successfully. Yeah I know how it's supposed to work but I'm wondering how we make sure we don't report false positives. > If GetDataIsActive() is a false, that means we do not get any OnData() after the > stream has started. If we get this callback before OnData is called - but OnData gets called immediately after, then we will have incorrectly recorded that we capture is not working. I'm basically asking, is that not possible? and if so, how do we guarantee that it is not possible. > > Let me know if I misunderstand things here.
Let me rephrase my question. What would differ if you had added the call to UMA_HISTOGRAM_BOOLEAN in the original DoCheckForNotData() instead?
https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:324: GetDataIsActive()); On 2014/05/09 13:56:21, tommi wrote: > On 2014/05/09 13:48:22, xians1 wrote: > > On 2014/05/09 13:40:43, tommi wrote: > > > How do you know that SetDataIsActive has been called at this point? > > > > OnData() calls SetDataIsActive(true) when getting recording data. So if > OnData() > > is called before the check, GetDataIsActive() return a true, which means the > > capture starts successfully. > > Yeah I know how it's supposed to work but I'm wondering how we make sure we > don't report false positives. > > > If GetDataIsActive() is a false, that means we do not get any OnData() after > the > > stream has started. > > If we get this callback before OnData is called - but OnData gets called > immediately after, then we will have incorrectly recorded that we capture is not > working. I'm basically asking, is that not possible? and if so, how do we > guarantee that it is not possible. > > > > > Let me know if I misunderstand things here. > Actually, I think this is fine. The 5 second period should be more than enough to check this, so lgtm. One more thing to think about down the line. If we start seeing that GetDataIsActive returns 0 in our logs, what would be the next steps to look at? Also, see one request below. https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:373: SetDataIsActive(true); Can you move this flag to the top of the function, above where we set the state?
Get it now as well. LGTM
Thanks. Tommi, we can discuss the follow-up after getting some statistics. Wdyt? https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.cc (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.cc:373: SetDataIsActive(true); On 2014/05/09 14:46:15, tommi wrote: > Can you move this flag to the top of the function, above where we set the state? Done. https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... File media/audio/audio_input_controller.h (right): https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... media/audio/audio_input_controller.h:256: // Method to check if getting recording data after a stream was started, On 2014/05/09 13:49:34, henrika wrote: > Perhaps change to "Method which checks if we get recorded data ..." Done.
The CQ bit was checked by xians@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/271093002/40001
On 2014/05/09 15:35:07, xians1 wrote: > Thanks. > > Tommi, we can discuss the follow-up after getting some statistics. Wdyt? > > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > File media/audio/audio_input_controller.cc (right): > > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > media/audio/audio_input_controller.cc:373: SetDataIsActive(true); > On 2014/05/09 14:46:15, tommi wrote: > > Can you move this flag to the top of the function, above where we set the > state? > > Done. > > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > File media/audio/audio_input_controller.h (right): > > https://codereview.chromium.org/271093002/diff/20001/media/audio/audio_input_... > media/audio/audio_input_controller.h:256: // Method to check if getting > recording data after a stream was started, > On 2014/05/09 13:49:34, henrika wrote: > > Perhaps change to "Method which checks if we get recorded data ..." > > Done. sgtm
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
The CQ bit was unchecked by xians@chromium.org
The CQ bit was checked by xians@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/271093002/40001
Message was sent while issue was closed.
Change committed as 269589 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
