Remove decoder monitor from AudioSink

RESOLVED FIXED in Firefox 42

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1146482
(Assignee)

Comment 2

3 years ago
Try looks green.
(Assignee)

Comment 3

3 years ago
Created attachment 8638281 [details] [diff] [review]
1186801_remove_decoder_monitor-v1.patch

Remove decoder monitor from AudioSink.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
(Assignee)

Comment 4

3 years ago
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.
Attachment #8638281 - Flags: review?(kinetik)
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!
Attachment #8638281 - Flags: review?(kinetik) → review+
(Assignee)

Comment 6

3 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/a8f7b1fb0a71
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.