1226450 - Report audio/video codecs used in HTMLMediaElement and WebAudio via telemetry

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

We should have telemetry to report how often various codecs are used in HTMLMediaElement and WebAudio in order to inform our decisions.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Add telemetry to report codec used

* Rely on the Reader setting mMimeType on the MediaInfo it produces after possibly updating it after the first frame has been decoded.
* For legacy readers (GStreamer, AndroidMediaReader, OmxReader) which don't easily expose what exact codec they're playing, just report the content type of the MediaResource, with a "resource" prefix so we can tell it's not an exact codec type.
* Distinguish between codecs in Ogg and codecs in WebM.
* Add a prefix for WebAudio, so we can distinguish usage of codecs in WebAudio decodeData vs usage in HTMLMediaElement.
* Report the foramt of WAV files.
* I don't bother trying to handle MSE changing the codec type at runtime. Seems hard and not likely to happen much.

r?jya for media.
r?vladan for telemetry.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aee1fb70d248

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Add telemetry to report codec used

I realised that once the WaveReader is moved over to being implemented in terms of MediaDataDemuxer/Decoder, it won't make sense to add the format to the mimeType. So same as before, but without the format on the Wave mime type.

We can report that by a separate telemetry probe if we really want that.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8689877 [details] [diff] [review]
Patch v2: Add telemetry to report codec used

Review of attachment 8689877 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2073,5 @@
>  }
>  
>  void
> +MediaDecoderStateMachine::ReportCodecTelemetry() const
> +{ 

trailing space

@@ +2097,5 @@
> +              ("Telemetry MEDIA_CODEC_USED= '%s'", codec.get()));
> +      Telemetry::Accumulate(Telemetry::ID::MEDIA_CODEC_USED, codec);
> +    }
> +  });
> +  AbstractThread::MainThread()->Dispatch(task.forget());

I think this would be better done in the MediaDecoder instead. It has access to all this information and all is done on the main thread.

This could be done in MediaDecoder::MetadataLoaded as we don't need to wait until the first frame is decoded. We know the codec type before that.

It would be much simpler and not require any modifications to any of the decoder or readers ; just the base MediaDecoder class

::: dom/media/webm/WebMDemuxer.cpp
@@ +375,2 @@
>        } else if (mAudioCodec == NESTEGG_CODEC_OPUS) {
> +        mInfo.mAudio.mMimeType = "audio/webm; codecs=opus";

I'm not too keen of having different mimetype for the same codec; but I can see why you would want that. Better granularity in reporting the type

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8689877 [details] [diff] [review]
> Patch v2: Add telemetry to report codec used
> 
> Review of attachment 8689877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +2073,5 @@
> >  }
> >  
> >  void
> > +MediaDecoderStateMachine::ReportCodecTelemetry() const
> > +{ 
> 
> trailing space
> 
> @@ +2097,5 @@
> > +              ("Telemetry MEDIA_CODEC_USED= '%s'", codec.get()));
> > +      Telemetry::Accumulate(Telemetry::ID::MEDIA_CODEC_USED, codec);
> > +    }
> > +  });
> > +  AbstractThread::MainThread()->Dispatch(task.forget());
> 
> I think this would be better done in the MediaDecoder instead. It has access
> to all this information and all is done on the main thread.

Sure.

> This could be done in MediaDecoder::MetadataLoaded as we don't need to wait
> until the first frame is decoded. We know the codec type before that.
> 
> It would be much simpler and not require any modifications to any of the
> decoder or readers ; just the base MediaDecoder class

We'd still need to modify the Readers which aren't MediaFormatReaders which we still use, as they don't yet fill out the mimetype fields of the MediaInfo.

> 
> ::: dom/media/webm/WebMDemuxer.cpp
> @@ +375,2 @@
> >        } else if (mAudioCodec == NESTEGG_CODEC_OPUS) {
> > +        mInfo.mAudio.mMimeType = "audio/webm; codecs=opus";
> 
> I'm not too keen of having different mimetype for the same codec; but I can
> see why you would want that. Better granularity in reporting the type

Precisely. We want to distinguish ogg/vorbis from webm/vorbis.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Comment on attachment 8689877 [details] [diff] [review]
> Patch v2: Add telemetry to report codec used
> 
> Review of attachment 8689877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +2073,5 @@
> >  }
> >  
> >  void
> > +MediaDecoderStateMachine::ReportCodecTelemetry() const
> > +{ 
> 
> trailing space
> 
> @@ +2097,5 @@
> > +              ("Telemetry MEDIA_CODEC_USED= '%s'", codec.get()));
> > +      Telemetry::Accumulate(Telemetry::ID::MEDIA_CODEC_USED, codec);
> > +    }
> > +  });
> > +  AbstractThread::MainThread()->Dispatch(task.forget());
> 
> I think this would be better done in the MediaDecoder instead. It has access
> to all this information and all is done on the main thread.
> 
> This could be done in MediaDecoder::MetadataLoaded as we don't need to wait
> until the first frame is decoded. We know the codec type before that.
> 
> It would be much simpler and not require any modifications to any of the
> decoder or readers ; just the base MediaDecoder class

I guess what you mean here is that the DirectShow, Wave, and Raw Readers are only created if the MediaResource's ContentType is known to be what is equivalent to the mime types I'm adding to the MediaInfo's mimeType. So we could refrain from adding them.

For Ogg, we specifically want to know what codecs are being used, so it makes sense to fill out these fields.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #6)

> I guess what you mean here is that the DirectShow, Wave, and Raw Readers are
> only created if the MediaResource's ContentType is known to be what is
> equivalent to the mime types I'm adding to the MediaInfo's mimeType. So we
> could refrain from adding them.
> 
> For Ogg, we specifically want to know what codecs are being used, so it
> makes sense to fill out these fields.

filling up the mimetype is fine. I'm more fuss about where the report is performed, and to me MediaDecoder is a better place, and the information being filled when reading the metadata, it can be done there instead of doing it after decoding the first frame

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3: Add telemetry to report codec used

With review comments addressed.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8690601 [details] [diff] [review]
Patch v3: Add telemetry to report codec used

Re-requesting review by vladan, now this patch has passed review by a media peer.

This patch reports what media codecs' mime types we play in HTML <audio> and <video> elements, and in WebAudio's decodeAudioData API.

Comment 10

[Vladan Djeric (:vladan)]

Comment on attachment 8690601 [details] [diff] [review]
Patch v3: Add telemetry to report codec used

Review of attachment 8690601 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/Histograms.json
@@ +8756,5 @@
>      "releaseChannelCollection": "opt-out"
>    },
> +  "MEDIA_CODEC_USED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",

* make it expire in 8 versions (~1 year), you can easily renew it then and you'll be notified by an automated email.
* add a bug_numbers field

@@ +8757,5 @@
>    },
> +  "MEDIA_CODEC_USED": {
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "keyed": true,

how many distinct keys are you expecting? and are they really going to be arbitrary strings? if it's less than a dozen, you should implement this probe as an enum histogram

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

On desktop the set of strings I expect we'd encounter are:

audio/mp4a-latm
video/avc
audio/mpeg
resource; audio/mpeg
resource; audio/mp3
video/ogg; codecs="theora"
audio/ogg; codecs="opus"
audio/ogg; codecs="vorbis"
video/webm; codecs="vp8"
video/webm; codecs="vp9"
audio/webm; codecs="opus"
audio/webm; codecs="vorbis"
resource; audio/x-wav
resource; audio/wav
resource; audio/wave
resource; audio/x-pn-wav
resource; video/x-raw
resource; video/x-raw-yuv
webaudio; audio/mp4a-latm
webaudio; audio/mpeg
webaudio; audio/mp3
webaudio;resource; audio/mpeg
webaudio;resource; audio/mp3
webaudio; audio/ogg; codecs="opus"
webaudio; audio/ogg; codecs="vorbis"
webaudio; audio/webm; codecs="opus"
webaudio; audio/webm; codecs="vorbis"
webaudio;resource; audio/x-wav
webaudio;resource; audio/wav
webaudio;resource; audio/wave
webaudio;resource; audio/x-pn-wav

On Linux if ffmpeg is not installed we use GStreamer, and then the set of types would be the above plus what's in the sContainers list in GStreamerFormatHelper.cpp.

On B2G and older versions of Android we support a wider range of mime types, so the set of strings increases; basically it's the above, plus whatever else is in gOmxTypes and gB2GOnlyTypes in DecoderTraits.cpp.

So unless there's a good reason to define an explicit enumeration, I'd prefer to report the strings. Is there a good reason?

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4:: Add telemetry to report codec used

bug_numbers and expires_in_version updated. I left the arbitrary strings being reported, as it would be onerous to enumerate and test all permutations on all platforms that we support...

Comment 13

[Jean-Yves Avenard [:jya]]

Worth noting that we are the one setting the strings; it doesn't come from random value and as such their number is limited.

Comment 14

[Vladan Djeric (:vladan)]

(In reply to Chris Pearce (:cpearce) from comment #11)
> So unless there's a good reason to define an explicit enumeration, I'd
> prefer to report the strings. Is there a good reason?

Enum histograms are simpler, so if possible, they're preferrable. It's easier to compare different "keys" in an enumerated histogram on a single telemetry.mozilla.org graph, they take less space to store in backend & Firefox, they're quicker for the backend to aggregate, quicker to retrieve for dashboarding, etc. In particular, we get backend performance problems if there are too many (e.g. 100+) distinct keys in a keyed histogram.

It seems like the # of possible keys in this histogram is finite, but it looks the # of keys could be large. Do you need to track each of these keys separately from each other or can you coalesce some of these keys?

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #14)
> (In reply to Chris Pearce (:cpearce) from comment #11)
> > So unless there's a good reason to define an explicit enumeration, I'd
> > prefer to report the strings. Is there a good reason?
> 
> Enum histograms are simpler, so if possible, they're preferrable. It's
> easier to compare different "keys" in an enumerated histogram on a single
> telemetry.mozilla.org graph, they take less space to store in backend &
> Firefox, they're quicker for the backend to aggregate, quicker to retrieve
> for dashboarding, etc. In particular, we get backend performance problems if
> there are too many (e.g. 100+) distinct keys in a keyed histogram.
> 
> It seems like the # of possible keys in this histogram is finite, but it
> looks the # of keys could be large. Do you need to track each of these keys
> separately from each other or can you coalesce some of these keys?

We can coalesce some of the keys. There are 2 mappings for mp3, 3 for Wav, and 2 for Raw. That would reduce the number of keys by 8 or so.

We do occasionally add codecs, so using a keyed histogram means we don't need to remember to update the telemetry.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/990e65f36d20

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/990e65f36d20