Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Description • 7 years ago

We should be able to use VP9 in MP4 in MSE+EME in ClearKey and probably Widevine.

	Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)
	Comment 2 • 7 years ago
mozreview-review

Comment on attachment 8835322 [details]
Bug 1338064 - Enable VP9 in MP4 for EME in Nightly.

https://reviewboard.mozilla.org/r/111008/#review112280

r+ as what's there looks good...

But I have some concerns below about some potential missing checks -- concerns which may well be unfounded as I didn't try to understand 100% of the huge pile of code, in which case ignore me!

::: dom/media/eme/MediaKeySystemAccess.cpp:341
(Diff revision 1)
>          { nsCString("video/mp4"), EME_CODEC_H264, MediaDrmProxy::AVC, &widevine.mMP4 },
>          { nsCString("audio/mp4"), EME_CODEC_AAC, MediaDrmProxy::AAC, &widevine.mMP4 },
>          { nsCString("video/webm"), EME_CODEC_VP8, MediaDrmProxy::VP8, &widevine.mWebM },
>          { nsCString("video/webm"), EME_CODEC_VP9, MediaDrmProxy::VP9, &widevine.mWebM},

Shouldn't you add a line in here, with video/mp4 and EME_CODEC_VP9? (I see it's in an Android section, maybe it's just not supported there?)

::: dom/media/eme/MediaKeySystemAccess.cpp:621
(Diff revision 1)
>      const bool isMP4 =
>        DecoderTraits::IsMP4SupportedType(containerType, aDiagnostics);

This will be false if `capabilities.mContentType` is "video/mp4; codecs=VP9", in which case you would have to handle this manually here! (Until DecoderTraits::IsMP4SupportedType is extended to allow vp9-in-mp4.)

::: dom/media/eme/MediaKeySystemAccess.cpp:669
(Diff revision 1)
>        if (isMP4) {
>          if (aCodecType == Audio) {
>            codecs.AppendElement(EME_CODEC_AAC);
>          } else if (aCodecType == Video) {
>            codecs.AppendElement(EME_CODEC_H264);

Ooh, this seems like a dangerous conclusion now, since `isMP4`&&`Video` could now be `EME_CODEC_VP9`.

However, the way I understand the code that follows, it's only faking a codec list because the original `codecs` list was empty, in which case it doesn't matter -- Right? Maybe a comment about this fakery would help dispell future confusion. ;-)

Though a bit later at line 737 this `codecs` list is given to CanDecryptAndDecode(). Could it be bad to give it the wrong codec(s)?

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

Comment on attachment 8835322 [details]
Bug 1338064 - Enable VP9 in MP4 for EME in Nightly.

https://reviewboard.mozilla.org/r/111008/#review112280

> Shouldn't you add a line in here, with video/mp4 and EME_CODEC_VP9? (I see it's in an Android section, maybe it's just not supported there?)

Will try a try push to see if VP9 in MP4 works on Android...

> This will be false if `capabilities.mContentType` is "video/mp4; codecs=VP9", in which case you would have to handle this manually here! (Until DecoderTraits::IsMP4SupportedType is extended to allow vp9-in-mp4.)

Oh oops, Jay fixed this in a later patch because I didn't get around to addressing this review comment!

> Ooh, this seems like a dangerous conclusion now, since `isMP4`&&`Video` could now be `EME_CODEC_VP9`.
> 
> However, the way I understand the code that follows, it's only faking a codec list because the original `codecs` list was empty, in which case it doesn't matter -- Right? Maybe a comment about this fakery would help dispell future confusion. ;-)
> 
> Though a bit later at line 737 this `codecs` list is given to CanDecryptAndDecode(). Could it be bad to give it the wrong codec(s)?

Bascially, this code is here because the user may specify "video/mp4", and we're supposed to assume some sort of "default" codec. I think at this stage it's reasonable to assume that "video/mp4" should imply H.264, and if someone means VP9 in MP4, they should actually specify it in the codecs param. If VP9 in MP4 takes off, we might need to revisit this decision in future!

	Chris Pearce [:cpearce (Not reading bugmail)] Assignee
	Comment 4 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5db8098a70565d5afc1454d524af68d3e73b5b
Bug 1338064 - Enable VP9 in MP4 for EME in Nightly. r=gerald

	Carsten Book [:Tomcat]
	Comment 5 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2d5db8098a70

	Eric Shepherd [:sheppy]
	Comment 6 • 7 years ago

Updated:

https://developer.mozilla.org/en-US/docs/Web/API/Encrypted_Media_Extensions_API

https://developer.mozilla.org/en-US/Firefox/Releases/54