Adding Media.InputStreamDuration to UMA histograms for low-latency input audio streams

Adding Media.InputStreamDuration to UMA histograms for low-latency input audio streams.

My idea was that the histogram could show if we do a very large number of short
calls and how the length of the streams are related to the length of
AudioTracks. If so, we might have issues in WebRTC clients or in how we
create/destroy the input streams.

BUG=405449
TEST=Manual testing using WebRTC clients.

Committed: https://crrev.com/f3a8a0562e1ff412ee0d3a58ab119e92b787fd07
Cr-Commit-Position: refs/heads/master@{#293496}

[henrika (OOO until Aug 14), 2014-09-03 13:36:33]

henrika@chromium.org changed reviewers:
+ grunell@chromium.org, tommi@chromium.org

[henrika (OOO until Aug 14), 2014-09-03 13:38:01]

Adding more UMA where I measure the total length of each low-latency input
stream. Goal is to see the distribution and possible find that we create too
many very short streams.

[henrika (OOO until Aug 14), 2014-09-03 13:38:31]

Plan was to add the XML-part when this initial phase is done.

[tommi (sloooow) - chröme, 2014-09-03 13:47:19]

On 2014/09/03 13:38:31, henrika wrote:
> Plan was to add the XML-part when this initial phase is done.

Is there a benefit with waiting and doing two reviews?

[tommi (sloooow) - chröme, 2014-09-03 13:50:03]

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:358: std::ostringstream oss;
use StringPrintf (stringstream should only be used for logging)

[henrika (OOO until Aug 14), 2014-09-03 13:50:19]

I did not want to add XML resource if you guys are not OK the basic idea of
adding the stat.

[tommi (sloooow) - chröme, 2014-09-03 13:54:29]

ah.  can you add to the description what the benefit will be?  If this will help
us understand usage or fix a problem, then that sounds like a good thing.  If
it's just noise, then perhaps not :)

[henrika (OOO until Aug 14), 2014-09-03 14:01:57]

LOL, well, I try not to add noise ;-)

My idea was that the histogram could show if we do a very large number of short
calls and how the length of the streams are related to the length of
AudioTracks. If so, we might have issues in WebRTC clients or in how we
create/destroy the input streams.

[tommi (sloooow) - chröme, 2014-09-03 14:43:06]

On 2014/09/03 14:01:57, henrika wrote:
> LOL, well, I try not to add noise ;-)
> 
> My idea was that the histogram could show if we do a very large number of
short
> calls and how the length of the streams are related to the length of
> AudioTracks. If so, we might have issues in WebRTC clients or in how we
> create/destroy the input streams.

great.  Sounds like a good thing to me.  Thanks for updating the description.

[Henrik Grunell, 2014-09-04 06:32:18]

What's the "XML part"?

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:356:
UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
I think it should be called Media.AudioInputController.InputStreamDuration.

[Henrik Grunell, 2014-09-04 08:32:08]

On 2014/09/04 06:32:18, Henrik Grunell wrote:
> What's the "XML part"?

Haha, you mean histograms.xml. :) Sorry, was thinking too big.

>
https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
> File media/audio/audio_input_controller.cc (right):
> 
>
https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
> media/audio/audio_input_controller.cc:356:
> UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
> I think it should be called Media.AudioInputController.InputStreamDuration.

Talked offline, I'm fine with both. There seems to be different schools when it
comes the naming. I'll defer to the histograms.xml reviewer.

[Henrik Grunell, 2014-09-04 08:32:35]

LGTM

[henrika (OOO until Aug 14), 2014-09-04 12:27:37]

Comments and questions only.

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:356:
UMA_HISTOGRAM_LONG_TIMES("Media.InputStreamDuration", duration);
Discussed off-line. Henrik G is OK with excluding the AudioInputController part.

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:358: std::ostringstream oss;
I tried to use it but was not able to come up with a nice way to make it accept
the int64 on all platforms. Any idea which format specifier I could use?

May I ask why stringstream is not OK here?

[henrika (OOO until Aug 14), 2014-09-04 12:42:43]

henrika@chromium.org changed reviewers:
+ asvitkine@chromium.org

[henrika (OOO until Aug 14), 2014-09-04 12:43:27]

Adding asvitkine@ for input on the XML part. Thanks for checking.

[henrika (OOO until Aug 14), 2014-09-04 12:54:13]

Fix based on feedback from Tommi.

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:358: std::ostringstream oss;
On 2014/09/04 12:27:37, henrika wrote:
> I tried to use it but was not able to come up with a nice way to make it
accept
> the int64 on all platforms. Any idea which format specifier I could use?
> 
> May I ask why stringstream is not OK here?

Anyhow, fixed now ;-)

[Alexei Svitkine (slow), 2014-09-04 12:56:23]

lgtm

[tommi (sloooow) - chröme, 2014-09-04 16:53:21]

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/537573002/diff/1/media/audio/audio_input_cont...
media/audio/audio_input_controller.cc:358: std::ostringstream oss;
On 2014/09/04 12:27:37, henrika wrote:
> I tried to use it but was not able to come up with a nice way to make it
accept
> the int64 on all platforms. Any idea which format specifier I could use?
> 
> May I ask why stringstream is not OK here?

It's because of the style guide.  stringstream is OK for logging only and from
discussing this with others, the interpretation is that logging is debug logging
(i.e. [D]LOG etc) and this falls outside of that since it could easily start
creeping into non-logging code such as functions etc.

[tommi (sloooow) - chröme, 2014-09-04 16:54:01]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2014-09-04 16:54:01]

lgtm

[commit-bot: I haz the power, 2014-09-04 17:02:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/henrika@chromium.org/537573002/40001

[commit-bot: I haz the power, 2014-09-04 18:31:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-04 18:31:16]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[henrika (OOO until Aug 14), 2014-09-05 09:29:45]

The CQ bit was checked by henrika@chromium.org

[commit-bot: I haz the power, 2014-09-05 09:30:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/henrika@chromium.org/537573002/60001

[commit-bot: I haz the power, 2014-09-05 10:35:32]

Committed patchset #4 (id:60001) as efab4b23c4a30c37c25c42259ec5d410a882ed15

[commit-bot: I haz the power, 2014-09-10 03:38:41]

Patchset 4 (id:??) landed as
https://crrev.com/f3a8a0562e1ff412ee0d3a58ab119e92b787fd07
Cr-Commit-Position: refs/heads/master@{#293496}