Closed
Bug 1098668
Opened 10 years ago
Closed 9 years ago
MediaDecoderStateMachine::HasLowUndecodedData can call GetAudioClock with mAudioCaptured
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: roc, Assigned: jwwang)
References
Details
Attachments
(1 file, 1 obsolete file)
4.30 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
GetAudioClock asserts !mAudioCaptured, but nothing stops MediaDecoderStateMachine::HasLowUndecodedData calling it with mAudioCapture==true.
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8522618 -
Flags: review?(cpearce)
Updated•10 years ago
|
Attachment #8522618 -
Attachment is patch: true
Attachment #8522618 -
Attachment mime type: message/rfc822 → text/plain
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8522618 [details] [diff] [review] fix Review of attachment 8522618 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1933,5 @@ > + if (AudioQueue().Peek()) { > + endOfDecodedAudioData = AudioQueue().Peek()->GetEndTime(); > + } else if (!mAudioCaptured) { > + endOfDecodedAudioData = GetAudioClock(); > + } The case of |mAudioCaptured == true| is not handled. Does it make sense to assert HasLowUndecodedData() is never called when captured?
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8522618 [details] [diff] [review] fix Review of attachment 8522618 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1933,5 @@ > + if (AudioQueue().Peek()) { > + endOfDecodedAudioData = AudioQueue().Peek()->GetEndTime(); > + } else if (!mAudioCaptured) { > + endOfDecodedAudioData = GetAudioClock(); > + } Sorry, I think not. This is the problem the patch try to fix.
Updated•10 years ago
|
Attachment #8522618 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8522618 [details] [diff] [review] fix Review of attachment 8522618 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +1933,5 @@ > + if (AudioQueue().Peek()) { > + endOfDecodedAudioData = AudioQueue().Peek()->GetEndTime(); > + } else if (!mAudioCaptured) { > + endOfDecodedAudioData = GetAudioClock(); > + } Should we use DecodedStreamData::mLastAudioPacketEndTime when the audio is captured and the audio queue is empty? Otherwise MediaDecoderStateMachine::HasLowUndecodedData will always return false under such a condition.
Assignee | ||
Comment 5•10 years ago
|
||
http://hg.mozilla.org/mozilla-central/annotate/2d0a51ef828d/dom/media/MediaDecoderStateMachine.cpp#l1933 I experienced crash if AudioSink kicks in between 2 AudioQueue().Peek() calls to pop out audio samples and leave an empty AudioQueue. I think we can remember the end time of the last decoded audio sample so we won't have to peek AudioQueue or query audio clock. And we can avoid the problem where |endOfDecodedAudioData| goes backward when AudioQueue is empty because there is latency between the time of an audio sample pushed to audio hardware and the time it is consumed by the audio hardware.
Reporter | ||
Comment 6•10 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #5) > http://hg.mozilla.org/mozilla-central/annotate/2d0a51ef828d/dom/media/ > MediaDecoderStateMachine.cpp#l1933 > > I experienced crash if AudioSink kicks in between 2 AudioQueue().Peek() > calls to pop out audio samples and leave an empty AudioQueue. I think we can > remember the end time of the last decoded audio sample so we won't have to > peek AudioQueue or query audio clock. > > And we can avoid the problem where |endOfDecodedAudioData| goes backward > when AudioQueue is empty because there is latency between the time of an > audio sample pushed to audio hardware and the time it is consumed by the > audio hardware. Sure. Can you write that patch and replace my patch here? Thanks!
Flags: needinfo?(jwwang)
Assignee | ||
Comment 8•10 years ago
|
||
Address comment 5. Remember the end time of the last decoded audio sample so that HasLowUndecodedData doesn't need to peek AudioQueue or query AudioClock. Try: https://tbpl.mozilla.org/?tree=Try&rev=fbb24f7b8258
Attachment #8522618 -
Attachment is obsolete: true
Attachment #8526567 -
Flags: review?(cpearce)
Updated•9 years ago
|
Attachment #8526567 -
Flags: review?(cpearce) → review+
Assignee | ||
Updated•9 years ago
|
Assignee: roc → jwwang
Keywords: checkin-needed
Comment 9•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad7f3e41256
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4ad7f3e41256
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•