Bug 1257716 (webm-eme)

[EME] Add WebM EME to ClearKey

VERIFIED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
P2
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: cpearce, Assigned: SingingTree)

Tracking

(Blocks: 1 bug, {meta})

unspecified
mozilla50
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox50 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Meta bug to track adding WebM support to ClearKey EME.
(Reporter)

Updated

2 years ago
Alias: webm-eme
(Reporter)

Updated

2 years ago
Depends on: 1257726
(Reporter)

Updated

2 years ago
Depends on: 1257727
(Reporter)

Updated

2 years ago
Depends on: 1257729
Blocks: 1265040
Blocks: 1265043
(Reporter)

Updated

2 years ago
Depends on: 1271242
(Assignee)

Comment 1

2 years ago
Created attachment 8756662 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

Make sure an encrypted event is created for WebMs. Handle encrypted WebM
streams for the clearkey case.

Review commit: https://reviewboard.mozilla.org/r/55328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55328/
Attachment #8756662 - Flags: review?(cpearce)
https://reviewboard.mozilla.org/r/55328/#review52070

fly by review because I couldn't help it :)

::: dom/media/VideoUtils.cpp:504
(Diff revision 1)
>  }
>  
> +bool
> +IsVorbisContentType(const nsAString& aContentType)
> +{
> +  return CheckContentType(aContentType,

why not use a nsContentTypeParser instead, then it can be called from any threads.

You're also doing it in WebMDecoder::CanHandleMediaType

kind of duplicated code.

::: dom/media/mediasource/TrackBuffersManager.cpp:1053
(Diff revision 1)
>    if (crypto && crypto->IsEncrypted()) {
>  #ifdef MOZ_EME
>      // Try and dispatch 'encrypted'. Won't go if ready state still HAVE_NOTHING.
>      for (uint32_t i = 0; i < crypto->mInitDatas.Length(); i++) {
>        NS_DispatchToMainThread(
> -        new DispatchKeyNeededEvent(mParentDecoder, crypto->mInitDatas[i].mInitData, NS_LITERAL_STRING("cenc")));
> +        new DispatchKeyNeededEvent(mParentDecoder, crypto->mInitDatas[i].mInitData, crypto->mInitDatas[i].mType));

80 columns formatting

::: dom/media/webm/WebMDemuxer.cpp:614
(Diff revision 1)
>    }
>  
>    int64_t discardPadding = 0;
>    (void) nestegg_packet_discard_padding(holder->Packet(), &discardPadding);
>  
> +  int packet_encryption = nestegg_packet_encryption(holder->Packet());

moz style uses camelCase as per wiki, so packetEncryption is preferred.

::: dom/media/webm/WebMDemuxer.cpp:630
(Diff revision 1)
>      if (aType == TrackInfo::kAudioTrack) {
>        isKeyframe = true;
>      } else if (aType == TrackInfo::kVideoTrack) {
> +      if (packet_encryption == NESTEGG_PACKET_HAS_SIGNAL_BYTE_ENCRYPTED) {
> +        // Packet is encrypted, can't peek, use packet info
> +        isKeyframe = nestegg_packet_has_keyframe(holder->Packet()) == NESTEGG_PACKET_HAS_KEYFRAME_TRUE;

We can't peek inside an encrypted packet?
that sucks.

though as it's only going to be used with mse, change of content format won't occur within a webm.

::: dom/media/webm/WebMDemuxer.cpp:1064
(Diff revision 1)
>  
>  void
>  WebMTrackDemuxer::UpdateSamples(nsTArray<RefPtr<MediaRawData>>& aSamples)
>  {
> +  for (size_t i = 0; i < aSamples.Length(); i++) {
> +    MediaRawData* sample = aSamples[i];

const RefPtr<MediaRawData>& would prevent the unecessary conversion.
(Assignee)

Comment 3

2 years ago
https://reviewboard.mozilla.org/r/55328/#review52070

> why not use a nsContentTypeParser instead, then it can be called from any threads.
> 
> You're also doing it in WebMDecoder::CanHandleMediaType
> 
> kind of duplicated code.

IIRC I'd tried to use nsContentTypeParser for the WebMDecoder case, but had issues with the service it calls into. Is nsContentTypeParser thread safe (or are there other issues which could possibly lead to the service being unavailable)?

In the VideoUtils case the calls do eventually drill down to nsContentTypeParser, and I'm just leaning on some of the helpers in VideoUtils.
(Assignee)

Comment 4

2 years ago
Comment on attachment 8756662 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55328/diff/1-2/
(Reporter)

Comment 5

2 years ago
Comment on attachment 8756662 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

https://reviewboard.mozilla.org/r/55328/#review52336

The stuff that's not in dom/media/webm/ looks like. I'll leave the WebM parser stuff to jya to review. It should really be in a separate patch. And you'll need tests. :)

::: dom/media/VideoUtils.cpp:519
(Diff revision 2)
> +bool
> +IsVP8ContentType(const nsAString& aContentType)
> +{
> +  return CheckContentType(aContentType,
> +    [](const nsAString& type) {
> +    return type.EqualsLiteral("video/webm");

Indentation; closing brace should line up with start of block, so:

bool
IsVP8ContentType(const nsAString& aContentType)
{
  return CheckContentType(aContentType,
    [](const nsAString& type) {
      return type.EqualsLiteral("video/webm");
    },
    [](const nsAString& codec) {
      return codec.EqualsLiteral("vp8");
    });

Not:

bool
IsVP8ContentType(const nsAString& aContentType)
{
  return CheckContentType(aContentType,
    [](const nsAString& type) {
    return type.EqualsLiteral("video/webm");
  },
    [](const nsAString& codec) {
    return codec.EqualsLiteral("vp8");
  });

::: dom/media/eme/MediaKeySystemAccess.cpp:484
(Diff revision 2)
> +                             const nsAString& aKeySystem,
> +                             const nsAString& aContentType,
> +                             DecoderDoctorDiagnostics* aDiagnostics)
> +{
> +  MOZ_ASSERT(HaveGMPFor(aGMPService,
> +             NS_ConvertUTF16toUTF8(aKeySystem),

Lines that are broken should line up with their opening parenthesis.

i.e.:

foo(bas(bar,
        faz))
        

Not:

foo(bas(bar,
    faz))
        
(Specifically the HaveGMPFor() call).
Attachment #8756662 - Flags: review?(cpearce) → review+
(Assignee)

Comment 6

2 years ago
https://reviewboard.mozilla.org/r/55328/#review52070

> IIRC I'd tried to use nsContentTypeParser for the WebMDecoder case, but had issues with the service it calls into. Is nsContentTypeParser thread safe (or are there other issues which could possibly lead to the service being unavailable)?
> 
> In the VideoUtils case the calls do eventually drill down to nsContentTypeParser, and I'm just leaning on some of the helpers in VideoUtils.

Scrach the above, had a further look and my memory must be incorrect given what I'm seeing. I did run into issue when doing this earlier, but I shall investigate again to see what the problem was. Splitting commits now and will submit the WebM part for review early next week.
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9af6cd7bcf4
(Assignee)

Updated

2 years ago
Assignee: nobody → bvandyk
(Assignee)

Comment 8

2 years ago
I've had a bit more of a look at the issue I'd though I'd previously had, which was consolidating the checks for different codec types into a single location. Right now, not only for webm related codecs, but also for mp4, we have some over lap in the functionality to decompose content type strings and ensure they're correct.

I did some messing around trying to move and consolidate functionality, specifically if I could do something with the different checks in VorbisDecoder, WebMDecoder, and the new checks I've added to VideoUtils. I believe I ran into issues with using nsContentTypeParser from VorbisDecoder, due to availability of the underlying service.

I've got a patch incoming shortly that follows the current convention set by the mp4 case. It'd be nice to consolidate some of this code, but I'm tempering this with getting WebM clearkey sorted.
(Assignee)

Comment 9

2 years ago
Comment on attachment 8756662 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55328/diff/2-3/
Attachment #8756662 - Attachment description: MozReview Request: Bug 1257716 - Handle clearkey encrypted WebMs. Requesting feedback: r?cpearce → Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.
Attachment #8756662 - Flags: review?(jyavenard)
(Assignee)

Comment 10

2 years ago
Created attachment 8759480 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review commit: https://reviewboard.mozilla.org/r/57472/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57472/
Attachment #8759480 - Flags: review?(cpearce)
(Assignee)

Updated

2 years ago
Attachment #8756662 - Attachment is obsolete: true
Attachment #8756662 - Flags: review?(jyavenard)
(Assignee)

Comment 11

2 years ago
Created attachment 8759487 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

Handle encrypted WebM streams for the clearkey case. Add checking for the
widevine case, though these should currently fail, as not all of the plumping
is in place for widevine.

Review commit: https://reviewboard.mozilla.org/r/57488/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57488/
Attachment #8759487 - Flags: review?(cpearce)
(Assignee)

Updated

2 years ago
Attachment #8759480 - Attachment is obsolete: true
Attachment #8759480 - Flags: review?(cpearce)
(Assignee)

Comment 12

2 years ago
Sorry for spam, trying to get a couple of commits out of me tree and onto review board together. Seems like it's not going to happen at the moment, will wait for their ancestor to make it to central which should make it workable.
(Reporter)

Updated

2 years ago
Attachment #8759487 - Flags: review?(cpearce) → review-
(Reporter)

Comment 13

2 years ago
Comment on attachment 8759487 [details]
Bug 1257716 - Handle clearkey encrypted WebMs. Put in place widevine checks.

https://reviewboard.mozilla.org/r/57488/#review55220

::: dom/media/VideoUtils.cpp:513
(Diff revision 1)
> +{
> +  return CheckContentType(aContentType,
> +    [](const nsAString& type) {
> +    return type.EqualsLiteral("video/webm");
> +  },
> +    [](const nsAString& codec) {

Indentation here is off, it should match IsVorbisContentType().

::: dom/media/VideoUtils.cpp:525
(Diff revision 1)
> +{
> +  return CheckContentType(aContentType,
> +    [](const nsAString& type) {
> +    return type.EqualsLiteral("video/webm");
> +  },
> +    [](const nsAString& codec) {

Indentation here is off, it should match IsVorbisContentType().

::: dom/media/eme/MediaKeySystemAccess.cpp:524
(Diff revision 1)
> -          GMPDecryptsAndGeckoDecodesH264(aGMPService, aKeySystem, aVideoType, aDiagnostics));
> +           GMPDecryptsAndGeckoDecodesH264(aGMPService, aKeySystem, aVideoType, aDiagnostics);
> +  }
> +  if (IsVP8ContentType(aVideoType)) {
> +    if (aKeySystem.EqualsLiteral("org.w3.clearkey")) {
> +      return GMPDecryptsAndGeckoDecodesVP8(aGMPService, aKeySystem, aVideoType, aDiagnostics);
> +    } else if (aKeySystem.EqualsLiteral("come.widevine.alpha")) {

Don't add Widevine here yet, let's do that in a separate bug.

::: dom/media/eme/MediaKeySystemAccess.cpp:531
(Diff revision 1)
> +    }
> +  }
> +  if (IsVP9ContentType(aVideoType)) {
> +    if (aKeySystem.EqualsLiteral("org.w3.clearkey")) {
> +      return GMPDecryptsAndGeckoDecodesVP9(aGMPService, aKeySystem, aVideoType, aDiagnostics);
> +    } else if (aKeySystem.EqualsLiteral("come.widevine.alpha")) {

Don't add Widevine here yet, let's do that in a separate bug.
(Assignee)

Comment 14

2 years ago
Created attachment 8761047 [details]
Bug 1257716 - Handle clearkey encrypted WebMs.

Handle encrypted WebM streams for the clearkey case. Add checking for the
widevine case, though these should currently fail, as not all of the plumping
is in place for widevine.

Review commit: https://reviewboard.mozilla.org/r/58400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58400/
Attachment #8761047 - Flags: review?(cpearce)
Attachment #8761048 - Flags: review?(cpearce)
(Assignee)

Comment 15

2 years ago
Created attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review commit: https://reviewboard.mozilla.org/r/58402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58402/
(Assignee)

Updated

2 years ago
Attachment #8759487 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d9211fa257d
(Reporter)

Comment 17

2 years ago
Comment on attachment 8761047 [details]
Bug 1257716 - Handle clearkey encrypted WebMs.

https://reviewboard.mozilla.org/r/58400/#review55250

::: dom/media/eme/MediaKeySystemAccess.cpp:521
(Diff revision 1)
>  {
> -  return IsH264ContentType(aVideoType) &&
> -         (GMPDecryptsAndDecodesH264(aGMPService, aKeySystem, aDiagnostics) ||
> -          GMPDecryptsAndGeckoDecodesH264(aGMPService, aKeySystem, aVideoType, aDiagnostics));
> +  if (IsH264ContentType(aVideoType)) {
> +    return GMPDecryptsAndDecodesH264(aGMPService, aKeySystem, aDiagnostics) ||
> +           GMPDecryptsAndGeckoDecodesH264(aGMPService, aKeySystem, aVideoType, aDiagnostics);
> +  }
> +  if (IsVP8ContentType(aVideoType)) {

I would do this as one "if" statement:

  if (IsVP8ContentType(aVideoType) && aKeySystem.EqualsLiteral("org.w3.clearkey")) {
    // ...
  }
  
Then you don't need an extra level of indent.
  
ditto for the VP9 case.
Attachment #8761047 - Flags: review?(cpearce) → review+
(Reporter)

Comment 18

2 years ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

https://reviewboard.mozilla.org/r/58402/#review55252
Attachment #8761048 - Flags: review?(cpearce) → review+
(Assignee)

Comment 19

2 years ago
Comment on attachment 8761047 [details]
Bug 1257716 - Handle clearkey encrypted WebMs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58400/diff/1-2/
(Assignee)

Comment 20

2 years ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58402/diff/1-2/
(Assignee)

Comment 21

2 years ago
Moving the VP8/VP9 decrypt and decode functions out of this also, as they're specific to the widevine case.
(Assignee)

Comment 22

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40e96754268d
No longer blocks: 1265040, 1265043
Blocks: 1279077
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f31427366602
(Assignee)

Comment 24

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4c74b3fc4b6
(Assignee)

Comment 25

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fa9e4448fcc
(Assignee)

Comment 26

a year ago
Comment on attachment 8761047 [details]
Bug 1257716 - Handle clearkey encrypted WebMs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58400/diff/2-3/
(Assignee)

Comment 27

a year ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58402/diff/2-3/
(Assignee)

Updated

a year ago
Attachment #8761048 - Flags: review+ → review?(cpearce)
(Reporter)

Comment 28

a year ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

https://reviewboard.mozilla.org/r/58402/#review57346

Almost, but we should have VP9 coverage too. Just in case there's a difference in the code paths somewhere.

::: dom/media/test/manifest.js:1376
(Diff revision 3)
> +      "8b5df745ad84145b5617c33116e35a67" : "bddfd35dd9be033ee73bc18bc1885056",
> +    },
> +    sessionType:"temporary",
> +    sessionCount:2,
> +    duration:1.60,
> +  },

Can you please add a VP9 stream as well? It's fine to pair the existing Vorbis stream there.
Attachment #8761048 - Flags: review?(cpearce) → review-
(Assignee)

Comment 29

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b891236d4156
(Assignee)

Comment 30

a year ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58402/diff/3-4/
Attachment #8761048 - Flags: review- → review?(cpearce)
(Assignee)

Comment 31

a year ago
Updated both the key system access and eme playback test cases with a VP9 Scenario. Added VP9 clearkey test media.
(Reporter)

Comment 32

a year ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

https://reviewboard.mozilla.org/r/58402/#review57462
Attachment #8761048 - Flags: review?(cpearce) → review+
(Assignee)

Comment 33

a year ago
Comment on attachment 8761047 [details]
Bug 1257716 - Handle clearkey encrypted WebMs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58400/diff/3-4/
(Assignee)

Comment 34

a year ago
Comment on attachment 8761048 [details]
Bug 1257716 - Add WebM Clearkey media to eme test list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58402/diff/4-5/
(Assignee)

Comment 35

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7786ad8f0baa
(Assignee)

Comment 36

a year ago
Auto-check-in looks to have failed. So rebasing this onto central and requesting check in.
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 37

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45e8bedbdc0b
Handle clearkey encrypted WebMs. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/069ae1858a84
Add WebM Clearkey media to eme test list. r=cpearce
Keywords: checkin-needed

Comment 38

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/45e8bedbdc0b
https://hg.mozilla.org/mozilla-central/rev/069ae1858a84
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
This looks like something that might require QA's attention. 

Bryce, what's your opinion on this? Could manual QA help test this feature?
Flags: qe-verify?
Flags: needinfo?(bvandyk)
(Assignee)

Comment 40

a year ago
I agree the manual QA could help test this.

This can be tested by setting the following prefs:
- media.mediasource.mp4.enabled to false
- media.mediasource.webm.enabled to true

Then loading the Shaka Player (http://shaka-player-demo.appspot.com/demo/) Angel One Clearkey video and making sure playback works correctly.
Flags: needinfo?(bvandyk) → needinfo?(andrei.vaida)
I verified the fix on Firefox 50.0b1-build2 (2016-09-20), on the following OSes:
- Windows 10 x64
- Ubuntu x64 16.04 LTS
- macOS 10.12 (Sierra)  
The Angel One ClearKey video is working as expected and is playing correctly.

   -- I noticed that with the following prefs: media.mediasource.mp4.enabled set to false and media.mediasource.webm.enabled set to true, on Ubuntu 16.04 LTS and macOS 10.12, the Angel One Clearkey video is greyed out. If the prefs are are not changed (default) the video is available again and it plays as expected.
It's this normal Bryce, what do you think? Leaving the flags in place for now.
Flags: needinfo?(andrei.vaida)
(Assignee)

Comment 42

a year ago
I'm going to see if I can take a look at what's going on here. It's not what I would have expected.
(Reporter)

Updated

a year ago
Depends on: 1306477
(Assignee)

Comment 43

a year ago
Follow up to Shaka player not working: I've had a look and I think this may be Shaka player using canPlayType to infer what can be played. In some cases its inference is incorrect. Follow up testing with some of the examples here: http://singingtree.github.io/Webm-EME-Examples/ has been successful, even in cases where Shaka has Clearkey options greyed out.

Given the above I do not believe this is an issue with the new code, but rather an interaction with how Shaka attempts to infer capabilities of client browsers. I think it's using mp4 playback functionality to gate some of the multi-codec media. So when we disable mp4 media source it leads to certain options becoming locked. It's odd that this differs by platform, but I do not believe it is a cause for concern.

Does that sound reasonable?
Flags: needinfo?(ciprian.georgiu)
(In reply to Bryce Van Dyk (:SingingTree) from comment #43)
> Given the above I do not believe this is an issue with the new code, but
> rather an interaction with how Shaka attempts to infer capabilities of
> client browsers. I think it's using mp4 playback functionality to gate some
> of the multi-codec media. So when we disable mp4 media source it leads to
> certain options becoming locked. It's odd that this differs by platform, but
> I do not believe it is a cause for concern.
> 
> Does that sound reasonable?

Thank you Bryce for following on this, it's much clear now. I will mark here accordingly.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Flags: needinfo?(ciprian.georgiu)
You need to log in before you can comment on or make changes to this bug.