Closed Bug 1279077 Opened 5 years ago Closed 4 years ago

Add WebM EME support for Widevine

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: cpeterson, Assigned: bryce)

References

Details

(Keywords: feature)

Attachments

(1 file)

Adding WebM support for Widevine depends on Bryce's WebM ClearKey work in bug 1257716.
Priority: -- → P2
Bryce said he is working on this bug.
Assignee: nobody → bvandyk
Keywords: feature
Update handling of VP8, VP9 and vorbis codecs to enable EME via widevine.

Currently not working with payed content on youtube/google play.

Review commit: https://reviewboard.mozilla.org/r/62560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62560/
Attachment #8768236 - Flags: review?(cpearce)
Pushing WIP to review for feedback.
Mass change P2 -> P3
Priority: P2 → P3
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

https://reviewboard.mozilla.org/r/62560/#review61116

Nothing major here. Will do a more thorough review when everything is working.
Attachment #8768236 - Flags: review?(cpearce)
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/1-2/
Attachment #8768236 - Attachment description: Bug 1279077 - WIP: Update GMP/EME path for webm. → Bug 1279077 - Update GMP/EME path to support webm.
Attachment #8768236 - Flags: review?(cpearce)
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

https://reviewboard.mozilla.org/r/62560/#review64630

::: dom/media/gmp/widevine-adapter/WidevineDecryptor.cpp:89
(Diff revision 2)
> +  } else if (!strcmp(aInitDataType, "keyids")) {
> +    initDataType = kKeyIds;
> +  } else {
> +    // Invalid init data type
> +    char* errorMsg = "Invalid init data type when creating session.";
> +    OnRejectPromise(aPromiseId, kNotSupportedError, 0, errorMsg, sizeof(errorMsg));

You should return at the end of the "invalid init data type" branch, so you avoid creating the session of invalid type.

::: dom/media/gmp/widevine-adapter/WidevineVideoDecoder.cpp:116
(Diff revision 2)
>    InputBuffer sample;
>  
>    RefPtr<MediaRawData> raw(new MediaRawData(aInputFrame->Buffer(), aInputFrame->Size()));
>    raw->mExtraData = mExtraData;
>    raw->mKeyframe = (aInputFrame->FrameType() == kGMPKeyFrame);
>    // Convert input from AVCC, which GMPAPI passes in, to AnnexB, which

I think it makes sense for the comment to be inside the branch, as we're only doing what the comment says on the branch path, right?

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:103
(Diff revision 2)
>  
>  void
>  VideoCallbackAdapter::Terminated()
>  {
>    // Note that this *may* be called from the proxy thread also.
> -  NS_WARNING("H.264 GMP decoder terminated.");
> +  NS_WARNING("GMP decoder terminated.");

"GMP video decoder terminated"

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:251
(Diff revision 2)
> -  codec.mCodecType = kGMPVideoCodecH264;
> -  codec.mWidth = mConfig.mImage.width;
> -  codec.mHeight = mConfig.mImage.height;
> -
>    nsTArray<uint8_t> codecSpecific;
> +  if (mConfig.mMimeType.EqualsLiteral("video/avc") ||

In a follow up bug, can you find all the places that we do these two string comparisons to identify H.264, and replace them all with a call to a helper function that does these comparisons?

That way, we're centralizing the string comparison that we use to identify H.264? This will reduce mistakes causing errors. Thanks.

I suppose putting the helper as a static function on MP4Demuxer makes the most sense.

In the same follow up, you can also dig up all the string comparisons that we're doing for "video/webm; codecs=vp8" and "video/webm; codecs=vp9" use VPXDecoder::IsVPX() instead.

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:257
(Diff revision 2)
> +      mConfig.mMimeType.EqualsLiteral("video/mp4")) {
> +    codec.mCodecType = kGMPVideoCodecH264;
> -  codecSpecific.AppendElement(0); // mPacketizationMode.
> +    codecSpecific.AppendElement(0); // mPacketizationMode.
> -  codecSpecific.AppendElements(mConfig.mExtraData->Elements(),
> +    codecSpecific.AppendElements(mConfig.mExtraData->Elements(),
> -                               mConfig.mExtraData->Length());
> +                                 mConfig.mExtraData->Length());
> +  } else if (mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp8")) {

Please use VPXDecoder::IsVPX() for these, and all other checks for VP8/9 in your patch. Let's stop comparing against the raw strings.

We're going to add VP9 in MP4. When we do, we don't want to have to change the strings here, and everywhere else.

Feel free to add a VPXDecoder::IsVP8(const nsACString&) and VPXDecoder::IsVP9(const nsACString&) for convenience.
Attachment #8768236 - Flags: review?(cpearce) → review+
https://reviewboard.mozilla.org/r/62560/#review64630

> I think it makes sense for the comment to be inside the branch, as we're only doing what the comment says on the branch path, right?

Agreed.

> In a follow up bug, can you find all the places that we do these two string comparisons to identify H.264, and replace them all with a call to a helper function that does these comparisons?
> 
> That way, we're centralizing the string comparison that we use to identify H.264? This will reduce mistakes causing errors. Thanks.
> 
> I suppose putting the helper as a static function on MP4Demuxer makes the most sense.
> 
> In the same follow up, you can also dig up all the string comparisons that we're doing for "video/webm; codecs=vp8" and "video/webm; codecs=vp9" use VPXDecoder::IsVPX() instead.

https://bugzilla.mozilla.org/show_bug.cgi?id=1290284
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/2-3/
Attachment #8768236 - Flags: review+ → review?(cpearce)
Requesting review for changes to MediaKeySystemAccess.cpp -- Removing commented code to enable Webm Widevine.
Attachment #8768236 - Flags: review?(cpearce) → review+
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

https://reviewboard.mozilla.org/r/62560/#review64892

r+ with IsVPx() helper function used.

::: dom/media/platforms/agnostic/eme/EMEVideoDecoder.cpp:42
(Diff revision 3)
>  {
> +  VideoInfo config = GetConfig();
> +  if (config.mMimeType.EqualsLiteral("video/avc") ||
> +      config.mMimeType.EqualsLiteral("video/mp4")) {
> -  aTags.AppendElement(NS_LITERAL_CSTRING("h264"));
> +    aTags.AppendElement(NS_LITERAL_CSTRING("h264"));
> +  } else if (config.mMimeType.EqualsLiteral("video/webm; codecs=vp8")) {

VPXDecoder::IsVP8()

::: dom/media/platforms/agnostic/eme/EMEVideoDecoder.cpp:44
(Diff revision 3)
> +  if (config.mMimeType.EqualsLiteral("video/avc") ||
> +      config.mMimeType.EqualsLiteral("video/mp4")) {
> -  aTags.AppendElement(NS_LITERAL_CSTRING("h264"));
> +    aTags.AppendElement(NS_LITERAL_CSTRING("h264"));
> +  } else if (config.mMimeType.EqualsLiteral("video/webm; codecs=vp8")) {
> +    aTags.AppendElement(NS_LITERAL_CSTRING("vp8"));
> +  } else if (config.mMimeType.EqualsLiteral("video/webm; codecs=vp9")) {

VPXDecoder::IsVP9()

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:52
(Diff revision 3)
>  already_AddRefed<MediaDataDecoder>
>  GMPDecoderModule::CreateVideoDecoder(const CreateDecoderParams& aParams)
>  {
> -  if (!aParams.mConfig.mMimeType.EqualsLiteral("video/avc")) {
> +  if (!aParams.mConfig.mMimeType.EqualsLiteral("video/avc") &&
> +      !aParams.mConfig.mMimeType.EqualsLiteral("video/mp4") &&
> +      !aParams.mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp8") &&

VPXDecoder::IsVP8()

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:53
(Diff revision 3)
>  GMPDecoderModule::CreateVideoDecoder(const CreateDecoderParams& aParams)
>  {
> -  if (!aParams.mConfig.mMimeType.EqualsLiteral("video/avc")) {
> +  if (!aParams.mConfig.mMimeType.EqualsLiteral("video/avc") &&
> +      !aParams.mConfig.mMimeType.EqualsLiteral("video/mp4") &&
> +      !aParams.mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp8") &&
> +      !aParams.mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp9")) {

VPXDecoder::IsVP9()

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:224
(Diff revision 3)
>                                     const Maybe<nsCString>& aGMP)
>  {
>    const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm");
>    const bool isH264 = aMimeType.EqualsLiteral("video/avc") ||
>                        aMimeType.EqualsLiteral("video/mp4");
> +  const bool isVP8 = aMimeType.EqualsLiteral("video/webm; codecs=vp8");

VPXDecoder::IsVP8()

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp:225
(Diff revision 3)
>  {
>    const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm");
>    const bool isH264 = aMimeType.EqualsLiteral("video/avc") ||
>                        aMimeType.EqualsLiteral("video/mp4");
> +  const bool isVP8 = aMimeType.EqualsLiteral("video/webm; codecs=vp8");
> +  const bool isVP9 = aMimeType.EqualsLiteral("video/webm; codecs=vp9");

VPXDecoder::IsVP9()

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:167
(Diff revision 3)
> -  const Maybe<nsCString> gmp(
> +    const Maybe<nsCString> gmp(
> -    GMPDecoderModule::PreferredGMP(NS_LITERAL_CSTRING("video/avc")));
> +      GMPDecoderModule::PreferredGMP(NS_LITERAL_CSTRING("video/avc")));
> -  if (gmp.isSome()) {
> +    if (gmp.isSome()) {
> -    aTags.AppendElement(gmp.value());
> +      aTags.AppendElement(gmp.value());
> -  }
> +    }
> +  } else if (mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp8")) {

Use VPXDecoder::IsVP8()

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:169
(Diff revision 3)
> -  if (gmp.isSome()) {
> +    if (gmp.isSome()) {
> -    aTags.AppendElement(gmp.value());
> +      aTags.AppendElement(gmp.value());
> -  }
> +    }
> +  } else if (mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp8")) {
> +    aTags.AppendElement(NS_LITERAL_CSTRING("vp8"));
> +  } else if (mConfig.mMimeType.EqualsLiteral("video/webm; codecs=vp9")) {

VPXDecoder::IsVP9()
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/3-4/
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/4-5/
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/5-6/
Above commits: fix missing const on char*, fix merge conflicts in inbound.
Comment on attachment 8768236 [details]
Bug 1279077 -  Update GMP/EME path to support webm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62560/diff/6-7/
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/786521deead2
Update GMP/EME path to support webm. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/786521deead2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Does this feature require manual QA? If so, could you please expand a bit on what we should focus on while testing it?
Flags: qe-verify?
Flags: needinfo?(bvandyk)
There's not a much media in the wild using WebM EME by default, most tests will default to mp4. With that in mind setting the

> media.mediasource.mp4.enabled

pref to false before any testing, and setting

> media.mediasource.webm.enabled

to true.

Then the following videos sites and videos can be used:
- http://shaka-player-demo.appspot.com/demo/ Sintel Widevine and Angel One Widevine.
- Youtube: Paid content. Note that paid content should not be tested in Google Play site, it must be via youtube (bug - https://bugzilla.mozilla.org/show_bug.cgi?id=1292356).

For youtube you can confirm that WebM is being used via the 'stats for nerds' option in the right click settings. The 'Mime Type' data should show 'video/webm', with the vp8 or vp9 codec.

It would be good to make sure that the above is working on different hardware platforms.

Cheers.
Flags: needinfo?(bvandyk)
Flags: needinfo?(andrei.vaida)
(In reply to Bryce Van Dyk (:SingingTree) from comment #21)
> - Youtube: Paid content. Note that paid content should not be tested in
> Google Play site, it must be via youtube (bug -
> https://bugzilla.mozilla.org/show_bug.cgi?id=1292356).

btw, I asked our YouTube contacts for EME content we can test with that is free. The following YouTube paid content is free ($0.00), though you will need to enter credit card information:

https://youtu.be/W6E7CGm12tA
Bryce, Chris -- thank you for following up on this. Ciprian is going to be the QA contact for this feature. He's already put together a draft Test Plan based on your test instructions and data:

    https://wiki.mozilla.org/QA/Webm_eme_support_widevine
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
QA Contact: ciprian.georgiu
This is verified fixed on 50.0-build1 (20161101104304) across platforms:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12.1
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Youtube purchased videos are playing in MP4 formats rather than VP9. Is something changed after this feature?

(In reply to achalshah20 from comment #25)

Youtube purchased videos are playing in MP4 formats rather than VP9. Is something changed after this feature?

When I was testing this I recall youtube preferring to serve mp4 for encrypted content, and will fall back to webm if mp4 is not supported by the user agent. Do you explicitly want vp9/webm for some reason?

Oh i think because we needed https://bugzilla.mozilla.org/show_bug.cgi?id=1306477 at that time. Now that is already in place, we can enable support for encrypted VP9 on Youtube.

You need to log in before you can comment on or make changes to this bug.