[EME] Enable VP9 in MP4 for EME in Nightly

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

We should be able to use VP9 in MP4 in MSE+EME in ClearKey and probably Widevine.
Comment hidden (mozreview-request)

Comment 2

2 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)?
Attachment #8835322 - Flags: review?(gsquelart) → review+
(Assignee)

Updated

2 years ago
Blocks: 1339204
(Assignee)

Comment 3

2 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!

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d5db8098a70
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.