Summary: | MediaPlayer should have mediaPlayerWaitingForKeyChanged() / bool waitingForKey() accessor | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jer Noble <jer.noble> | ||||||||
Component: | Media | Assignee: | Jer Noble <jer.noble> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | calvaris, commit-queue, eric.carlson, pnormand, webkit-bug-importer, zan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Safari Technology Preview | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Jer Noble
2018-09-25 09:57:39 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. 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. Thanks Jer. Created attachment 350766 [details]
Patch
I'd appreciate if you can wait until Monday since I am going on holidays. 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 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! :) No concerns from my side. A last iteration seems needed though to turn EWS green. Created attachment 350871 [details]
Patch for landing
Created attachment 350874 [details]
Patch for landing
Comment on attachment 350874 [details] Patch for landing Clearing flags on attachment: 350874 Committed r236572: <https://trac.webkit.org/changeset/236572> All reviewed patches have been landed. Closing bug. |