1199121 - Move clock switching code from MDSM into MediaSink

Status: RESOLVED FIXED

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

Description

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

https://hg.mozilla.org/mozilla-central/file/f61c3cc0eb8b/dom/media/MediaDecoderStateMachine.cpp#l2622

MDSM switches from audio clock to system clock when audio reaches the end and video is not played out yet.

We should move the logic into MediaSink so MDSM can always rely on the position reported by the MediaSink. It also greatly simplifies the code when switching audio capture mode.

Comment 1

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

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

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Hi JW,

I'm working on Bug 1194918 (To create a DecodedVideoDataSink and acts like DecodedAudioDataSink pulling data for consumption).
I can make video frames sent to VideoFrameContainer inside DecodedVideoDataSink's thread loop now.
A/V playback seems ok without big problems (roughly test with some mochitest).

One thing bothers me is that when I tried to Move |MDSM::UpdateRenderedVideoFrames()| into DecodedVideoDataSink,  But |MDSM::GetClock()| is needed to be called inside |UpdateRenderedVideoFrames()| to retrieve a reference time for DecodedVideoDataSink dropping/keeping video frames.

In my current local WIP patch, I made |MDSM::GetClock()| public for DecodedVideoDataSink to call, and remove some MOZ_ASSERT(OnTaskQueue) in MDMS functions because these functions need to be called in videoloop thread.

I'm thinking about abstracting |MDSM::GetClock()| and its time-related member variables to a new class call RefMediaClock providing a general logic to offer reference playback clock. And now I see these bugs you created ! 

I traced a little bit and think you're trying to create a clock retrieval machinery in MediaSink which somehow will affect what I'm doing.   I'll reach you tomorrow f2f for more detail.

Comment 3

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

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

Comment 4

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

rebase and try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61b29ad43a56

Comment 5

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

Attachment: MozReview Request: Bug 1199121. Part 1 - add the ability to handle video-only streams to AudioSinkWrapper.

Bug 1199121. Part 1 - add the ability to handle video-only streams to AudioSinkWrapper.
Note AudioSinkWrapper can now report correct playback position when the media is video-only or audio-only.
We will handle the case where audio ends before video in next patch where it needs to switch to system clock when audio reaches the end.

Comment 6

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

Attachment: MozReview Request: Bug 1199121. Part 2 - handle the case where audio ends before video and switch to system clock for calculating playback position.

Bug 1199121. Part 2 - handle the case where audio ends before video and switch to system clock for calculating playback position.

Comment 7

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

Attachment: MozReview Request: Bug 1199121. Part 3 - remove unused code.

Bug 1199121. Part 3 - remove unused code.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8655853 [details]
MozReview Request: Bug 1199121. Part 1 - add the ability to handle video-only streams to AudioSinkWrapper.

https://reviewboard.mozilla.org/r/18031/#review16453

::: dom/media/mediasink/AudioSinkWrapper.cpp:93
(Diff revision 1)
> +    // Return the duration how long we've played if we are not playing.

I think it makes more sense if you remove "the duration" from this comment?

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8655854 [details]
MozReview Request: Bug 1199121. Part 2 - handle the case where audio ends before video and switch to system clock for calculating playback position.

https://reviewboard.mozilla.org/r/18033/#review16455

::: dom/media/mediasink/AudioSinkWrapper.cpp:87
(Diff revision 1)
> -    // Reply on the audio sink to report playback position when we have one.
> +    // Reply on the audio sink to report playback position when it is not ended.

"Reply" should be "Rely" in the comment.  Missed this from the first patch, so better to fix it there.

Comment 10

[Matthew Gregan [:kinetik]]

Comment on attachment 8655855 [details]
MozReview Request: Bug 1199121. Part 3 - remove unused code.

https://reviewboard.mozilla.org/r/18035/#review16457

Comment 11

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

https://reviewboard.mozilla.org/r/18031/#review16453

> I think it makes more sense if you remove "the duration" from this comment?

Sure. Thanks for the review!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d71e93b86d1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b926fd7b6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6af5de32b89b

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d71e93b86d1f
https://hg.mozilla.org/mozilla-central/rev/11b926fd7b6b
https://hg.mozilla.org/mozilla-central/rev/6af5de32b89b