Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
200293002:
added uma stats to check the usage of media stream audio track audio processing (Closed)
|
Created:
6 years, 9 months ago by no longer working on chromium Modified:
6 years, 9 months ago CC:
chromium-reviews, jar (doing other things), fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, wjia+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptionadded uma stats to check the usage of media stream audio track audio processing.
NOTRY=true
BUG=264611
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259222
Patch Set 1 #
Total comments: 2
Patch Set 2 : added AUDIO_PROCESSING_IN_WEBRTC #
Total comments: 6
Patch Set 3 : addressed Alexei's comments and changed to kAudioProcessingNumberOfChannels #
Total comments: 2
Patch Set 4 : fixed the indentation and EXPECT_EQ #
Messages
Total messages: 23 (0 generated)
Hi, could you please take a look? Thanks, SX
https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... content/renderer/media/media_stream_audio_processor.cc:333: AUDIO_PROCESSING_DISABLED, AUDIO_PROCESSING_MAX); Could we have a different value for this case? Here we're actually still running the APM in Chrome, but the constraints specify that no APM should be done (right?). So I think that by putting this case in the same bucket as 'disabled' could skew the numbers (as the APM code path is actually enabled). Maybe AUDIO_PROCESSING_BYPASSED?
https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... content/renderer/media/media_stream_audio_processor.cc:333: AUDIO_PROCESSING_DISABLED, AUDIO_PROCESSING_MAX); On 2014/03/14 14:38:32, tommi wrote: > Could we have a different value for this case? > > Here we're actually still running the APM in Chrome, but the constraints specify > that no APM should be done (right?). So I think that by putting this case in > the same bucket as 'disabled' could skew the numbers (as the APM code path is > actually enabled). > > Maybe AUDIO_PROCESSING_BYPASSED? Do we really want this? AUDIO_PROCESSING_BYPASSED will soon go away, in one month? And it probably does not tell much. Note that we can't keep two paths for long, the code has to go away then we can do a thorough clean up.
One month is perhaps a tad optimistic. But isn't it AUDIO_PROCESSING_DISABLED that will go away? AUDIO_PROCESSING_BYPASSED will continue to exist, no? On 2014/03/14 14:43:55, xians1 wrote: > https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... > File content/renderer/media/media_stream_audio_processor.cc (right): > > https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... > content/renderer/media/media_stream_audio_processor.cc:333: > AUDIO_PROCESSING_DISABLED, AUDIO_PROCESSING_MAX); > On 2014/03/14 14:38:32, tommi wrote: > > Could we have a different value for this case? > > > > Here we're actually still running the APM in Chrome, but the constraints > specify > > that no APM should be done (right?). So I think that by putting this case in > > the same bucket as 'disabled' could skew the numbers (as the APM code path is > > actually enabled). > > > > Maybe AUDIO_PROCESSING_BYPASSED? > > Do we really want this? AUDIO_PROCESSING_BYPASSED will soon go away, in one > month? And it probably does not tell much. > Note that we can't keep two paths for long, the code has to go away then we can > do a thorough clean up.
On 2014/03/14 14:49:29, tommi wrote: > One month is perhaps a tad optimistic. > But isn't it AUDIO_PROCESSING_DISABLED that will go away? > AUDIO_PROCESSING_BYPASSED will continue to exist, no? > > On 2014/03/14 14:43:55, xians1 wrote: > > > https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... > > File content/renderer/media/media_stream_audio_processor.cc (right): > > > > > https://codereview.chromium.org/200293002/diff/1/content/renderer/media/media... > > content/renderer/media/media_stream_audio_processor.cc:333: > > AUDIO_PROCESSING_DISABLED, AUDIO_PROCESSING_MAX); > > On 2014/03/14 14:38:32, tommi wrote: > > > Could we have a different value for this case? > > > > > > Here we're actually still running the APM in Chrome, but the constraints > > specify > > > that no APM should be done (right?). So I think that by putting this case > in > > > the same bucket as 'disabled' could skew the numbers (as the APM code path > is > > > actually enabled). > > > > > > Maybe AUDIO_PROCESSING_BYPASSED? > > > > Do we really want this? AUDIO_PROCESSING_BYPASSED will soon go away, in one > > month? And it probably does not tell much. > > Note that we can't keep two paths for long, the code has to go away then we > can > > do a thorough clean up. What are the reasons for not adding another enum value btw? Another option could be to log the first case as AUDIO_PROCESSING_IN_WEBRTC to make a clear distinction and then keep on using the 'enabled'/'disabled' flags to map to user intent (constraints).
Hi Tommi, I have already added AUDIO_PROCESSING_IN_WEBRTC, PTAL. SX
lgtm https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:34: const int kAudioProcessingNumberOfChannel = 1; nit: kAudioProcessingNumberOfChannels (plural) I know that the number is currently 1, but "number of channel" doesn't mean the same thing as "number of channels" (plural) which is what you want here.
Hi Alexei, could you please take a look? Thanks SX
https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:38: enum AudioTrackProcessingStates { Nit: Add a comment that this is used for an UMA histograms and entries shouldn't re-ordered or removed. https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:378: UMA_HISTOGRAM_ENUMERATION("Media.AudioTrackProcessingStates", Nit: Can you make a function in the anon namespace that takes an enum value and logs this? Each macro use bloats the binary a bit, so it's better to consolidate ones for the same histogram.
https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:34: const int kAudioProcessingNumberOfChannel = 1; On 2014/03/17 16:00:39, tommi wrote: > nit: kAudioProcessingNumberOfChannels (plural) > > I know that the number is currently 1, but "number of channel" doesn't mean the > same thing as "number of channels" (plural) which is what you want here. Done. https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:38: enum AudioTrackProcessingStates { On 2014/03/17 22:15:50, Alexei Svitkine wrote: > Nit: Add a comment that this is used for an UMA histograms and entries shouldn't > re-ordered or removed. Done. https://codereview.chromium.org/200293002/diff/30001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:378: UMA_HISTOGRAM_ENUMERATION("Media.AudioTrackProcessingStates", On 2014/03/17 22:15:50, Alexei Svitkine wrote: > Nit: Can you make a function in the anon namespace that takes an enum value and > logs this? Each macro use bloats the binary a bit, so it's better to consolidate > ones for the same histogram. Done.
Alexei, PTAL. Thanks, SX
lgtm with a nit https://codereview.chromium.org/200293002/diff/50001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:380: 0); Nit: Bad alignment. Also, the expected value should be the first param.
Thanks, I am going to land it now. https://codereview.chromium.org/200293002/diff/50001/content/renderer/media/m... File content/renderer/media/media_stream_audio_processor.cc (right): https://codereview.chromium.org/200293002/diff/50001/content/renderer/media/m... content/renderer/media/media_stream_audio_processor.cc:380: 0); On 2014/03/19 13:59:11, Alexei Svitkine wrote: > Nit: Bad alignment. > > Also, the expected value should be the first param. 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/200293002/70001
The CQ bit was unchecked by xians@chromium.org
The CQ bit was checked by xians@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
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/200293002/70001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/200293002/70001
Message was sent while issue was closed.
Change committed as 259222 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
