Closed
Bug 948267
Opened 11 years ago
Closed 8 years ago
Remove the push-based/blocking write code from AudioStream
Categories
(Core :: Audio/Video: Playback, defect, P2)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: kinetik, Assigned: jwwang)
References
Details
Attachments
(3 files)
The plan is to move all users of cubeb to a pull-based model using cubeb's callback. Much of the code in AudioStream becomes unnecessary once this happens. Bug 750596 and bug 848954 cover changing the two main callers. The third (mozWriteAudio) will be removed in bug 927245.
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: MSG/cubeb/GMP
Comment 1•9 years ago
|
||
I presume this has been resolved?
Rank: 25
Flags: needinfo?(padenot)
Priority: -- → P2
Comment 2•8 years ago
|
||
Nope, still going because bug 750596 has not been fixed. We've talked a bit with cpearce, and there might another path that would unify the MSG and normal audio output, in which case this bug would be INVALID, but we haven't decided anything yet. Best to leave this open for now.
Flags: needinfo?(padenot)
Assignee | ||
Comment 3•8 years ago
|
||
Change component to playback since MSG will use cubeb without AudioStream. The only user of AudioStream is DecodedAudioDataSink. This change allows us to reduce buffer copies by removing the circular buffer in AudioStream. It will also remove the audio thread in DecodedAudioDataSink to fix bug 1234529.
Component: Audio/Video: MediaStreamGraph → Audio/Video: Playback
Assignee | ||
Comment 4•8 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88b46f6a016b
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30269/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30269/
Attachment #8706179 -
Flags: review?(kinetik)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30271/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30271/
Attachment #8706180 -
Flags: review?(kinetik)
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/30273/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30273/
Attachment #8706181 -
Flags: review?(kinetik)
Reporter | ||
Updated•8 years ago
|
Attachment #8706179 -
Flags: review?(kinetik) → review+
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8706179 [details] MozReview Request: Bug 948267. Part 1 - add the interface DataSource to implement pull model and remove members no longer useful in the pull model. r=kinetik. https://reviewboard.mozilla.org/r/30269/#review27167 ::: dom/media/AudioStream.cpp:426 (Diff revision 1) > + // DataCallback might be called before we exist this scope "exist" typo?
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8706180 [details] MozReview Request: Bug 948267. Part 2 - implement AudioStream::DataSource for DecodedAudioDataSink and remove its audio thread. r=kinetik. https://reviewboard.mozilla.org/r/30271/#review27173 ::: dom/media/mediasink/DecodedAudioDataSink.cpp:150 (Diff revision 1) > + mAudioStream = audioStream; I think we can initialize mAudioStream directly and dispense with the audioStream temporary now - that only existed so we could separate initialization from publishing the initialized value to another thread. ::: dom/media/mediasink/DecodedAudioDataSink.cpp:173 (Diff revision 1) > - SINK_LOG("AudioLoop started"); > + , mData(aBuffer->mAudioData.get() + aBuffer->mChannels * aOffset) {} Assert somewhere that mData is a valid offset into mBuffer? ::: dom/media/mediasink/DecodedAudioDataSink.cpp:242 (Diff revision 1) > -void > + UniquePtr<AudioStream::Chunk> rv; rv is usually reserved for error code type return values, maybe just call this "chunk" or something here.
Attachment #8706180 -
Flags: review?(kinetik) → review+
Reporter | ||
Comment 10•8 years ago
|
||
Comment on attachment 8706181 [details] MozReview Request: Bug 948267. Part 3 - remove MDSM::AdjustAudioThresholds() to ensure we have enough data for AudioStream::DataCallback() to consume in the audio-only case. https://reviewboard.mozilla.org/r/30273/#review27179
Attachment #8706181 -
Flags: review?(kinetik) → review+
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/30269/#review27167 > "exist" typo? r/exist/exit/
Assignee | ||
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/30271/#review27173 > Assert somewhere that mData is a valid offset into mBuffer? MOZ_ASSERT(aOffect + aFrames <= aBuffer->mFrames) to ensure the entire mData lies within mBuffer->mAudioData.
Assignee | ||
Comment 13•8 years ago
|
||
Thanks for the review!
Assignee | ||
Comment 14•8 years ago
|
||
try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2865dc2744c0
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ec013d048436 https://hg.mozilla.org/mozilla-central/rev/9ce51229f0f9 https://hg.mozilla.org/mozilla-central/rev/0af7319a6a41
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ec013d048436 https://hg.mozilla.org/mozilla-central/rev/9ce51229f0f9 https://hg.mozilla.org/mozilla-central/rev/0af7319a6a41
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 17•8 years ago
|
||
if this patch can mitigate the crash from bug 1234529, could it be uplifted to 45? the mozilla::media::DecodedAudioDataSink::DispatchTask crash signature is currently #25 top crash and causing 0.50 % of all crashes on 45 beta.
Flags: needinfo?(jwwang)
Comment 18•8 years ago
|
||
(In reply to philipp from comment #17) > if this patch can mitigate the crash from bug 1234529, could it be uplifted > to 45? > > the mozilla::media::DecodedAudioDataSink::DispatchTask crash signature is > currently #25 top crash and causing 0.50 % of all crashes on 45 beta. This is a pretty huge patch for beta uplift if it's a non-sec issue. Perhaps there's a simpler solution for the crash? Matthew?
Flags: needinfo?(kinetik)
Comment 19•8 years ago
|
||
the point of thinking there was not to lug avoidable crashes around for another year in 45esr...
Reporter | ||
Comment 20•8 years ago
|
||
I'd prefer to avoid uplifting a large patch if possible, but it's really a question for JW I think.
Flags: needinfo?(kinetik)
Assignee | ||
Comment 21•8 years ago
|
||
I don't like to uplift large patches either. Large patches are high risk. I will rather wait for at least 2 versions for it to stablize in our codebase.
Flags: needinfo?(jwwang)
Depends on: 1255711
No longer depends on: 1255711
Depends on: 1245386
You need to log in
before you can comment on or make changes to this bug.
Description
•