Bug 189616

Summary: [EME] Add support the waitingforkey event
Product: WebKit Reporter: Xabier Rodríguez Calvar <calvaris>
Component: New BugsAssignee: Xabier Rodríguez Calvar <calvaris>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jer.noble, pnormand, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 185590, 189720    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Xabier Rodríguez Calvar 2018-09-14 01:34:30 PDT
[EME] Add support the waitingforkey event
Comment 1 Xabier Rodríguez Calvar 2018-09-14 01:36:48 PDT
Created attachment 349751 [details]
Patch
Comment 2 Philippe Normand 2018-09-14 02:13:37 PDT
Comment on attachment 349751 [details]
Patch

Surely there are some layout tests than can be unskipped/unflagged after this?
Comment 3 Xabier Rodríguez Calvar 2018-09-14 03:12:50 PDT
(In reply to Philippe Normand from comment #2)
> Comment on attachment 349751 [details]
> Patch
> 
> Surely there are some layout tests than can be unskipped/unflagged after
> this?

When I land the patch for the bug depending on this one, yes.
Comment 4 Philippe Normand 2018-09-14 03:30:42 PDT
I'm talking about platform-agnostic tests.
Comment 5 Xabier Rodríguez Calvar 2018-09-14 03:37:02 PDT
(In reply to Philippe Normand from comment #4)
> I'm talking about platform-agnostic tests.

Me too, but this patch only implements the crossplatform part that requires the platform one to work.
Comment 6 Philippe Normand 2018-09-14 05:31:04 PDT
Comment on attachment 349751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349751&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:2845
> +    // The platform already suspends and resumes playback
> +    // automagically.

Can you elaborate?
Comment 7 Xabier Rodríguez Calvar 2018-09-14 06:37:35 PDT
(In reply to Philippe Normand from comment #6)
> Comment on attachment 349751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349751&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2845
> > +    // The platform already suspends and resumes playback
> > +    // automagically.
> 
> Can you elaborate?

In GStreamer the decryptors block until they can continue decrypting and once they can, they go on without needing intervention, playback is "naturally suspended". I don't know about other platforms but I guess we can talk about it when the time comes.
Comment 8 Xabier Rodríguez Calvar 2018-09-14 07:59:33 PDT
Created attachment 349760 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-09-14 08:37:21 PDT
Comment on attachment 349760 [details]
Patch for landing

Clearing flags on attachment: 349760

Committed r236006: <https://trac.webkit.org/changeset/236006>
Comment 10 WebKit Commit Bot 2018-09-14 08:37:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-09-14 08:38:57 PDT
<rdar://problem/44457738>
Comment 12 Jer Noble 2018-09-18 15:29:15 PDT
This patch broke a number of our internal EME tests, and the Modern EME feature. Unless the client calls waitingForKey(), it will never receive an attemptToDecrypt().

There is an existing client callback, initializationDataEncountered(), that could have been used for the same purpose as waitingForKey(), or this patch could have added waitingForKey() everywhere initializationDataEncountered() was already called.
Comment 13 Xabier Rodríguez Calvar 2018-09-18 23:41:31 PDT
(In reply to Jer Noble from comment #12)
> This patch broke a number of our internal EME tests, and the Modern EME
> feature. Unless the client calls waitingForKey(), it will never receive an
> attemptToDecrypt().
> 
> There is an existing client callback, initializationDataEncountered(), that
> could have been used for the same purpose as waitingForKey(), or this patch
> could have added waitingForKey() everywhere initializationDataEncountered()
> was already called.

Hi Jer,

Oh! Sorry to read that this patch broke your internal tests. About the spec and the wait for key algorithms, it is clear for me that they are different things and can be triggered in different situations:
* initializationDataEncountered() happens when you're playing (or about to) something and you get the initialization data from the stream that you have to report to JS but unless you cannot play anymore because you are already getting encrypted data, they are separate things.
* waitingForKey(), according to the spec means that you cannot play anymore because you are blocked and it doesn't necessarily correspond with the moment you get the initialization data

For me, the case of initialization data encountered is clear, when you see the data, you report it. The case of the wait for key algorithm has the following cases:

[1] https://www.w3.org/TR/encrypted-media/#htmlmediaelement-extensions

When one of the following occurs while the decryption blocked waiting for key value is true, the user agent shall run the Wait for Key algorithm.
* The user agent cannot advance the current playback position in the direction of playback.
* The user agent cannot provide data for the current playback position.

[2] https://www.w3.org/TR/encrypted-media/#media-may-contain-encrypted-blocks

The Media Data May Contain Encrypted Blocks algorithm pauses playback if the user agent requires specification of a MediaKeys object before playing the media data. Requests to run this algorithm include a target HTMLMediaElement object.

...

2. If the media element's mediaKeys attribute is null and the implementation requires specification of a MediaKeys object before decoding potentially-encrypted media data, run the following steps:

[3] https://www.w3.org/TR/encrypted-media/#attempt-to-decrypt

NOTE
Once the user agent has rendered the blocks preceding the block that cannot be decrypted (as much as it can, such as, all complete video frames), it will run the Wait for Key algorithm.

[4] https://www.w3.org/TR/encrypted-media/#wait-for-key

The algorithm itself.

[5] https://www.w3.org/TR/encrypted-media/#resume-playback

Attempt to Resume Playback If Necessary



From [2] I get that you could even run the wait for key algorithm even before finding initialization data if you require it, which we (GStreamer) don't.

From [1] and [3] I get that you might get initialization data but this algorithm should be run when you can't render anything else because you're blocked waiting for decryption. For example, in our tests with YouTube and other streams we have seen that playback begins unencrypted, you get the initialization data and fire it to JS which can trigger all the process until being able to decrypt without being blocked because you are ready to decrypt in advance. Actually, the wait to decrypt is a symptom that you're too late because you have to suspend playback, right?

It's true that [5] bails out in step 2 and I thought that it could create issue so I paid attention to Mac the bots when landing but I saw no breakage so I was quiet in the end.
Comment 14 Jer Noble 2018-09-19 09:42:42 PDT
(In reply to Xabier Rodríguez Calvar from comment #13)
> (In reply to Jer Noble from comment #12)
> > This patch broke a number of our internal EME tests, and the Modern EME
> > feature. Unless the client calls waitingForKey(), it will never receive an
> > attemptToDecrypt().
> > 
> > There is an existing client callback, initializationDataEncountered(), that
> > could have been used for the same purpose as waitingForKey(), or this patch
> > could have added waitingForKey() everywhere initializationDataEncountered()
> > was already called.
> 
> Hi Jer,
> 
> Oh! Sorry to read that this patch broke your internal tests. 

Yeah, it's really unfortunate that we can't run any /actual/ EME tests as part of the n normal LayoutTest process; there's an external server requirement.  I've got a background task of trying to figure out a way to get EME tests running in an automated and public way.

> About the spec
> and the wait for key algorithms, it is clear for me that they are different
> things and can be triggered in different situations... [snip]

Thanks for clarifying. We'll try to update our own platform implementation match the spec as closely as possible. That said, I was really referring to restoring the status-quo-ante for ports which have not yet implemented waitingForKey().  Bug #189720 should fix that for the macOS and iOS ports.