Closed Bug 1257716 (webm-eme) Opened 8 years ago Closed 8 years ago

[EME] Add WebM EME to ClearKey

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox50 --- verified

People

(Reporter: cpearce, Assigned: bryce)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(2 files, 3 obsolete files)

Meta bug to track adding WebM support to ClearKey EME.
Alias: webm-eme
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.
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.
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/
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+
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: nobody → bvandyk
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.
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)
Attachment #8756662 - Attachment is obsolete: true
Attachment #8756662 - Flags: review?(jyavenard)
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)
Attachment #8759480 - Attachment is obsolete: true
Attachment #8759480 - Flags: review?(cpearce)
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.
Attachment #8759487 - Flags: review?(cpearce) → review-
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.
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)
Attachment #8759487 - Attachment is obsolete: true
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+
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+
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/
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/
Moving the VP8/VP9 decrypt and decode functions out of this also, as they're specific to the widevine case.
No longer blocks: 1265040, 1265043
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/
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/
Attachment #8761048 - Flags: review+ → review?(cpearce)
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-
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)
Updated both the key system access and eme playback test cases with a VP9 Scenario. Added VP9 clearkey test media.
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+
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/
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/
Auto-check-in looks to have failed. So rebasing this onto central and requesting check in.
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
https://hg.mozilla.org/mozilla-central/rev/45e8bedbdc0b
https://hg.mozilla.org/mozilla-central/rev/069ae1858a84
Status: NEW → RESOLVED
Closed: 8 years ago
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)
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)
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.
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
Flags: needinfo?(ciprian.georgiu)
You need to log in before you can comment on or make changes to this bug.