Closed Bug 1127235 Opened 7 years ago Closed 7 years ago

Refactor stream clock calculation in MediaDecoderStateMachine

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

No description provided.
Depends on: 1127171
Depends on: 1128420
refactor stream clock calculation in MediaDecoderStateMachine.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
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.
Attachment #8562496 - Flags: review?(roc)
Comment on attachment 8562496 [details] [diff] [review]
1127235_refactor_stream_clock_calculation-v1.patch

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

Great!
Attachment #8562496 - Flags: review?(roc) → review+
Blocks: 1128417
Depends on: 1128411
https://hg.mozilla.org/mozilla-central/rev/0459d8d576e4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Can we get this uplifted to 2.2? Please see bug 1139157.
(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.
Blocks: 1139157
No longer depends on: 1139157
Blocks: 1147386
You need to log in before you can comment on or make changes to this bug.