1178718 - Remove dependency on decoder monitor from DecodedStream

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

In order to remove decoder monitor from MDSM, we need to remove dependency on decoder monitor from DecodedStream.

Comment 1

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8790d27dc97d

Comment 2

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

Attachment: 1178718_remove_monitor_from_DecodedStream-v1.patch

Let DecodedStream have its own monitor.

Comment 3

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

Comment on attachment 8627617 [details] [diff] [review]
1178718_remove_monitor_from_DecodedStream-v1.patch

Try looks green. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8790d27dc97d

Comment 4

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

Comment on attachment 8627617 [details] [diff] [review]
1178718_remove_monitor_from_DecodedStream-v1.patch

Review of attachment 8627617 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3417,5 @@
> +  nsRefPtr<MediaDecoderStateMachine> self = this;
> +  nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction([self] () -> void
> +  {
> +    ReentrantMonitorAutoEnter mon(self->mDecoder->GetReentrantMonitor());
> +    self->mDecodedStream.SetPlaying(self->IsPlaying());

IsPlaying() should only be called on the state machine thread.

Comment 5

[Bobby Holley (:bholley)]

Thanks for working on this!

I still think that all the MDSM methods (aside from ctor, dtor, and Init) should run on the state machine thread - having a few random methods (RecreateDecodedStream, AddOutputStream, and GetDecodedStream) running on the main thread seems like the wrong design.

What about creating a "DecodedStreamManager" object, whose methods run on the main thread and use mirroring + dispatch to communicate with the state machine?

Comment 6

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

(In reply to Bobby Holley (:bholley) from comment #5)
> What about creating a "DecodedStreamManager" object, whose methods run on
> the main thread and use mirroring + dispatch to communicate with the state
> machine?

Not sure about how your DecodedStreamManager is different from my DecodedStream in design. My plan was to move all capture-stream related code into DecodedStream and make it thread safe. Then it can be called on any thread. This is the active object pattern which allows DecodedStream to hide thread details from MDSM.

I am still half way there. But this patch should help your work continue.

Comment 7

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

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=98b0efd87194

Comment 8

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

Attachment: 1178718_part2_no_acquire_monitor_in_BreakCycles-v1.patch

We don't need to acquire the decoder monitor when calling mDecodedStream.DestroyData().

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8627617 [details] [diff] [review]
1178718_remove_monitor_from_DecodedStream-v1.patch

Review of attachment 8627617 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not super happy about adding a new lock here. Can you at least document the locking order to ensure we don't create deadlocks?

Comment 10

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> I'm not super happy about adding a new lock here. Can you at least document
> the locking order to ensure we don't create deadlocks?

DecodedStream should never call back into MDSM funcitons so the locking order will never be reversed and deadlocks can be prevented. By having DecodedStream have its own lock, we increase the autonomy of DecodedStream without depending on the decoder monitor and make it thread-safe to be able to called on any thread.

I will add a comment to note DecodedStream should not call back to its client object to avoid deadlocks. If it ever becomes necessary in the future, the lock needs to be released before the callback.

Comment 11

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

Usually we don't have to worry about deadlocks when object dependency is one way instead of bi-directional. Since the object dependency between MediaDecoder and MediaDecoderStateMachine is two-way, we chose to share the same decoder monitor without worrying about the locking order. There are also cases like AudioStream and libcubeb where AudioStream has to release its own lock whenever calling libcubeb functions since libcubeb could call back into AudioStream with its own lock held.

Therefore, DecodedStream is designed to avoid bi-directional dependency in mind at the beginning to avoid sophisticated locking order.

Comment 12

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #6)
> Not sure about how your DecodedStreamManager is different from my
> DecodedStream in design.

From a very high level, the difference is that my proposal is intended to avoid having any MDSM methods run on the main thread, whereas the patch in this bug has us doing that.

> My plan was to move all capture-stream related code
> into DecodedStream and make it thread safe.
...
> I am still half way there. But this patch should help your work continue.

Ok. This patch certainly improves the current situation, but not to the level we were at before bug 1163467 landed. If your intention is to fix that up soon, I'm fine with this as an interim fix, though I'd also be fine to wait a few more days and have it all together.

Comment 13

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

It can't be finished in a week time. I will move all capture-stream related code into DecodedStream and I should be able to apply mirroring + dispatch as you suggested to remove the temporary monitor introduced in this bug. All DecodedStream should run in the main thread as is required for MediaStream functions. Does that meet your design?

Comment 14

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

Attachment: 1178718_part1_remove_monitor_from_DecodedStream-v2.patch

Add comments.

See the discussion from comment 10 to comment 13 for the rationale behind this temp solution.

Comment 15

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

Thanks for the review!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae899f88106
https://hg.mozilla.org/integration/mozilla-inbound/rev/50795f61169b

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ae899f88106
https://hg.mozilla.org/mozilla-central/rev/50795f61169b

Comment 18

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #13)
> It can't be finished in a week time. I will move all capture-stream related
> code into DecodedStream and I should be able to apply mirroring + dispatch
> as you suggested to remove the temporary monitor introduced in this bug. All
> DecodedStream should run in the main thread as is required for MediaStream
> functions. Does that meet your design?

I think so, assuming that we don't need to call MDSM methods on the main thread to do this work. I leave it in your capable hands! :-)