Closed
Bug 1279077
Opened 8 years ago
Closed 8 years ago
Add WebM EME support for Widevine
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
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.
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Reporter | ||
Comment 1•8 years ago
|
||
Bryce said he is working on this bug.
Assignee: nobody → bvandyk
Keywords: feature
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
Pushing WIP to review for feedback.
Mass change P2 -> P3
Priority: P2 → P3
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
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/
Assignee | ||
Updated•8 years ago
|
Attachment #8768236 -
Flags: review+ → review?(cpearce)
Assignee | ||
Comment 10•8 years ago
|
||
Requesting review for changes to MediaKeySystemAccess.cpp -- Removing commented code to enable Webm Widevine.
Updated•8 years ago
|
Attachment #8768236 -
Flags: review?(cpearce) → review+
Comment 11•8 years ago
|
||
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()
Assignee | ||
Comment 12•8 years ago
|
||
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/
Assignee | ||
Comment 13•8 years ago
|
||
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/
Assignee | ||
Comment 14•8 years ago
|
||
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
Above commits: fix missing const on char*, fix merge conflicts in inbound.
Assignee | ||
Comment 17•8 years ago
|
||
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/
Comment 18•8 years ago
|
||
Pushed by bvandyk@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/786521deead2
Update GMP/EME path to support webm. r=cpearce
Comment 19•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 20•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
testdata |
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)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(andrei.vaida)
Reporter | ||
Comment 22•8 years ago
|
||
testdata |
(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
Comment 23•8 years ago
|
||
testplan |
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
Comment 24•8 years ago
|
||
This is verified fixed on 50.0-build1 (20161101104304) across platforms:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- macOS 10.12.1
Comment 25•5 years ago
|
||
Youtube purchased videos are playing in MP4 formats rather than VP9. Is something changed after this feature?
Assignee | ||
Comment 26•5 years ago
|
||
(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?
Comment 27•5 years ago
|
||
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.
Description
•