1199104 - Create a subclass of MediaSink to wrap DecodedAudioDataSink for audio rendering

Status: RESOLVED FIXED

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

Description

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

Step 1 & 2 of bug 1199098.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: MozReview Request: Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

Bug 1199104. Part 1 - create MediaSink.

Comment 4

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

Attachment: MozReview Request: Bug 1199104. Part 2 - create AudioSinkWrapper. r=kinetik.

Bug 1199104. Part 2 - create AudioSinkWrapper.

Comment 5

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

Attachment: MozReview Request: Bug 1199104. Part 3 - use AudioSinkWrapper in MDSM. r=kinetik.

Bug 1199104. Part 3 - use AudioSinkWrapper in MDSM.

Comment 6

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

Comment on attachment 8654066 [details]
MozReview Request: Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

Need more insights to make sure the interface is designed correctly.

Comment 7

[Matthew Gregan [:kinetik]]

Comment on attachment 8654068 [details]
MozReview Request: Bug 1199104. Part 3 - use AudioSinkWrapper in MDSM. r=kinetik.

https://reviewboard.mozilla.org/r/17601/#review15927

::: dom/media/MediaDecoderStateMachine.cpp:2401
(Diff revision 1)
> -        // return an incorrect value due to a null mAudioSink.
> +        // return an incorrect value due to mAudioSink is stopped.

"being stopped"?

::: dom/media/MediaDecoderStateMachine.cpp:3040
(Diff revision 1)
> -  // "before StartAudioSink" and "after StopAudioSink" where mAudioSink
> +  // valid only when playback is in action.

Since we don't null mAudioSink now, we can tell the difference between "never had one/not yet started" and "reached end", and the comment that the audio end time can't be valid outside of playback doesn't feel right to me.  Can we change it so that the sink just continues to return a valid end time after it ends? Would that make sense?

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8654067 [details]
MozReview Request: Bug 1199104. Part 2 - create AudioSinkWrapper. r=kinetik.

https://reviewboard.mozilla.org/r/17599/#review15929

I think we could just modify the audio sink to behave the way we want it to directly rather than wrapped in, but I'm not sure if you have other plans here.

Comment 9

[Matthew Gregan [:kinetik]]

Comment on attachment 8654066 [details]
MozReview Request: Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

https://reviewboard.mozilla.org/r/17597/#review15931

::: dom/media/mediasink/MediaSink.h:67
(Diff revision 1)
> +  // Must be called after playback starts.

It seems like this could be called in any start, since before playback begins it could return the start time?

Comment 10

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

https://reviewboard.mozilla.org/r/17601/#review15927

> Since we don't null mAudioSink now, we can tell the difference between "never had one/not yet started" and "reached end", and the comment that the audio end time can't be valid outside of playback doesn't feel right to me.  Can we change it so that the sink just continues to return a valid end time after it ends? Would that make sense?

This is a design decision in order to keep the semantics of MediaSink as simple as possible. Should we continue to return a valid end time of last playback session? Or should we return -1 once playback is stopped? Or should GetEndTime() be called only after playback starts as well as GetPosition()? I think 3. is simpler than the previous 2.

Here is the revised design:
GetEndTime() returns the end time of the audio/video data that has been consumed or -1 if no such track. Must be called after playback starts.

There are 3 places in MDSM to call GetEndTime():
1. case DECODER_STATE_COMPLETED: MediaSink is guaranteed to be already started. The revised semantics fits well.
2. UpdateRenderedVideoFrames(): as above
3. GetDecodedAudioDuration() and AudioDecodedUsecs(): GetEndTime() is used to account for audio pushed but not played by the hardware. The value is simply 0 if MedisSink is not Start()ed.

Thought?

Comment 11

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

https://reviewboard.mozilla.org/r/17597/#review15943

::: dom/media/mediasink/MediaSink.h:67
(Diff revision 1)
> +  // Must be called after playback starts.

The start time is provided in Start() and the calculation of playback position depends on the start time. Therefore GetPosition() should only be called after Start()ed.

Comment 12

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

https://reviewboard.mozilla.org/r/17599/#review15929

This indirection (a wrapper) is to make bug 1199121 easier where we want to handle clock switching (to system clock) if audio ends before video. This is beyond the capability of an AudioSink and therefore I decide to put it in a wrapper. When VideoSink is done by Kilik, this wrapper will be exteneded to include both an AudioSink and a VideoSink to fulfill the spec of MediaSink (A/V sync and audio/video rendering).

Comment 13

[Matthew Gregan [:kinetik]]

https://reviewboard.mozilla.org/r/17597/#review15943

> The start time is provided in Start() and the calculation of playback position depends on the start time. Therefore GetPosition() should only be called after Start()ed.

Good point -- missed that as the current implementation ignores the start time from Start().

Comment 14

[Matthew Gregan [:kinetik]]

https://reviewboard.mozilla.org/r/17601/#review15927

> This is a design decision in order to keep the semantics of MediaSink as simple as possible. Should we continue to return a valid end time of last playback session? Or should we return -1 once playback is stopped? Or should GetEndTime() be called only after playback starts as well as GetPosition()? I think 3. is simpler than the previous 2.
> 
> Here is the revised design:
> GetEndTime() returns the end time of the audio/video data that has been consumed or -1 if no such track. Must be called after playback starts.
> 
> There are 3 places in MDSM to call GetEndTime():
> 1. case DECODER_STATE_COMPLETED: MediaSink is guaranteed to be already started. The revised semantics fits well.
> 2. UpdateRenderedVideoFrames(): as above
> 3. GetDecodedAudioDuration() and AudioDecodedUsecs(): GetEndTime() is used to account for audio pushed but not played by the hardware. The value is simply 0 if MedisSink is not Start()ed.
> 
> Thought?

I agree, that sounds good, thanks.

Comment 15

[Matthew Gregan [:kinetik]]

Comment on attachment 8654066 [details]
MozReview Request: Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

https://reviewboard.mozilla.org/r/17597/#review15953

(not sure why mozreview cancelled my review request in bugzilla, I thought I reviewed this)

Comment 16

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

Comment on attachment 8654066 [details]
MozReview Request: Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

Bug 1199104. Part 1 - create MediaSink. r=kinetik. r?cpearce.

Comment 17

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

Comment on attachment 8654067 [details]
MozReview Request: Bug 1199104. Part 2 - create AudioSinkWrapper. r=kinetik.

Bug 1199104. Part 2 - create AudioSinkWrapper. r=kinetik.

Comment 18

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

Comment on attachment 8654068 [details]
MozReview Request: Bug 1199104. Part 3 - use AudioSinkWrapper in MDSM. r=kinetik.

Bug 1199104. Part 3 - use AudioSinkWrapper in MDSM. r=kinetik.

Comment 19

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

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

Comment 20

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

Since Chris is on PTO, I will take r+ from kinetik and check it the code.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70fae4852730
https://hg.mozilla.org/integration/mozilla-inbound/rev/209f45821b75
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dcb70eb5798

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70fae4852730
https://hg.mozilla.org/mozilla-central/rev/209f45821b75
https://hg.mozilla.org/mozilla-central/rev/0dcb70eb5798