1310548 - [EME] WPT clearkey-mp4-playback-temporary-multikey-sequential.html fails

Status: RESOLVED FIXED

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

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

clearkey-mp4-playback-temporary-multikey-sequential.html fails because we're not setting the readyState to HAVE_METADATA while we're waiting for a key.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3081a28cfeb3

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #1)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3081a28cfeb3

The tests failed in this push because the following happens:
1. We load the stream, hit the TENC box on streams, and fire the "encrypted" event.
2. The MDSM goes into WAIT_FOR_CDM state.
3. Script processes the "encrypted" event, and creates a MediaKeys, attaches it to the HTMLMediaElement.
4. The MDSM wakes up and moves from WAIT_FOR_CDM to DECODING_FIRSTFRAME.
5. Decoder finds it can't decode, starts waiting for key.
6. Script negotiates a license with CDM, keys are marked as usable, and decode starts.
7. MDSM successfully decodes first frame, and calls HTMLMediaElement::FirstFrameLoaded(), which updates the readyState, but thinks we're still waiting for key, so we remain in HAVE_METADATA, and don't fire "loadeddata".

One easy solution here would be to have HTMLMediaElement::FirstFrameLoaded() reset the "waiting for key" state, but the problem there is we could potentially have just hit a frame which we couldn't decode for, and end up overwriting the "waiting for key" state.

So we're better off propagating "waiting for key" a different way.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b78bc89c8bff

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1310548 - Delay firing of 'waitingforkey' until all decoded data has been rendered.

This means 'waitingforkey' will be fired at a predictable time.

Review commit: https://reviewboard.mozilla.org/r/87398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87398/

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note: this patch makes /encrypted-media/drm-mp4-playback-temporary-multikey-sequential.html pass but /encrypted-media/{clearkey,drm}-mp4-playback-temporary-multikey-sequential-readyState.html fail still until https://github.com/w3c/encrypted-media/issues/338 is resolved in the spec and the WPT are updated.

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8803142 [details]
Bug 1310548 - Delay firing of 'waitingforkey' until all decoded data has been rendered.

https://reviewboard.mozilla.org/r/87398/#review86410

::: dom/html/HTMLMediaElement.h:1595
(Diff revision 1)
>    bool mIsEncrypted;
>  
> -  // True when the CDM cannot decrypt the current block, and the
> -  // waitingforkey event has been fired. Back to false when keys have become
> -  // available and we can advance the current playback position.
> +  // True when the CDM cannot decrypt the current block due to lacking a key.
> +  // Note: the "waitingforkey" event is not dispatched until all decoded data
> +  // has been rendered.
>    bool mWaitingForKey;

couldn't we have used an enum for this ? like a 3 states value, if NOT_WAITING=0 then WAITING WAITING_FIRED something like that.

all those bool are always a pain to maintain.

Additionally, if mWaitingForKey is false, then mDispatchedWaitingForKey is always false seeing that both are always reset to false simultaneously.

::: dom/html/HTMLMediaElement.cpp:4761
(Diff revision 1)
>      return;
>    }
>  
>    if (mDownloadSuspendedByCache &&
>        mDecoder && !mDecoder->IsEnded() &&
> +      !mWaitingForKey &&

note: i don't think this can happen as EME is only used with MSE, and mDownloadSuspendedByCache is always false

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Comment on attachment 8803142 [details]
> Bug 1310548 - Delay firing of 'waitingforkey' until all decoded data has
> been rendered.
> 
> https://reviewboard.mozilla.org/r/87398/#review86410
> 
> ::: dom/html/HTMLMediaElement.h:1595
> (Diff revision 1)
> >    bool mIsEncrypted;
> >  
> > -  // True when the CDM cannot decrypt the current block, and the
> > -  // waitingforkey event has been fired. Back to false when keys have become
> > -  // available and we can advance the current playback position.
> > +  // True when the CDM cannot decrypt the current block due to lacking a key.
> > +  // Note: the "waitingforkey" event is not dispatched until all decoded data
> > +  // has been rendered.
> >    bool mWaitingForKey;
> 
> couldn't we have used an enum for this ? 

Sure, sounds good.

> ::: dom/html/HTMLMediaElement.cpp:4761
> (Diff revision 1)
> >      return;
> >    }
> >  
> >    if (mDownloadSuspendedByCache &&
> >        mDecoder && !mDecoder->IsEnded() &&
> > +      !mWaitingForKey &&
> 
> note: i don't think this can happen as EME is only used with MSE, and
> mDownloadSuspendedByCache is always false

Good point. Thanks!

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8803142 [details]
Bug 1310548 - Delay firing of 'waitingforkey' until all decoded data has been rendered.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87398/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ec35e0e63b5
Delay firing of 'waitingforkey' until all decoded data has been rendered. r=jya

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ec35e0e63b5