1127235 - Refactor stream clock calculation in MediaDecoderStateMachine

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

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

Attachment: 1127235_refactor_stream_clock_calculation-v1.patch

refactor stream clock calculation in MediaDecoderStateMachine.

Comment 2

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

Comment on attachment 8562496 [details] [diff] [review]
1127235_refactor_stream_clock_calculation-v1.patch

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

Try is green: https://tbpl.mozilla.org/?tree=Try&rev=854eae361f01
with 20+ runs on each platform.

::: dom/media/MediaDecoder.h
@@ +404,5 @@
> +      return mListener->IsFinishedOnMainThread();
> +    }
> +
> +    int64_t GetClock() const {
> +      return mInitialTime + mListener->GetLastOutputTime();

stream clock is simply calculated as |mInitialTime| + |the last stream output time|. |the last stream output time| depends on how much data we feed to the stream in MediaDecoderStateMachine::SendStreamData()

@@ -467,5 @@
> -      MutexAutoLock lock(mMutex);
> -      bool result = !mStreamFinishedOnMainThread;
> -      mStreamFinishedOnMainThread = aFinished;
> -      return result;
> -    }

remove dead code.

::: dom/media/MediaDecoderStateMachine.cpp
@@ -349,5 @@
>      VERBOSE_LOG("writing %lld frames of silence to MediaStream", silentFrames);
>      AudioSegment silence;
> -    StreamTime duration = aStream->mStream->TicksToTimeRoundDown(
> -        mInfo.mAudio.mRate, silentFrames);
> -    silence.InsertNullDataAtStart(duration);

I was wrong about it. Each track has its own track rate which is different from the graph rate. So we don't need the conversion when calling InsertNullDataAtStart().

@@ -627,5 @@
>  
>    // Don't skip frame for video-only decoded stream because the clock time of
>    // the stream relies on the video frame.
> -  if (mDecoder->GetDecodedStream() && !HasAudio()) {
> -    DECODER_LOG("Video-only decoded stream, set skipToNextKeyFrame to false");

This log message is too verbose. Let's remove it.

@@ -1347,5 @@
> -    if (HasAudio()) {
> -      // The audio clock is active so force a resync now in case the audio
> -      // clock is ahead of us (observed on Android), since after mAudioCaptured
> -      // gets set can't call GetAudioClock().
> -      ResyncAudioClock();

We don't care about audio clock any more since we will start to use stream clock from now on. So don't bother to re-sync audio clock.

@@ +3085,5 @@
> +  // Note we have to update playback position before releasing the monitor.
> +  // Otherwise, MediaDecoder::AddOutputStream could kick in when we are outside
> +  // the monitor and get a staled value from GetCurrentTimeUs() which hits the
> +  // assertion in GetClock().
> +

Note we have to update playback position here before releasing the monitor.

::: dom/media/MediaDecoderStateMachine.h
@@ -361,5 @@
> -  // |mPlayDuration| are updated to provide a good base for calculating video
> -  // stream time using the system clock.
> -  void ResyncMediaStreamClock();
> -  int64_t GetCurrentTimeViaMediaStreamSync() const;
> -

We don't need these functions any more since stream clock is calculated solely by the decoded stream without the help of the state machine.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8562496 [details] [diff] [review]
1127235_refactor_stream_clock_calculation-v1.patch

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

Great!

Comment 4

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

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0459d8d576e4

Note: bug 1127235, bug 1128417 and bug 901102 must be landed together.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0459d8d576e4

Comment 6

[KTucker [:KTucker][Inactive 3/4/2016]]

Can we get this uplifted to 2.2? Please see bug 1139157.

Comment 7

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

(In reply to KTucker [:KTucker] from comment #6)
> Can we get this uplifted to 2.2? Please see bug 1139157.

The change is huge. I will find a simpler fix for bug 1139157.