Closed Bug 1543350 Opened 6 years ago Closed 6 years ago

RDD does not wait for the decoder to shutdown

Categories

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

66 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: achronop, Assigned: achronop)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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

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.

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)

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.
Status: NEW → RESOLVED
Closed: 6 years 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)

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)

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.

Attachment

General

Created:
Updated:
Size: