Closed Bug 1203418 Opened 4 years ago Closed 4 years ago

Make MediaDecoder::GetStatistics and MediaDecoder::ComputePlaybackRate run on the main thread

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

No description provided.
Blocks: 1179498
Priority: -- → P2
Assignee: nobody → jwwang
Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM.
Since the results of ComputePlaybackRate() depend on mDuration and mPlaybackStatistics,
we call ComputePlaybackRate() when mDuration is changed or mPlaybackStatistics->Stop() is called.
GetStatistics() won't have to call ComputePlaybackRate() because the results are already stored in
mPlaybackBytesPerSecond and mPlaybackRateReliable.
We will fix the MDSM part in the next patch.
Attachment #8659689 - Flags: review?(cpearce)
Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread.
Attachment #8659690 - Flags: review?(cpearce)
https://reviewboard.mozilla.org/r/18997/#review16877

::: dom/media/MediaDecoderStateMachine.cpp:708
(Diff revision 1)
> +  mPlaybackOffset = offset;

I wonder if this should be:

mPlaybackOffset = max(aSample->mOffset, mPlaybackOffset)

With mPlaybackOffset reset when we seek/reset playback.

Doing this would reduce the "jitter" caused by audio and video samples offset values not always increasing exactly.

Ditto for the OnVideoPopped() case.
Comment on attachment 8659689 [details]
MozReview Request: Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM. r=cpearce.

https://reviewboard.mozilla.org/r/18999/#review16879
Attachment #8659689 - Flags: review?(cpearce) → review+
Attachment #8659690 - Flags: review?(cpearce) → review+
Comment on attachment 8659690 [details]
MozReview Request: Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread. r=cpearce.

https://reviewboard.mozilla.org/r/19001/#review16881
(In reply to Chris Pearce (:cpearce) from comment #4)
> https://reviewboard.mozilla.org/r/18997/#review16877
> 
> ::: dom/media/MediaDecoderStateMachine.cpp:708
> (Diff revision 1)
> > +  mPlaybackOffset = offset;
> 
> I wonder if this should be:
> 
> mPlaybackOffset = max(aSample->mOffset, mPlaybackOffset)
> 
> With mPlaybackOffset reset when we seek/reset playback.
> 
> Doing this would reduce the "jitter" caused by audio and video samples
> offset values not always increasing exactly.
> 
> Ditto for the OnVideoPopped() case.

Totally makes sense to me. I think MediaDecoder::mPlaybackPosition should be also mono-increasing.
Comment on attachment 8659689 [details]
MozReview Request: Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM. r=cpearce.

Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM. r=cpearce.
Since the results of ComputePlaybackRate() depend on mDuration and mPlaybackStatistics,
we call ComputePlaybackRate() when mDuration is changed or mPlaybackStatistics->Stop() is called.
GetStatistics() won't have to call ComputePlaybackRate() because the results are already stored in
mPlaybackBytesPerSecond and mPlaybackRateReliable.
We will fix the MDSM part in the next patch.
Attachment #8659689 - Attachment description: MozReview Request: Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM. → MozReview Request: Bug 1203418. Part 1 - cache the results of ComputePlaybackRate() so they can be mirrored by MDSM. r=cpearce.
Comment on attachment 8659690 [details]
MozReview Request: Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread. r=cpearce.

Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread. r=cpearce.
Attachment #8659690 - Attachment description: MozReview Request: Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread. → MozReview Request: Bug 1203418. Part 2 - duplicate the implementation of MediaDecoder::GetStatistics so MDSM can call it on its own thread. r=cpearce.
Bug 1203418. Part 3 - ensure MDSM::mPlaybackOffset and MediaDecoder::mPlaybackPosition are mono-increasing to avoid "jitter" in calculating playback statistics.
Attachment #8659755 - Flags: review?(cpearce)
Blocks: 1203877
Comment on attachment 8659755 [details]
MozReview Request: Bug 1203418. Part 3 - ensure MDSM::mPlaybackOffset and MediaDecoder::mPlaybackPosition are mono-increasing to avoid "jitter" in calculating playback statistics.

https://reviewboard.mozilla.org/r/19013/#review17115
Attachment #8659755 - Flags: review?(cpearce) → review+
Thanks for the review.
You need to log in before you can comment on or make changes to this bug.