1269672 - move audible data checking from MDSM to DecodedAudioDataSink

Status: RESOLVED FIXED

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

Description

[Alastor Wu [:alwu]]

After landing bug1264199, the |mAudioData| in AudioData would be empty during the audible checking in MDSM, that is because the audio data was moved out entirely if the input needs to be converted. [1]

We need to do the audible-checking before converting and make new API to inform the MDSM about the audible state.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/mediasink/DecodedAudioDataSink.cpp#458

Comment 1

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

Review commit: https://reviewboard.mozilla.org/r/50731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50731/

Comment 2

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

Review commit: https://reviewboard.mozilla.org/r/50739/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50739/

Comment 3

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=846aac53fee8

Comment 4

[Alastor Wu [:alwu]]

Trying to fix test cases fail, it seems that the audible state would become unstable after applying the patch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aae281bdb50a&selectedJob=20510786

Comment 5

[Alastor Wu [:alwu]]

Comment on attachment 8749079 [details]
MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50731/diff/1-2/

Comment 6

[Alastor Wu [:alwu]]

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50739/diff/1-2/

Comment 7

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=56d25ea2ff2b

Comment 8

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/50739/#review48417

::: dom/media/mediasink/DecodedAudioDataSink.h:22
(Diff revision 2)
>  #include "mozilla/dom/AudioChannelBinding.h"
>  #include "mozilla/Atomics.h"
>  #include "mozilla/Maybe.h"
>  #include "mozilla/MozPromise.h"
>  #include "mozilla/Monitor.h"
> +#include "mozilla/StateMirroring.h"

what you are using is not StateMirrorring, and you do not need to include this header.

::: dom/media/mediasink/DecodedAudioDataSink.cpp:348
(Diff revision 2)
> +DecodedAudioDataSink::CheckIsAudible(const AudioData* aData)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +
> +  bool isAudible = aData->IsAudible();
> +  if (isAudible && !mIsAudioDataAudible) {

would be much simpler with

if (isAudible != mIsAudioAudible {
  mIsAudioDataAudible = isAudible;
  mAudibleEvent.Notify(mIsAudioDataAudible);
}

Comment 9

[Alastor Wu [:alwu]]

Thanks for comment, I'll modify them!

Comment 10

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

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

https://reviewboard.mozilla.org/r/50739/#review48621

::: dom/media/mediasink/DecodedAudioDataSink.cpp:348
(Diff revision 2)
> +DecodedAudioDataSink::CheckIsAudible(const AudioData* aData)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +
> +  bool isAudible = aData->IsAudible();
> +  if (isAudible && !mIsAudioDataAudible) {

Just publish the event anyway. MDSM::mIsAudioDataAudible is a Canonical which can handle duplicated values.

::: dom/media/mediasink/DecodedAudioDataSink.cpp:390
(Diff revision 2)
>      // Ignore the element with 0 frames and try next.
>      if (!data->mFrames) {
>        continue;
>      }
>  
> +    CheckIsAudible(data);

I would prefer to do this in PopFrames() where audio data is really consumed by AudioStream.

Comment 11

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

Comment on attachment 8749079 [details]
MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

https://reviewboard.mozilla.org/r/50731/#review48623

Comment 12

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1269672 - part3 : modify test.

Remake a file which doesn't have the audio-gap, and modify the test.

Review commit: https://reviewboard.mozilla.org/r/51885/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51885/

Comment 13

[Alastor Wu [:alwu]]

Comment on attachment 8749079 [details]
MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50731/diff/2-3/

Comment 14

[Alastor Wu [:alwu]]

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50739/diff/2-3/

Comment 15

[Alastor Wu [:alwu]]

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50739/diff/3-4/

Comment 16

[Alastor Wu [:alwu]]

Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51885/diff/1-2/

Comment 17

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/50739/#review48677

::: dom/media/mediasink/DecodedAudioDataSink.cpp:349
(Diff revisions 2 - 3)
>  DecodedAudioDataSink::CheckIsAudible(const AudioData* aData)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
>  
>    bool isAudible = aData->IsAudible();
> -  if (isAudible && !mIsAudioDataAudible) {
> +  mAudibleEvent.Notify(isAudible);

JW, this isn't a Canonical/Mirror. You will now queue a new task for every packet popped.

this seems highly inneficient to me.

Notify do not check if the value has changed or not.

Comment 18

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

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

https://reviewboard.mozilla.org/r/50739/#review48679

::: dom/media/mediasink/DecodedAudioDataSink.cpp:321
(Diff revision 4)
>  
>    if (needPopping) {
>      // We can now safely pop the audio packet from the processed queue.
>      // This will fire the popped event, triggering a call to NotifyAudioNeeded.
>      RefPtr<AudioData> releaseMe = mProcessedQueue.PopFront();
> +    CheckIsAudible(releaseMe);

It can be just |mAudibleEvent.Notify(releaseMe->IsAudible());|.

::: dom/media/mediasink/DecodedAudioDataSink.cpp:345
(Diff revision 4)
>  }
>  
>  void
> +DecodedAudioDataSink::CheckIsAudible(const AudioData* aData)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());

You will hit the assertion because CheckIsAudible() is called from PopFrames() which runs in the cubeb thread.

Comment 19

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

Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

https://reviewboard.mozilla.org/r/51885/#review48683

::: dom/base/test/mochitest.ini:4
(Diff revision 2)
>  [DEFAULT]
>  support-files =
>    audio.ogg
> -  audioEndedDuringPlaying.webm
> +  audioEndDuringPlaying.mov

What is the format of this .mov file? Is it supported on all platforms?

::: dom/base/test/test_audioNotificationSilent_audioFile.html
(Diff revision 2)
>    expectedPlaybackActive = 'inactive';
>    expectedPlaying = true;
>  }
>  
> -function audioPlayingEnd() {
> +function finish() {
> -  audio.onended = function() {

We no longer wait for 'ended' to finish the test?

Comment 20

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

https://reviewboard.mozilla.org/r/50739/#review48677

> JW, this isn't a Canonical/Mirror. You will now queue a new task for every packet popped.
> 
> this seems highly inneficient to me.
> 
> Notify do not check if the value has changed or not.

I don't really think it will add much overhead as we already queue a new task (NotifyAudioNeeded) for each packet pushed to mAudioQueue. However, I am fine with using mIsAudioDataAudible to reduce events that will be published.

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8749079 [details]
MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50731/diff/3-4/

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50739/diff/4-5/

Comment 23

[Alastor Wu [:alwu]]

Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51885/diff/2-3/

Comment 24

[Alastor Wu [:alwu]]

Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51885/diff/3-4/

Comment 25

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

Comment on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

https://reviewboard.mozilla.org/r/50739/#review48731

Comment 26

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

Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

https://reviewboard.mozilla.org/r/51885/#review48733

Comment 27

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fec08c19ade1&selectedJob=20687136

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd71f17c28af
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd59c1fea61
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd51bec16e0e

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f603bb31593

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dd71f17c28af
https://hg.mozilla.org/mozilla-central/rev/9fd59c1fea61
https://hg.mozilla.org/mozilla-central/rev/cd51bec16e0e
https://hg.mozilla.org/mozilla-central/rev/2f603bb31593