RDD does not wait for the decoder to shutdown

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: achronop, Assigned: achronop)

Tracking

(Blocks 1 bug)

66 Branch
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed, firefox68 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 months ago

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

Assignee

Updated

3 months ago
Blocks: 1543360
Blocks: RDD
No longer blocks: 1543360
Priority: P2 → P1
Summary: RDD does not wait the decoder to shutdown → RDD does not wait for the decoder to shutdown
Flags: needinfo?(mfroman)

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 3

3 months 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.

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

Flags: needinfo?(mfroman) → needinfo?(jyavenard)

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

Flags: needinfo?(jyavenard)
Assignee

Comment 6

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

Attachment #9057280 - Attachment description: Bug 1543350 - Clean task queue when shutdown promise has been resolved. → Bug 1543350 - TaskQueue no longer requires to be explicitly shutdown it will shutdown when ref counter is zero.
Attachment #9057280 - Attachment description: Bug 1543350 - TaskQueue no longer requires to be explicitly shutdown it will shutdown when ref counter is zero. → Bug 1543350 - don't shut-down taskqueue early.

Comment 7

2 months ago
Pushed by achronopoulos@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4aebb965d8a0
don't shut-down taskqueue early. r=jya,mjf

Comment 8

2 months ago
bugherder
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → achronop

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

Flags: needinfo?(achronop)
Assignee

Comment 10

2 months 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.

Flags: needinfo?(achronop)
Assignee

Comment 11

2 months 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:
Attachment #9057280 - Flags: approval-mozilla-beta?

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.

Attachment #9057280 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.