1230004 - Remove MediaDecoderStateMachine::mDecoder

Status: RESOLVED FIXED

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

Description

[JW Wang [:jwwang] [:jw_wang]]

This will eliminate the circular reference between MediaDecoder and MDSM, and therefore reducing the bi-directional dependency.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1230004. Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

Bug 1230004. Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Attachment: MozReview Request: Bug 1230004. Part 2 - have MDSM::BeginShutdown return a promise and remove MDSM::mDecoder.

Bug 1230004. Part 2 - have MDSM::BeginShutdown return a promise and remove MDSM::mDecoder.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8695079 [details]
MozReview Request: Bug 1230004. Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

https://reviewboard.mozilla.org/r/26995/#review24391

::: dom/media/MediaDecoderStateMachine.cpp:400
(Diff revision 1)
> -                  mDecoder->GetFrameStatistics(),
> +                  *mFrameStats,

I feel like a GetFrameStatistics() accessor would be nice here. Rather than needing to deref mFrameStats. Up to you though.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8695080 [details]
MozReview Request: Bug 1230004. Part 2 - have MDSM::BeginShutdown return a promise and remove MDSM::mDecoder.

https://reviewboard.mozilla.org/r/26997/#review24397

::: dom/media/MediaDecoderStateMachine.cpp:2166
(Diff revision 1)
> -{
> +MediaDecoderStateMachine::BeginShutdown()

Do we need separate MediaDecoderStateMachine::Shutdown() and MediaDecoderStateMachine::BeginShutdown() methods? It's confusing having two things with similar names.

Can we move the mStreamSink->BeginShutdown() call into MediaDecoderStateMachine::Shutdown(), and just have the caller of MediaDecoderStateMachine::BeginShutdown() just InvokeAsync MediaDecoderStateMachine::Shutdown() instead?

::: dom/media/MediaDecoderStateMachine.cpp:2214
(Diff revision 1)
>    // We must daisy-chain these events to destroy the decoder. We must

This comment is now out of date. It probably can just be removed.

The whole daisy-chain thing here always bugged me. Glad to be rid of the last vestiges of it. :)

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

https://reviewboard.mozilla.org/r/26997/#review24397

> Do we need separate MediaDecoderStateMachine::Shutdown() and MediaDecoderStateMachine::BeginShutdown() methods? It's confusing having two things with similar names.
> 
> Can we move the mStreamSink->BeginShutdown() call into MediaDecoderStateMachine::Shutdown(), and just have the caller of MediaDecoderStateMachine::BeginShutdown() just InvokeAsync MediaDecoderStateMachine::Shutdown() instead?

It is unfortunate DecodedStream::BeginShutdown() must be called on the main thread. I have a todo item to remove it.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Thanks for the review!

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f82dccc7ae89&exclusion_profile=false

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5836cdd7d259
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0be6f556504

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5836cdd7d259
https://hg.mozilla.org/mozilla-central/rev/c0be6f556504

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

Patch for beta.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Part 2 - have MDSM::BeginShutdown return a promise.

This is a rework for beta. It only includes the changes related to having MDSM::Shutdown uses ShutdownPromise. Unfortunately the patch contained also the removal of mDecoder as using ShutdownPromise was the last straw in cleaning up the MDSM.

I'm confident the rebase is good, but another set of eyes would be good. This is required to uplift 1207220 which is a top-crasher

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8703998 [details] [diff] [review]
Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

Approval is for both patches.

Approval Request Comment
[Feature/regressing bug #]: 1230004
[User impact if declined]: Can uplift bug 1207220 that depends on this patch to be applied. bug 1207220 is the proper fix for 1217135 (top crasher)
[Describe test coverage new/current, TreeHerder]: In aurora/central for a while
[Risks and why]: Minimal, code is well tested and fundamentally only allows us to track when an operation has ended, which will be used in bug 1207229. Three set of eyes on patch.
[String/UUID change made/needed]: None

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jean-Yves, is the patch from bug 1207220 sufficient to fix the top crasher bug 1217315? If yes, I am not inclined to take this bug's patch.

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to Ritu Kothari (:ritu) from comment #13)
> Jean-Yves, is the patch from bug 1207220 sufficient to fix the top crasher
> bug 1217315? If yes, I am not inclined to take this bug's patch.

no...

fix from bug 1207220 can't work without this patch being included (and it won't apply either).
The scope of the rebase is much smaller than the original ticket. It was edited to narrow the scope.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8703998 [details] [diff] [review]
Part 1 - cache data in MDSM so it won't need to ask MediaDecoder.

Both Milan and I feel that while these fixes are needed, the timing of these changes is worrisome. Let these ride with Aurora45 -> Beta45. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1207220#c34 and comment 35 for more info.