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

Status: RESOLVED FIXED

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

Comment 1

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

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

Comment 2

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

Attachment: 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.
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.

Comment 3

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

Attachment: 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.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 7

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

(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 8

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

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.

Comment 9

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

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.

Comment 10

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

Attachment: MozReview Request: Bug 1203418. Part 3 - ensure MDSM::mPlaybackOffset and MediaDecoder::mPlaybackPosition are mono-increasing to avoid "jitter" in calculating playback statistics.

Bug 1203418. Part 3 - ensure MDSM::mPlaybackOffset and MediaDecoder::mPlaybackPosition are mono-increasing to avoid "jitter" in calculating playback statistics.

Comment 11

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

Try for part 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab09e974b145

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 13

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

Thanks for the review.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cc8d9beba22
https://hg.mozilla.org/integration/mozilla-inbound/rev/20f737a2b959
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d2b0c046a3

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1cc8d9beba22
https://hg.mozilla.org/mozilla-central/rev/20f737a2b959
https://hg.mozilla.org/mozilla-central/rev/c2d2b0c046a3