1184874 - Remove dependency on MediaDecoderStateMachine::DispatchOnPlaybackOffsetUpdate from AudioSink

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

This bug reduces bi-directional dependency between MDSM and AudioSink and makes it easier to unify AudioSink and DecodedStream.

Comment 1

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

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

Comment 2

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

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

Comment 3

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

Attachment: 1184874_part1_fix_indentation-v1.patch

Fix indentation.

Comment 4

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

Attachment: 1184874_part2_add_MediaQueue_PopListener-v1.patch

Part 2 - add the ability to know which item is popped from the MediaQueue.

Comment 5

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

Attachment: 1184874_part3_remove_DispatchOnPlaybackOffsetUpdate-v1.patch

Part 3 - call mDecoder->UpdatePlaybackOffset in On{Audio,Video}Popped and remove MediaDecoderStateMachine::DispatchOnPlaybackOffsetUpdate.

Comment 6

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

Comment on attachment 8636932 [details] [diff] [review]
1184874_part3_remove_DispatchOnPlaybackOffsetUpdate-v1.patch

Review of attachment 8636932 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +688,5 @@
>  MediaDecoderStateMachine::OnAudioPopped(const AudioData* aSample)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  // TODO: could mOffset be -1?

It mOffset should not be < 0. You could assert that. Or even do:

mDecoder->UpdatePlaybackOffset(std::max(0, aSample->mOffset));

Comment 7

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

Comment on attachment 8636932 [details] [diff] [review]
1184874_part3_remove_DispatchOnPlaybackOffsetUpdate-v1.patch

Review of attachment 8636932 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +688,5 @@
>  MediaDecoderStateMachine::OnAudioPopped(const AudioData* aSample)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    ReentrantMonitorAutoEnter mon(mDecoder->GetReentrantMonitor());
> +  // TODO: could mOffset be -1?

The code is moved from AudioSink. Sure, I will adjust the value to be non-negative.

Comment 8

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

Thanks for the review.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3bb1b5861b
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8dd2f681ce8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4625fdae1cc3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a3bb1b5861b
https://hg.mozilla.org/mozilla-central/rev/f8dd2f681ce8
https://hg.mozilla.org/mozilla-central/rev/4625fdae1cc3