Closed
Bug 1186801
Opened 9 years ago
Closed 9 years ago
Remove decoder monitor from AudioSink
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(1 file)
8.87 KB,
patch
|
kinetik
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
Blocks: MediaMonitor
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Try looks green.
Assignee | ||
Comment 3•9 years ago
|
||
Remove decoder monitor from AudioSink.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 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 5•9 years ago
|
||
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•9 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!
Status: ASSIGNED → RESOLVED
Closed: 9 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.
Description
•