Bug 223742

Summary: [EME][GStreamer] Abort decryptor operations immediately and without errors on flush
Product: WebKit Reporter: Enrique Ocaña <eocanha>
Component: WebKitGTKAssignee: Enrique Ocaña <eocanha>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, calvaris, cgarcia, eric.carlson, ews-watchlist, glenn, gns, jer.noble, menard, philipj, pnormand, sergio, vjaquez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Enrique Ocaña 2021-03-25 04:01:15 PDT
A decryptor transformInPlace() operation can cause potentially long waits in two situations:

- transformInPlace() is waiting to get the cdmProxy.
- The CDMProxy::decrypt() method is internally waiting for a specific key to become available.

If a seek operation is performed during those long waits, the main thread will be blocked until the seek finishes the conditions those long waits are waiting for will never be fulfilled (because the operations that complete them happen in the main thread, which is blocked), the internal wait timeouts will trigger and the decoder will trigger an unrecoverable error.
Comment 1 Enrique Ocaña 2021-03-26 11:24:53 PDT
Created attachment 424378 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2021-04-06 05:01:39 PDT
Comment on attachment 424378 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:62
> +    bool isFlushing = false;

Nit: bool isFlushing { false }

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:205
> +            return priv->cdmProxy || priv->isFlushing;

Nit: I would change the order, I think shortcircuiting on a boolean here can prevent calling the smart pointer methods to see if the cdmProxy instance is valid.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:207
> +        // Note that waitFor() releases the mutex lock internally while it waits, so isFlushing may have been changed.

I don't see this comment as needed but I don't have a strong opinion about removing it either.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:220
> +    // Temporarily release the lock while we don't need to access priv. The lower level API is used
> +    // in order to avoid creating several scopes with different LockHolder instances in each one.
> +    priv->mutex.unlock();

I know we are not using any priv members from there till the moment we really decrypt but I think if we unlock and lock manually, both calls should be very close to each other to avoid any possible future problems in case somebody forgets we're not locked from here on. Please, move this down to just where it is needed.

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:392
> +            if (!isCdmProxyAttached)
> +                priv->condition.notifyOne();
> +            locker.unlockEarly();
> +            if (isCdmProxyAttached)
> +                priv->cdmProxy->abortWaitingForKey();

This piece of code looks a bit weird to me, can't we just do an if/else?
Comment 3 Enrique Ocaña 2021-04-06 09:21:50 PDT
Created attachment 425287 [details]
Patch
Comment 4 Xabier Rodríguez Calvar 2021-04-07 00:44:39 PDT
Comment on attachment 425287 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/eme/WebKitCommonEncryptionDecryptorGStreamer.cpp:393
> +                locker.unlockEarly();

This line is not needed, the lock is going to be released at the end of the scope.
Comment 5 Enrique Ocaña 2021-04-07 02:23:52 PDT
Created attachment 425370 [details]
Patch
Comment 6 EWS 2021-04-07 02:53:17 PDT
Committed r275599: <https://commits.webkit.org/r275599>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425370 [details].