Move clock switching code from MDSM into MediaSink

RESOLVED FIXED in Firefox 43

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1199104
(Assignee)

Updated

2 years ago
Blocks: 1199098

Comment 2

2 years ago
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.
Priority: -- → P2
(Assignee)

Comment 5

2 years ago
Created attachment 8655853 [details]
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.
Attachment #8655853 - Flags: review?(kinetik)
(Assignee)

Comment 6

2 years ago
Created 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.

Bug 1199121. Part 2 - handle the case where audio ends before video and switch to system clock for calculating playback position.
Attachment #8655854 - Flags: review?(kinetik)
(Assignee)

Comment 7

2 years ago
Created attachment 8655855 [details]
MozReview Request: Bug 1199121. Part 3 - remove unused code.

Bug 1199121. Part 3 - remove unused code.
Attachment #8655855 - Flags: review?(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?
Attachment #8655853 - Flags: review?(kinetik) → review+
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.
Attachment #8655854 - Flags: review?(kinetik) → review+
Attachment #8655855 - Flags: review?(kinetik) → review+
(Assignee)

Comment 11

2 years ago
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!
https://hg.mozilla.org/mozilla-central/rev/d71e93b86d1f
https://hg.mozilla.org/mozilla-central/rev/11b926fd7b6b
https://hg.mozilla.org/mozilla-central/rev/6af5de32b89b
Assignee: nobody → jwwang
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.