Bug 223742 – [EME][GStreamer] Abort decryptor operations immediately and without errors on flush

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Enrique Ocaña]

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.

[Enrique Ocaña]

Created attachment 424378 [details]
Patch

[Xabier Rodríguez Calvar]

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?

[Enrique Ocaña]

Created attachment 425287 [details]
Patch

[Xabier Rodríguez Calvar]

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.

[Enrique Ocaña]

Created attachment 425370 [details]
Patch

[EWS]

Committed r275599: <https://commits.webkit.org/r275599>

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