If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[EME] Implement async shutdown in clearkey GMP

RESOLVED FIXED in Firefox 41

Status

()

Core
Audio/Video
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8620813 [details] [diff] [review]
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)
Attachment #8620813 - Flags: review?(cpearce)
(Assignee)

Comment 2

2 years ago
As a bonus, we can simulate a plugin not shutting down by just removing the ShutdownComplete call from ClearKeyAsyncShutdown::BeginShutdown.
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.
Attachment #8620813 - Flags: review?(cpearce) → review+
(Assignee)

Comment 4

2 years ago
Created attachment 8621442 [details] [diff] [review]
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.
Attachment #8620813 - Attachment is obsolete: true
Attachment #8621442 - Flags: review?(cpearce)
(Assignee)

Comment 5

2 years ago
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.
Attachment #8621442 - Flags: review?(cpearce)
(Assignee)

Updated

2 years ago
See Also: → bug 1175364
(Assignee)

Comment 6

2 years ago
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.
Attachment #8621442 - Flags: review?(cpearce)
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f1a74007e71
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.
Attachment #8621442 - Flags: review?(cpearce) → review+
(Assignee)

Comment 9

2 years ago
(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...
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1175776

Comment 10

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08b29870bef4
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/08b29870bef4
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.