1173631 - [EME] Implement async shutdown in clearkey GMP

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Some issues may be present around async shutdown of GMPs, e.g.: bug 1164264.
It would be useful to implement async shutdown in gmp-clearkey, to make it easier to test async shutdown in some situations where we can use clearkey.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1173631-clearkey-async-shutdown.patch

Here's one I prepared earlier.
Note that it is simply returning a "ShutdownComplete" straight away, assuming that other decrypting/decoding operations have already ceased (which seems to be the case in my limited testing).

Chris, if you think we should do a proper check and delay shutdown if necessary, just r- this patch.
Though I would argue that since we previously didn't have the async shutdown capability, the previous code would have worked pretty much the same way; So at the very least this patch should not make things worse, but at least it exercises the async-shutdown path from the GMPService point of view.

If necessary we could make another bug&patch for a better async shutdown implementation that ensures the whole GMP is properly shutdown.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2bbb83a020a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e41213f9bf (2nd try just to verify build after adding a missing 'explicit' to the first tried patch)

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

As a bonus, we can simulate a plugin not shutting down by just removing the ShutdownComplete call from ClearKeyAsyncShutdown::BeginShutdown.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8620813 [details] [diff] [review]
1173631-clearkey-async-shutdown.patch

Review of attachment 8620813 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/gmp-clearkey/0.1/ClearKeyAsyncShutdown.cpp
@@ +31,5 @@
> +{
> +  // Dumb implementation that just immediately reports completion.
> +  // Real GMPs should ensure they are properly shutdown.
> +  CK_LOGD("ClearKeyAsyncShutdown::BeginShutdown calling mHost->ShutdownComplete");
> +  mHost->ShutdownComplete();

Can you please call ShutdownComplete() in a task you dispatch to the main thread, so that shutdown is actually async, so that people who copy our GMP don't end up thinking they should do everything synchronously here.

::: media/gmp-clearkey/0.1/ClearKeyAsyncShutdown.h
@@ +20,5 @@
> +#include "gmp-async-shutdown.h"
> +#include "RefCounted.h"
> +
> +class ClearKeyAsyncShutdown : public GMPAsyncShutdown
> +                            , public RefCounted

You make this refcounted, but you never addref it. Better to not make it refcounted I think.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1173631-clearkey-async-shutdown.patch

New r? because of "interesting" change in GMPChild.cpp: Resetting mAsyncShutdown to nullptr in ShutdownComplete, so we won't call BeginShutdown again. This may play well with your new bug 1174064?

Also related, I've actually added AddRef/Release in ClearKeyAsyncShutdown (instead of making it not RefCounted), seemed a bit cleaner not to leak.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8621442 [details] [diff] [review]
1173631-clearkey-async-shutdown.patch

Hold the press!
It seems this patch is causing a hang or crash when shutting down while both clearkey decoding and webrtc de/encoding are working.

Steps:
1. Launch Firefox
2. Go to http://people.mozilla.org/~cpearce/mse-clearkey/ , start video
3. Open a 2nd tab, go to https://mozilla.github.io/webrtc-landing/pc_test.html
4. Check 'Require H.264 video'
5. Start (and optionally authorize access to camera and/or mic)
6. (Optional) Switch to 1st tab
7. Close Firefox
Different sorts of trouble may happen: webrtc::RTCPReceiver::SetSsrcs waiting on a mutex, assertion trying to stop decode thread, or something else...)

Will investigate; Maybe clearkey's async shutdown really needs to do a proper job of shutting done decoding/encoding threads.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8621442 [details] [diff] [review]
1173631-clearkey-async-shutdown.patch

The above issue was not due to this patch (see bug 1175364), so it's back for review.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f1a74007e71

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8621442 [details] [diff] [review]
1173631-clearkey-async-shutdown.patch

Review of attachment 8621442 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/gmp/GMPChild.cpp
@@ +586,5 @@
>  GMPChild::ShutdownComplete()
>  {
>    LOGD("%s", __FUNCTION__);
>    MOZ_ASSERT(mGMPMessageLoop == MessageLoop::current());
> +  mAsyncShutdown = nullptr;

This should not be needed (anymore?); since bug 1174064 tries to ensure that we never reuse a GMP once it's started its async shutdown, we should never end up doing async shutdown twice. If we do that's a bug, which arguably we should MOZ_DIAGNOSTIC_ASSERT to detect.

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8621442 [details] [diff] [review]
> 1173631-clearkey-async-shutdown.patch
> 
> Review of attachment 8621442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/gmp/GMPChild.cpp
> @@ +586,5 @@
> >  GMPChild::ShutdownComplete()
> >  {
> >    LOGD("%s", __FUNCTION__);
> >    MOZ_ASSERT(mGMPMessageLoop == MessageLoop::current());
> > +  mAsyncShutdown = nullptr;
> 
> This should not be needed (anymore?); since bug 1174064 tries to ensure that
> we never reuse a GMP once it's started its async shutdown, we should never
> end up doing async shutdown twice. If we do that's a bug, which arguably we
> should MOZ_DIAGNOSTIC_ASSERT to detect.

After discussion with Chris, we've decided to leave this patch as-is, and spawn a separate bug to tackle expected invariants around shutdown...

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08b29870bef4

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08b29870bef4