Closed Bug 1098668 Opened 10 years ago Closed 9 years ago

MediaDecoderStateMachine::HasLowUndecodedData can call GetAudioClock with mAudioCaptured

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: roc, Assigned: jwwang)

References

Details

Attachments

(1 file, 1 obsolete file)

GetAudioClock asserts !mAudioCaptured, but nothing stops MediaDecoderStateMachine::HasLowUndecodedData calling it with mAudioCapture==true.
Attached patch fix (obsolete) — Splinter Review
Attachment #8522618 - Flags: review?(cpearce)
Attachment #8522618 - Attachment is patch: true
Attachment #8522618 - Attachment mime type: message/rfc822 → text/plain
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?
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.
Attachment #8522618 - Flags: review?(cpearce) → review+
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.
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.
Blocks: 1099193
(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)
Sure. Thanks.
Flags: needinfo?(jwwang)
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)
Attachment #8526567 - Flags: review?(cpearce) → review+
Assignee: roc → jwwang
Keywords: checkin-needed
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.