RDD does not wait for the decoder to shutdown
Categories
(Core :: Audio/Video: Playback, defect, P1)
Tracking
()
People
(Reporter: achronop, Assigned: achronop)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
RDD shutdown the decoder in [1] but it does wait for the decoder to shutdown. The Shutdown()
method returns an promise in order to get some time for cleanups. This may require posting a task in TaskQueue. The way that RDD is handling the shutdown TaskQueue may shutdown before decoder's shutdown promise being resolved.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Comment 1•6 years ago
|
||
indeed the RDD must wait for the decoder's shutdown promise to be resolved before it can shutdown the decoder
but all this code can be removed, there's no need to call begin shutdown, following bug 1409664; task queue's will automatically shutdown when needed.
Assignee | ||
Comment 2•6 years ago
|
||
Assignee | ||
Comment 3•6 years ago
|
||
This patch works well, task queue is active during the shutdown but I am trying to break (with the debugger) inside Then()
and it does not break like not being called so I try to figure out why that happens.
Comment 4•6 years ago
|
||
I based RemoteDecoderParent on VideoDecoderParent during the RDD development so the same problem exists in VideoDecoderParent.cpp here[1].
Jean-Yves, should the same fix be applied in VideoDecoderParent as RemoteDecoderParent?
[1] https://searchfox.org/mozilla-central/source/dom/media/ipc/VideoDecoderParent.cpp#246
Comment 5•6 years ago
|
||
(In reply to Michael Froman [:mjf] from comment #4)
I based RemoteDecoderParent on VideoDecoderParent during the RDD development so the same problem exists in VideoDecoderParent.cpp here[1].
Jean-Yves, should the same fix be applied in VideoDecoderParent as RemoteDecoderParent?
[1] https://searchfox.org/mozilla-central/source/dom/media/ipc/VideoDecoderParent.cpp#246
Theorically we do yes.
However, currently the VideoDecoderParent is only used with the WMF decoder, which doesn't dispatch tasks other than the origianl shutdown task, nor use callback. So it's not a problem there.
but it should be done.
Ideally, we should find a way to merge VideoDecoderParent (GPU) with RemoteDecoderParent (RDD)
Assignee | ||
Comment 6•6 years ago
|
||
Continuing from comment 3 the reason that resolve is not called is that at the point we call RemoteDecoderParent::ActorDestroy()
the mDecoder
is already null from RemoteDecoderParent::RecvShutdown()
.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 8•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Comment 9•6 years ago
|
||
The patch is minimal and a P1, Alex is that something we should uplift to beta or is it safer to let it ride the 68 train? Thanks
Assignee | ||
Comment 10•6 years ago
|
||
I would prefer to uplift in beta. Since it has happened in Nightly it can appear in beta too. I will create a request asap.
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 9057280 [details]
Bug 1543350 - don't shut-down taskqueue early.
Beta/Release Uplift Approval Request
- User impact if declined: This solves some low traffic crashes during AV1 playback.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): This is a very small improvement that has been verified in Nightly for some time.
- String changes made/needed:
Comment 12•6 years ago
|
||
Comment on attachment 9057280 [details]
Bug 1543350 - don't shut-down taskqueue early.
Low risk patch that baked a week on Nightly and should prevent some AV1 playback crashes, uplift approved for 67 beta 15, thanks.
Comment 13•6 years ago
|
||
bugherder uplift |
Description
•