Closed Bug 1269672 Opened 8 years ago Closed 8 years ago

move audible data checking from MDSM to DecodedAudioDataSink

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

Attachments

(3 files)

MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.
58 bytes, text/x-review-board-request
jwwang
: review+
Details
MozReview Request: Bug 1269672 - part3 : modify test.
58 bytes, text/x-review-board-request
jwwang
: review+
Details 
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
Blocks: 1264199
Blocks: 1240423
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 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 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/
Attachment #8749079 - Flags: review?(jwwang)
Attachment #8749080 - Flags: review?(jwwang)
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);
}
Thanks for comment, I'll modify them!
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.
Attachment #8749080 - Flags: review?(jwwang)
Comment on attachment 8749079 [details]
MozReview Request: Bug 1269672 - part1 : revert sampling rate changing of the bug1235612.

https://reviewboard.mozilla.org/r/50731/#review48623
Attachment #8749079 - Flags: review?(jwwang) → review+
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/
Attachment #8751152 - Flags: review?(jwwang)
Attachment #8749080 - Flags: review?(jwwang)
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 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 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 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/
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 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.
Attachment #8749080 - Flags: review?(jwwang)
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?
Attachment #8751152 - Flags: review?(jwwang)
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 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/
Attachment #8749080 - Flags: review?(jwwang)
Attachment #8751152 - Flags: review?(jwwang)
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 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 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 on attachment 8749080 [details]
MozReview Request: Bug 1269672 - part2 : move audible data checking from MDSM to DecodedAudioDataSink.

https://reviewboard.mozilla.org/r/50739/#review48731
Attachment #8749080 - Flags: review?(jwwang) → review+
Comment on attachment 8751152 [details]
MozReview Request: Bug 1269672 - part3 : modify test.

https://reviewboard.mozilla.org/r/51885/#review48733
Attachment #8751152 - Flags: review?(jwwang) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: