Closed Bug 1351124 Opened 3 years ago Closed 2 years ago

[EME] support for key rotation

Categories

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

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: modmaker, Assigned: cpearce, NeedInfo)

Details

Attachments

(3 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.110 Safari/537.36

Steps to reproduce:

When using key rotation, there are multiple 'pssh' boxes within the media segments.  The 'pssh' box appears within the 'moof' box of the media segment.  An 'encrypted' event should be fired for these boxes in addition to those found in the init segments.

Additionally, it appears that the key ID is only taken from the 'tenc' box.  This is incorrect and only specifies the default value.  According to CENC (ISO/IEC 23001-7) section 6, the key ID can be overridden in the sample groups.

Here is some example content that shows Widevine key rotation:
https://shaka-player-demo.appspot.com/demo/?asset=//storage.googleapis.com/shaka-demo-assets/_bugs/firefox-key-rotation/dash.mpd;license=//cwip-shaka-proxy.appspot.com/pst


Actual results:

Video fails to play to the end.  No 'encrypted' events fired and decryption fails even if the correct keys are available.


Expected results:

Example video should play to the end.  'encrypted' events should fire for all 'pssh' boxes in the media and the key ID for samples should be overridden from the default.
(In reply to modmaker from comment #0)
> Additionally, it appears that the key ID is only taken from the 'tenc' box. 
> This is incorrect and only specifies the default value.  According to CENC
> (ISO/IEC 23001-7) section 6, the key ID can be overridden in the sample
> groups.

Firefox 54, shipped last week, and that contains a fix for key ids being defined in the sgpd box (bug 1318792).
Assignee: nobody → cpearce
I spoke to jya about this at MozSF. The "encrypted" event is fired in TrackBuffersManager [1]. The TrackBuffersManager demuxes every append in its entirety into a big array of demuxed samples. During this operation, we need to have the MoofParser report the new PSSH. One way we could do this is by having the MoofParser add the reference to the new PSSH to the MediaInfo that each sample is associated with. We then need to detect the change in PSSH and emit the "encrypted" event.


[1] https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/dom/media/mediasource/TrackBuffersManager.cpp#1142
Comment on attachment 8886009 [details]
Bug 1351124 - Factor out ClearKey license generation in EME mochitests.

https://reviewboard.mozilla.org/r/156786/#review161968
Attachment #8886009 - Flags: review?(jyavenard) → review+
Comment on attachment 8886010 [details]
Bug 1351124 - Test for PSSH boxes in MOOF boxes.

https://reviewboard.mozilla.org/r/156788/#review161972

Can't we reuse existing tests for that? or adapt a test to check we receive the expected amount of encrypted events?

::: dom/media/test/test_eme_pssh_in_moof.html:31
(Diff revision 2)
> +        log_pane.appendChild(document.createElement("br"));
> +      }
> +
> +      function DownloadMedia(url, type, mediaSource) {
> +        return new Promise((resolve, reject) => {
> +          var sourceBuffer = mediaSource.addSourceBuffer(type);

I don't know the format of the stream being played.

But if you have more than one buffer, you end up with multiple sourcebuffer.

This is dangerous. It requires that all buffers being added is made of init+media.

this should test that no sourceBuffer of an existing type already exist.

I see that the same code exists in other tests, so it's probably fine...

::: dom/media/test/test_eme_pssh_in_moof.html:45
(Diff revision 2)
> +
> +      function LoadMSE(video, tracks) {
> +        var ms = new MediaSource();
> +        video.src = URL.createObjectURL(ms);
> +        once(ms, "sourceopen", () => {
> +          Promise.all(tracks.map(track => DownloadMedia(track.url, track.type, ms)))

I think this could lead to intermittent failure if you have both video and audio tracks.

The order in which segments are appended is important.
No media segment must be appended before all init segments for all tracks have been added.

so the order should be something like:
appendBuffer(initsegment_video);
appendBuffer(initsegment_audio);

after that the order into which you append data between audio and video is irrelevant.
Attachment #8886010 - Flags: review?(jyavenard) → review+
Comment on attachment 8886011 [details]
Bug 1351124 - Detect MP4 PSSH boxes in MOOF boxes and dispatch those in 'encrypted' events to content.

https://reviewboard.mozilla.org/r/156790/#review161974

I think we have an issue with 64 bits length box... so in doubt....

::: dom/media/mediasource/TrackBuffersManager.cpp:1283
(Diff revision 2)
>    MSE_DEBUG("%" PRIuSIZE " video samples demuxed", aSamples->mSamples.Length());
>    mVideoTracks.mDemuxRequest.Complete();
>    mVideoTracks.mQueuedSamples.AppendElements(aSamples->mSamples);
> +
> +  // Try and dispatch 'encrypted'. Won't go if ready state still HAVE_NOTHING.
> +  for (const RefPtr<MediaRawData>& sample : aSamples->mSamples) {

this code is duplicated...

Please move this to a separate function.

::: dom/media/mediasource/TrackBuffersManager.cpp:1284
(Diff revision 2)
>    mVideoTracks.mDemuxRequest.Complete();
>    mVideoTracks.mQueuedSamples.AppendElements(aSamples->mSamples);
> +
> +  // Try and dispatch 'encrypted'. Won't go if ready state still HAVE_NOTHING.
> +  for (const RefPtr<MediaRawData>& sample : aSamples->mSamples) {
> +    for (const nsTArray<uint8_t>& initData : sample->mCrypto.mInitDatas) {

this doesn't do what the commit title states "to see whether they have any new EME init data"

It doesn't check if we have new EME init data, it only checks if there are *any* init datas.

The commit message should be modified, or this code modified

::: media/libstagefright/binding/Box.cpp:122
(Diff revision 2)
>        !byteRange->Contains(boxRange)) {
>      return;
>    }
>  
>    mRange = boxRange;
> +  mHeader.AppendElements(header, sizeof(header));

this assume that the header contains all the information required.

If it's an extended header (type 1), the size is encoded as 64 bits int and is found after the 8 bytes header.

Shouldn't this be saved?

Otherwise you'll get a copy of 00001 | FOURCC(pss)

and the actual size is lost.

::: media/libstagefright/binding/Index.cpp:127
(Diff revision 2)
>    if (!mIndex->mSource->ReadAt(sample->mOffset, writer->Data(), sample->Size(),
>                                 &bytesRead) || bytesRead != sample->Size()) {
>      return nullptr;
>    }
>  
> +  if (mCurrentSample == 0 && mIndex->mMoofParser) {

so if you seek in the media, are you supposed to get the encrypted event again?

::: media/libstagefright/binding/Index.cpp:132
(Diff revision 2)
> +  if (mCurrentSample == 0 && mIndex->mMoofParser) {
> +    const nsTArray<Moof>& moofs = mIndex->mMoofParser->Moofs();
> +    MOZ_ASSERT(mCurrentMoof < moofs.Length());
> +    const Moof* currentMoof = &moofs[mCurrentMoof];
> +    if (!currentMoof->mPsshes.IsEmpty()) {
> +      // This Moof contained new crypto init data. Report that.

I don't see how this test that we have new crypto init data, just that we have crypto init data in the moof.
Comment on attachment 8886010 [details]
Bug 1351124 - Test for PSSH boxes in MOOF boxes.

https://reviewboard.mozilla.org/r/156788/#review161972

I'm tryng to test on a finer grain here, rather than adding huge monolithic tests which test everything at once.

Also, we need to fix how we dispatch contiguous PSSH for PSSH boxes in the MOOV, as we don't concatenate them correctly yet. (Bug 1378996)

> I don't know the format of the stream being played.
> 
> But if you have more than one buffer, you end up with multiple sourcebuffer.
> 
> This is dangerous. It requires that all buffers being added is made of init+media.
> 
> this should test that no sourceBuffer of an existing type already exist.
> 
> I see that the same code exists in other tests, so it's probably fine...

In this test, there are two streams, and so two sourceBuffers, and the each stream is appended in its entirety in a single append. i.e. the append for both streams contains both init+media data.

So given that you said "It requires that all buffers being added is made of init+media.", in this specific test it should be fine.

> I think this could lead to intermittent failure if you have both video and audio tracks.
> 
> The order in which segments are appended is important.
> No media segment must be appended before all init segments for all tracks have been added.
> 
> so the order should be something like:
> appendBuffer(initsegment_video);
> appendBuffer(initsegment_audio);
> 
> after that the order into which you append data between audio and video is irrelevant.

Both streams are appended in their entirety in a single append, so we should be good on this specific test.
Comment on attachment 8886011 [details]
Bug 1351124 - Detect MP4 PSSH boxes in MOOF boxes and dispatch those in 'encrypted' events to content.

https://reviewboard.mozilla.org/r/156790/#review161974

Good point...

> so if you seek in the media, are you supposed to get the encrypted event again?

We only demux when the media data is appended, and store the resulting demuxed samples. So if we seek in the stream, we won't re-demux the MOOFs right? So we shouldn't run the code re-demux and check for PSSH boxes again on the same MOOFs when we seek? We should only redemux if we re-append, and if so I think dispatching again is allowed under the EME spec, as we're supposed to dispatch whenever we encounter crypto init data.

For plain mp4, we'll re-dispatch on seek, but that's unsupported... And I think it would still be ok even if we did suport it.
Comment on attachment 8886011 [details]
Bug 1351124 - Detect MP4 PSSH boxes in MOOF boxes and dispatch those in 'encrypted' events to content.

https://reviewboard.mozilla.org/r/156790/#review162266

::: media/libstagefright/binding/MoofParser.cpp:411
(Diff revision 3)
> +
> +  // The EME spec requires that PSSH boxes which are contiguous in the
> +  // file are dispatched to the media element in a single "encrypted" event.
> +  // So append contiguous boxes here.
> +  for (size_t i = 0; i < psshBoxes.Length(); ++i) {
> +    Box box = psshBoxes[i];

use a const reference here to avoid a copy
Attachment #8886011 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/f1c12c6694a8
https://hg.mozilla.org/mozilla-central/rev/bcaeba51bdb7
https://hg.mozilla.org/mozilla-central/rev/6d07cd4c3102
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I'm doing a test on FireFox 64(userAgent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0)

I still find it can't play content with 'pssh' box in 'moof' box. 

The same content plays fine on chrome.

A demo page can be find at https://github.com/luna2216/ClearKeyDemo
Flags: needinfo?(cpearce)
(In reply to luna_2216 from comment #20)
> I'm doing a test on FireFox 64(userAgent: Mozilla/5.0 (Windows NT 6.1;
> Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0)
> 
> I still find it can't play content with 'pssh' box in 'moof' box. 
> 
> The same content plays fine on chrome.
> 
> A demo page can be find at https://github.com/luna2216/ClearKeyDemo

This demo (live at https://cpearce.github.io/ClearKeyDemo/clearkey.html ) only plays the first segment for me in Chrome, not the entire resource.

I looked at this, and the initial EME session is being setup and the key session is being negotiated, and we're dispatching two "encrypted" events, so we're reading at least 2 PSSH boxes (there's 1 in the init segment). But the demuxer never successfully demuxes any samples.

Looking at the Web Console logging, Firefox isn't able to demux the segments; in Chrome I see logging such as:

[...]
clearkey.js:159 video buffer appended:[ ]
clearkey.js:34 Loaded: ./cmaf/seg-1.m4s
clearkey.js:159 video buffer appended:[ 1544690360.408-1544690360.608 ]
clearkey.js:34 Loaded: ./cmaf/seg-2.m4s
clearkey.js:159 video buffer appended:[ 1544690360.408-1544690360.808 ]
[...]

Whereas in Firefox this looks like

[...]
video buffer appended:[ ]
Loaded: ./cmaf/seg-1.m4s
video buffer appended:[ ]
Loaded: ./cmaf/seg-2.m4s
video buffer appended:[ ]
Loaded: ./cmaf/seg-3.m4s
video buffer appended:[ ]
Loaded: ./cmaf/seg-4.m4s
[...]

So it looks like the appendBuffer isn't resulting in readable data.

jya: are you able to look into why Firefox is unable to demux these segments?
Flags: needinfo?(cpearce) → needinfo?(jyavenard)

I've broken some of our handling here: please see bug 1552717. I'm currently investigating, and will put updates in that bug.

That said, that bustage takes place after the reports in comment #20 by a month, so we have another issue here. Indeed, during my testing I was seeing problems with us appending data and it not being playable and me needed to skip forward in order to get media to play correct.

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