Bug 189951 - MediaPlayer should have mediaPlayerWaitingForKeyChanged() / bool waitingForKey() accessor
Summary: MediaPlayer should have mediaPlayerWaitingForKeyChanged() / bool waitingForKe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-25 09:57 PDT by Jer Noble
Modified: 2018-09-27 14:42 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.08 KB, patch)
2018-09-25 11:19 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (22.51 KB, patch)
2018-09-26 10:48 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (22.61 KB, patch)
2018-09-26 11:11 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2018-09-25 09:57:39 PDT
Currently, HTMLMediaElement assumes that the MediaPlayer is no longer waiting for a key after attemptToDecrypt() is called. It elides the "If the user agent can advance the current playback position in the direction of playback:" step.

MediaPlayer should provide a "bool waitingForKey() const" accessor so that HTMLMediaElement can query whether it's still waiting for a key after attemptToDecrypt(), as well as a mediaPlayerWaitingForKeyChanged() client notification when this value changes. This would replace the existing mediaPlayerWaitingForKey() notification.
Comment 1 Jer Noble 2018-09-25 09:59:26 PDT Comment hidden (obsolete)
Comment 2 Jer Noble 2018-09-25 09:59:34 PDT
I'll post a patch making this change, but I'll wait till I hear back from Igalia people before landing, as this will definitely have the potential to break ports.
Comment 3 Philippe Normand 2018-09-25 10:09:50 PDT
Thanks Jer.
Comment 4 Jer Noble 2018-09-25 11:19:58 PDT
Created attachment 350766 [details]
Patch
Comment 5 Xabier Rodríguez Calvar 2018-09-25 11:30:43 PDT
I'd appreciate if you can wait until Monday since I am going on holidays.
Comment 6 Eric Carlson 2018-09-25 11:33:53 PDT
Comment on attachment 350766 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:2807
> +        setReadyState(m_player->readyState());

Nit: this should be a noop because the player always updates readyState when it changes.
Comment 7 Xabier Rodríguez Calvar 2018-09-25 15:54:30 PDT
Comment on attachment 350766 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:2807
>> +        setReadyState(m_player->readyState());
> 
> Nit: this should be a noop because the player always updates readyState when it changes.

I am not sure of this. IIRC, when I wrote the first patch I updated the readyState in the element without syncing it to the player, so this would force it to be reset to what the player had.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:311
> +            gst_element_post_message(GST_ELEMENT(self), gst_message_new_element(GST_OBJECT(self), gst_structure_new_empty("drm-key-received")));

Wow, I am still amazed by how you solved this and your ability to send GStreamer events in one side and catch them at the other :) . First I thought this couldn't work but then I realized that it actually can, at least on paper and no (more) tests are broken. First I thought that now that we track if we can play or not, you should remove the code below this that resends the waitingforkey event. Then I realized that we are not tracking that properly because you only consider one decryptor being blocked and you can have more than one. Then I realized that the code below would fix that as it would resend the waitingforkey event.

I also thought this wouldn't work because the attempt to decrypt is not connected but our decryptors are quite autonomous and if they are waiting, they will carry on once they get the key, which will put things in motion again.

This said, I made an "interesting" interpretation of the ability to continue playback, which is something I can't know unless I query the all decryptors so I assume yes and let the algorithm stop again once I "can't continue decrypting things on the encrypted blocks awaiting decryption list". I guess your approach is better but I'll keep that as a fix me for the future.

r=me on the GStreamer part as no tests are broken and solution seems sensible. I don't know if Phil has some more comments on this.

PS: You can still come to the hackfest, btw! :)
Comment 8 Philippe Normand 2018-09-26 00:35:26 PDT
No concerns from my side. A last iteration seems needed though to turn EWS green.
Comment 9 Jer Noble 2018-09-26 10:48:34 PDT
Created attachment 350871 [details]
Patch for landing
Comment 10 Jer Noble 2018-09-26 11:11:43 PDT
Created attachment 350874 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2018-09-27 14:41:33 PDT
Comment on attachment 350874 [details]
Patch for landing

Clearing flags on attachment: 350874

Committed r236572: <https://trac.webkit.org/changeset/236572>
Comment 12 WebKit Commit Bot 2018-09-27 14:41:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-09-27 14:42:29 PDT
<rdar://problem/44844394>