Bug 223742 - [EME][GStreamer] Abort decryptor operations immediately and without errors on flush
Summary: [EME][GStreamer] Abort decryptor operations immediately and without errors on...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
Depends on:
Reported: 2021-03-25 04:01 PDT by Enrique Ocaña
Modified: 2021-04-07 02:53 PDT (History)
13 users (show)

See Also:

Patch (28.61 KB, patch)
2021-03-26 11:24 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (28.66 KB, patch)
2021-04-06 09:21 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (28.61 KB, patch)
2021-04-07 02:23 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Xabier Rodríguez Calvar 2021-04-06 05:01:39 PDT
Comment on attachment 424378 [details]

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]
Comment 4 Xabier Rodríguez Calvar 2021-04-07 00:44:39 PDT
Comment on attachment 425287 [details]

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]
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].