1120818 - Ensure AudioSink shutdown is handled properly in MediaDecoderStateMachine

Status: RESOLVED DUPLICATE of bug 1127171

(Core :: Audio/Video, defect)

Description

[Andreas Pehrson [:pehrsons]]

From bug 1113600 comment 59:

I think MediaDecoderStateMachine::SetSyncPointForMediaStream() should happen after AudioSink is completely finished. Otherwise, there will be some audio samples which are played by AudioSink but not counted into |mPlayDuration|. The stream time (near the end) will be inaccurate.

However, that shouldn't be a problem. When you capture in the middle of playback, you can never know the precise time when the capture takes effect.

But, it could be a problem that ResyncAudioClock() happens again in OnAudioSinkComplete() despite it is done already in SetAudioCaptured().

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Only use AudioSink shutdown to resync audio clock

This makes us more consistent on when the audio clock is resynced. The idea is to only and always resync after the AudioSink has finished.

This also means that even if there is an mDecodedStream we might still use the audio clock in GetClock() in case the AudioSink is still running.

I use mAudioCompleted to indicate that we have ResyncAudioClock() and GetDecodedStream()->mStreamInitialized that we have SetSyncPointForMediaStream().

Comment 2

[Andreas Pehrson [:pehrsons]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f285fe7b021e

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8550194 [details] [diff] [review]
Only use AudioSink shutdown to resync audio clock

Review of attachment 8550194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1205,5 @@
>    }
>  
>    mSyncPointInMediaStream = stream->GetLastOutputTime();
>    TimeDuration timeSincePlayStart = mPlayStartTime.IsNull() ? TimeDuration(0) :
>                                      TimeStamp::Now() - mPlayStartTime;

I don't understand why TimeStamp::Now() is used. In the case of capturing an audio-only file, system clock shouldn't be used because it introduce uncentainty and could lead to end time fluctuation.

@@ +1225,5 @@
>    }
> +
> +  // Reset sync point so asserts work again
> +  mSyncPointInMediaStream = -1;
> +  mSyncPointInDecodedStream = -1;

I don't understand this bit. If you set mSyncPointInDecodedStream to -1, you will hit assertion next time GetCurrentTimeViaMediaStreamSync() is called.

@@ +2953,5 @@
>    if (!IsPlaying()) {
>      clock_time = mPlayDuration + mStartTime;
>    } else {
> +    if (mDecoder->GetDecodedStream() &&
> +        mDecoder->GetDecodedStream()->mStreamInitialized) {

It concern me that there might be a gap between |mAudioCompleted| becomes true and |mStreamInitialized| becomes true. If so, clock will switch from audio clock, via system clock, and then to stream clock. We might observe clock going backward in such a case.

@@ +3503,3 @@
>    // AudioSink not used with captured streams, so ignore errors in this case.
> +  if (mDecoder->GetDecodedStream() &&
> +      mDecoder->GetDecodedStream()->mStreamInitialized) {

|mStreamInitialized == true| will only happen after audio thread is shut down which will happen after OnAudioSinkComplete() or OnAudioSinkError(). Therefore the check doesn't make sense for |mStreamInitialized| is always false here.

@@ +3509,5 @@
>    ResyncAudioClock();
>    mAudioCompleted = true;
>  
>    // Make the best effort to continue playback when there is video.
>    if (HasVideo()) {

We should just return when we have video or |mAudioCaptured| is true.

Comment 4

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8550194 [details] [diff] [review]
Only use AudioSink shutdown to resync audio clock

Review of attachment 8550194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1225,5 @@
>    }
> +
> +  // Reset sync point so asserts work again
> +  mSyncPointInMediaStream = -1;
> +  mSyncPointInDecodedStream = -1;

The idea is to only ResyncMediaStreamClock() once. On the next GetClock(), system clock should be used.

Or if we have audio, we should perhaps use the audio clock rather than the system clock, per your comment above.

@@ +2953,5 @@
>    if (!IsPlaying()) {
>      clock_time = mPlayDuration + mStartTime;
>    } else {
> +    if (mDecoder->GetDecodedStream() &&
> +        mDecoder->GetDecodedStream()->mStreamInitialized) {

True that there is a gap where the system clock might be used. It shouldn't go backwards though, since we will always resync before switching to system clock and set a MediaStream sync point before using the MediaStream clock.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8550194 [details] [diff] [review]
Only use AudioSink shutdown to resync audio clock

Review of attachment 8550194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1225,5 @@
>    }
> +
> +  // Reset sync point so asserts work again
> +  mSyncPointInMediaStream = -1;
> +  mSyncPointInDecodedStream = -1;

I see. So ResyncMediaStreamClock() only happens when switching from media stream clock to som other clock.

@@ +2953,5 @@
>    if (!IsPlaying()) {
>      clock_time = mPlayDuration + mStartTime;
>    } else {
> +    if (mDecoder->GetDecodedStream() &&
> +        mDecoder->GetDecodedStream()->mStreamInitialized) {

Ok, I see. But end time fluctuation could be still a problem since SetSyncPointForMediaStream() take into account time elapsed since playback started last time when calculating |timeSincePlayStart|.

Comment 6

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8550194 [details] [diff] [review]
Only use AudioSink shutdown to resync audio clock

Review of attachment 8550194 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2953,5 @@
>    if (!IsPlaying()) {
>      clock_time = mPlayDuration + mStartTime;
>    } else {
> +    if (mDecoder->GetDecodedStream() &&
> +        mDecoder->GetDecodedStream()->mStreamInitialized) {

Yeah, I'll try to find a solution to that for my next patch.

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Part 1. Only use AudioSink shutdown to resync audio clock

Should be no end time fluctuation from SetSyncPointForMediaStream() anymore, with GetMediaTime() instead of (Now()-mPlayStart)+mPlayDuration.

ResyncMediaStreamClock() will only be called during shutdown so we don't have to think about resyncing to audio clock.

I also introduced |MediaDecoderStateMachine::mCaptureToStreamStarted| since,
* We are allowing use of the audio clock until |mAudioCompleted| is true if a stream does not exist or is not initialized,
* Initialization of output streams was tracked on the |DecodedStreamData| object itself,
* The decoded stream is destroyed before the audio sink completes.

So we'd think that the stream was not initialized while it had in fact been initialized and then destroyed.

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Part 2. Rename MediaDecoderStateMachine::mPlayStartTime to mLatestPlayStartTime

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8554406 [details] [diff] [review]
Part 1. Only use AudioSink shutdown to resync audio clock

Review of attachment 8554406 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoder.cpp
@@ +1238,5 @@
>          // algorithm set the current playback position on the main thread,
>          // and we don't want to override the seek algorithm and change the
>          // current time after the seek has started but before it has
>          // completed.
> +        if (GetDecodedStream() && GetDecodedStream()->mStreamInitialized) {

I think we should call mDecoderStateMachine->GetCurrentTime() anyway. The state machine should be responsible for returning correct current time no matter it is captured or not.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1228,5 @@
>      return;
>    }
>  
>    mSyncPointInMediaStream = stream->GetLastOutputTime();
> +  mSyncPointInDecodedStream = GetMediaTime();

This seems wrong to me. GetMediaTime() is updated by AdvanceFrame() which is not in perfect sync with audio clock.

@@ +1242,5 @@
>  
>    if (IsPlaying()) {
> +    // There is no way to uncapture captured media so this resync will only
> +    // happen during shutdown when we switch to system clock. Should it become
> +    // uncapturable we'll need to account for syncing to the audio clock here.

The comment isn't right. ResyncMediaStreamClock() also happens in reponse to MediaDecoderStateMachine::StartSeek().

@@ +3053,5 @@
>    if (mPlaybackRate == 0.0) {
>      return;
>    }
>  
> +  if (mCapturingToStreamStarted) {

The timing has changed. Now SendStreamData() is called only after the stream is initialized. Is that your intention?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

I found video frames are still being rendered on a captured media element while audio is stopped. This is inconsistent. Maybe we should keep audio and video going on a captured media element to save all the trouble in switching to stream clock.

Comment 11

[Andreas Pehrson [:pehrsons]]

I'll have to bow down for JW's great work in bug 1127171, which will clean up the stream clock start.