After we get rid of the bi-directional dependency between MDSM and AudioSink, we can have AudioSink own its monitor without worrying about deadlocks. The bug is required to remove the decoder monitor of bug 1146482.

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

Try looks green.

Remove decoder monitor from AudioSink.

Comment on attachment 8638281 [details] [diff] [review] 1186801_remove_decoder_monitor-v1.patch Review of attachment 8638281 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ -1054,5 @@ > SetPlayStartTime(TimeStamp()); > } > - // Notify the audio sink, so that it notices that we've stopped playing, > - // so it can pause audio playback. > - mDecoder->GetReentrantMonitor().NotifyAll(); AudioSink::SetPlaying will call NotifyAll(). We don't need this code anymore.

Comment on attachment 8638281 [details] [diff] [review] 1186801_remove_decoder_monitor-v1.patch Review of attachment 8638281 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/AudioSink.h @@ +54,4 @@ > > + // Wake up the audio loop if it is waiting for data to play or the audio > + // queue is finished. > + void NotifyData(); It'd be nice if we could tie this directly to what it's waiting on, i.e. a change in AudioQueue state, so that callers don't need to remember to call this everywhere that might be necessary. But I'm fine with this as-is for now!

Yes, that is what I am planning to do. It is unfortunate that the notification model of MediaQueue don't fit well into the audio loop employed by AudioSink. The next step is refactor the audio loop into a series of event handlers so we can register AudioSink with the push/pop/finish listeners of MediaQueue. And then we can remove AudioSink::NotifyData without the help of MDSM. Thanks for the review!