1543350 - RDD does not wait for the decoder to shutdown

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Alex Chronopoulos [:achronop]]

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.

[1] https://searchfox.org/mozilla-central/rev/8d78f219702286c873860f39f9ed78bad1a6d062/dom/media/ipc/RemoteDecoderParent.cpp#159

Comment 1

[Jean-Yves Avenard [:jya]]

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.

Comment 2

[Alex Chronopoulos [:achronop]]

Attachment: Bug 1543350 - don't shut-down taskqueue early.

Comment 3

[Alex Chronopoulos [:achronop]]

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

[Michael Froman [:mjf]]

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

[Jean-Yves Avenard [:jya]]

(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)

Comment 6

[Alex Chronopoulos [:achronop]]

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().

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4aebb965d8a0
don't shut-down taskqueue early. r=jya,mjf

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4aebb965d8a0

Comment 9

[Pascal Chevrel:pascalc]

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

Comment 10

[Alex Chronopoulos [:achronop]]

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.

Comment 11

[Alex Chronopoulos [:achronop]]

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

[Pascal Chevrel:pascalc]

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

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/66d938a1a824