media: Record counts of detected src= audio, video, and text tracks

media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
Committed: https://crrev.com/75faefc2e0e80b50d20ec597bbb5767cb863fa4d
Cr-Commit-Position: refs/heads/master@{#382436}

[wolenetz, 2016-03-19 00:24:29]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-19 00:25:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811413004/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811413004/1

[wolenetz, 2016-03-19 00:27:07]

wolenetz@chromium.org changed reviewers:
+ dalecurtis@chromium.org, holte@chromium.org

[wolenetz, 2016-03-19 00:27:08]

Please take a look. This data will help us get a better understanding of where
src= media contains multitrack A / V / T.

dalecurtis@ : all
holte@: histograms.xml

[wolenetz, 2016-03-19 00:43:47]

On 2016/03/19 00:27:08, wolenetz wrote:
> Please take a look. This data will help us get a better understanding of where
> src= media contains multitrack A / V / T.
> 
> dalecurtis@ : all
> holte@: histograms.xml

s/where/"if"/

[DaleCurtis, 2016-03-19 00:48:40]

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:121: int32_t capped_audio =
Why cap? Lets also just use int everywhere for sanity.

[DaleCurtis, 2016-03-19 00:49:17]

Also how useful is it to know the counts w/o the types? Should we add a new
DetectedTrackCodecs?

[commit-bot: I haz the power, 2016-03-19 02:55:42]

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

[commit-bot: I haz the power, 2016-03-19 02:55:43]

Dry run: This issue passed the CQ dry run.

[wolenetz, 2016-03-21 18:30:45]

On 2016/03/19 00:49:17, DaleCurtis wrote:
> Also how useful is it to know the counts w/o the types? Should we add a new
> DetectedTrackCodecs?

The main thing we're looking for here is how many src= playbacks include
multiple tracks for a single type (where type=audio or video or text).
DetectedTrackCodecs sounds very similar to Media.Detected{Audio,Video}CodecHash,
which we have already (and should be corrected by this CL to cover all in each
src= playback attempt).
Getting a handle on the distribution of codecs (or even track.Kinds) across a
single .src= playback is harder.
For example, how would we report:
track 1 = opus audio
track 2 = vorbis audio
track 3 = vorbis audio

Right now, we'd report the codec hash for just opus audio (since we incorrectly
short-circuited and didn't report the remainder once we found a supported track,
per type).
With this CL, now we'd report opus *1 + vorbis * 2, plus audio track count = 3.
Are you asking for some additional ability to say something like:
audio track count=2*vorbis + 1*opus?
If so, I'm not sure how that can be represented into UMA.
Or are you suggesting we make "type" more fine-grained than audio/video/text,
and add a count per-codec hash? If so, I think we'd still need this CL's current
version of reporting to at least understand how many playbacks contain multiple
video tracks.
Maybe we should chat briefly so I can understand your comment better :)

[wolenetz, 2016-03-21 18:36:02]

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:121: int32_t capped_audio =
On 2016/03/19 00:48:39, DaleCurtis wrote:
> Why cap? Lets also just use int everywhere for sanity.

I was unable to find good documentation on how UMA determines bucket boundaries:
if we used the default "100 buckets", would counts of 0, 1 and >1 tracks be
bucketed together (that would be bad)? I'll see if I can get better information
about the boundary logic to see if I can indeed remove the caps here.

[Steven Holte, 2016-03-21 20:01:58]

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:121: int32_t capped_audio =
Basically, the bucket generation divides up the range between underflow and
overflow into the requested number of buckets, and tries to keep the buckets a
log-constant width (i.e 50-100 has same log width as 500-1000), but rounds
bucket boundaries down to nearest integer, and has minimum bucket width of 1.

Underflow==0 is the same as underflow==1, and you'll have [0,1) range bucket
either way.

There is a more detailed description at the top of this file:
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

The code that generates buckets is here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

[wolenetz, 2016-03-21 20:57:53]

Patchset #2 (id:20001) has been deleted

[wolenetz, 2016-03-21 21:01:12]

Description was changed from

==========
media: Record counts of detected src= audio, video, and text tracks

Recorded counts of each track type are capped at 5.
Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
==========

to

==========
media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
==========

[wolenetz, 2016-03-21 21:03:46]

Please take a look at patch set 2 (same R= mapping as before).
dalecurtis@ : please also see my earlier response to your "Should we add a new
DetectedTrackCodecs?" question. tl;dr: Don't we already have that in
Detected{Audio,Video}CodecHash ?

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1811413004/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:121: int32_t capped_audio =
On 2016/03/21 20:01:58, Steven Holte wrote:
> Basically, the bucket generation divides up the range between underflow and
> overflow into the requested number of buckets, and tries to keep the buckets a
> log-constant width (i.e 50-100 has same log width as 500-1000), but rounds
> bucket boundaries down to nearest integer, and has minimum bucket width of 1.
> 
> Underflow==0 is the same as underflow==1, and you'll have [0,1) range bucket
> either way.
> 
> There is a more detailed description at the top of this file:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...
> 
> The code that generates buckets is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/metrics/histo...

Thanks for the pointers.
int everywhere: done.
Cap at 5? Changed to 100. >=100 is probably fine in one bucket. We're most
interested in 0,1,>1 tri-state, but further information in the >1 section could
also be useful.

[wolenetz, 2016-03-21 21:04:05]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 21:04:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811413004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811413004/40001

[DaleCurtis, 2016-03-21 21:10:53]

We don't want DetectedAudio/Video codec hash used for this purpose. Those
histograms should remain for the selected codec and not for all codecs in the
container.

[wolenetz, 2016-03-21 21:21:56]

On 2016/03/21 21:10:53, DaleCurtis wrote:
> We don't want DetectedAudio/Video codec hash used for this purpose. Those
> histograms should remain for the selected codec and not for all codecs in the
> container.

I see now what you mean.

I also see that in current Chromium code, DetectedAudio/Video codec hash is
reported "// Log the codec detected, whether it is supported or not.", and is
reported for *all* streams until we find one we do support. IIUC, this is
incorrect. Rather, DetectedAudio/VideoCodecHash should only log *if* we support
it and use it. And a new set of DetectedTrackAudio/Video/TextCodecHash should
log for every stream found by ffmpeg, regardless of support or use by our
pipeline. Is this correct?

[wolenetz, 2016-03-21 21:24:39]

On 2016/03/21 21:21:56, wolenetz wrote:
> On 2016/03/21 21:10:53, DaleCurtis wrote:
> > We don't want DetectedAudio/Video codec hash used for this purpose. Those
> > histograms should remain for the selected codec and not for all codecs in
the
> > container.
> 
> I see now what you mean.
> 
> I also see that in current Chromium code, DetectedAudio/Video codec hash is
> reported "// Log the codec detected, whether it is supported or not.", and is
> reported for *all* streams until we find one we do support. IIUC, this is
> incorrect. Rather, DetectedAudio/VideoCodecHash should only log *if* we
support
> it and use it. And a new set of DetectedTrackAudio/Video/TextCodecHash should
> log for every stream found by ffmpeg, regardless of support or use by our
> pipeline. Is this correct?

(See also current
https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/ffmp...
which shows how a "detected audio/video codec hash" might be recorded for a
stream for which we later find out we don't support.)

[DaleCurtis, 2016-03-21 21:27:19]

Ah, I also didn't realize these were logging all detected codecs and not the
chosen ones. I guess the selected codec is already logged in Media.VideoCodec
and Media.AudioCodec?

[wolenetz, 2016-03-21 21:39:13]

On 2016/03/21 21:27:19, DaleCurtis wrote:
> Ah, I also didn't realize these were logging all detected codecs and not the
> chosen ones. I guess the selected codec is already logged in Media.VideoCodec
> and Media.AudioCodec?

Correct. (I also missed we already log the explicit codec enum, not even a hash,
for each audio/video that we actually use).
So, this CL fixes the DetectedAudio/VideoCodecHash to log for *all*, not just
those up and until we find a codec for audio/video that we support. It also adds
track counting for audio/video/text.
I can include some comment in the histograms.xml for
DetectedAudio/VideoCodecHash <summary> to mention that, as of this CL (M51),
these counts are more correct/complete.
Outside of that histograms.xml update, do you see anything else needing fixing
with patch set 2?

[DaleCurtis, 2016-03-21 21:42:03]

lgtm

[wolenetz, 2016-03-21 21:46:08]

wolenetz@chromium.org changed reviewers:
+ jrummell@chromium.org

[wolenetz, 2016-03-21 21:46:09]

Thanks Dale.
R+=jrummell@ for the histograms.xml <summary> updates for each of
DetectedAudio/VideoCodecHashes.

[jrummell, 2016-03-21 21:56:54]

lgtm

[wolenetz, 2016-03-21 22:25:29]

The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-21 22:25:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811413004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811413004/60001

[Steven Holte, 2016-03-21 22:52:36]

lgtm

[wolenetz, 2016-03-21 22:53:35]

The CQ bit was unchecked by wolenetz@chromium.org

[wolenetz, 2016-03-21 22:53:41]

The CQ bit was checked by wolenetz@chromium.org

[wolenetz, 2016-03-21 22:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1811413004/#ps60001
(title: "Note the fix to DetectedAudio/VideoCodecHash UMA in their histogram
summaries")

[commit-bot: I haz the power, 2016-03-21 22:54:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811413004/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811413004/60001

[commit-bot: I haz the power, 2016-03-21 23:37:37]

Description was changed from

==========
media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
==========

to

==========
media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
==========

[commit-bot: I haz the power, 2016-03-21 23:37:38]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-21 23:38:41]

Description was changed from

==========
media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128
==========

to

==========
media: Record counts of detected src= audio, video, and text tracks

Also fixes Detected{Audio,Video}CodecHash stats reporting to be done
respectively by type for all streams, not just those up to and including
one the player supports.

BUG=595128

Committed: https://crrev.com/75faefc2e0e80b50d20ec597bbb5767cb863fa4d
Cr-Commit-Position: refs/heads/master@{#382436}
==========

[commit-bot: I haz the power, 2016-03-21 23:38:42]

Patchset 3 (id:??) landed as
https://crrev.com/75faefc2e0e80b50d20ec597bbb5767cb863fa4d
Cr-Commit-Position: refs/heads/master@{#382436}