1235612 - Audio indicator/mute button shown for page with no audible sound

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Mike Taylor [:miketaylr]]

STR:

1) Visit https://reactforbeginners.com/

Expected: No audio/mute button in tab
Actual: audio/mute button in tab

There's a <video autoplay>:

<video poster="/images/poster.jpg" src="/images/using.mp4" loop autoplay width="640" class="screenshot"></video>

I'm not sure what's up with this particular video, but Chrome doesn't show their audio tab indicator.

Comment 1

[Alastor Wu [:alwu]]

When the video has audio track and its volume is not ZERO or muted, we would show the audio tab indicator.
The problem is this video has audio track but no any audio data in that track.

Comment 2

[Andrea Marchesini [:baku]]

> The problem is this video has audio track but no any audio data in that
> track.

https://hg.mozilla.org/try/file/b65a5b8ca666/dom/html/HTMLMediaElement.cpp#l4759

In theory we check the volume too.

Comment 3

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #2) 
> https://hg.mozilla.org/try/file/b65a5b8ca666/dom/html/HTMLMediaElement.
> cpp#l4759
> 
> In theory we check the volume too.

In this case, the video isn't muted and its volume is also not ZERO.

See [1], right click -> show control & show statistic, then you can see more details.

[1] https://reactforbeginners.com/images/using.mp4

Comment 4

[Alastor Wu [:alwu]]

After checking the audio data in the AudioStream, the audio track in that video is totally silence.
So we need to create a method to notify HTMLMediaElement if the audio track has no audible data within a long while.

Comment 6

[Alastor Wu [:alwu]]

Hi, Baku,

Here is my preliminary method for this issue, could you give me some suggestions?
Very appreciate!

---

[Main idea]
Don't dispatch "audio-playback" event in RegisterAudioChannelAgent(), firing event only when there are actual audible data in the audio track. Move this event from AudioChannelService to HTMLMediaElement.

[Implementation]
Not to dispatch "audio-playback" by-default when we register the AudioChannelAgent.
Check audio data in the MediaDecoderStateMachine, if there has any audible data within a specific period, we would notify the MediaElement, then fire this event from the MediaElement.

Comment 7

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 8

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 9

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part3 : send audio-playback event from ME.

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

Comment 10

[Alastor Wu [:alwu]]

The implementation of patch3 wasn't correct, I'll update new patch.

Comment 11

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 12

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 13

[Alastor Wu [:alwu]]

It's the first version patches, it can work but needs to check other test case fails.
Because I change some behaviors and I would describe them later.

Comment 14

[Alastor Wu [:alwu]]

[Main idea]
Split the audio-playback event from the agent registration, this event would be dispatch by the agent owner when it actually starts producing audible sound. 

---

[Main reason]
It's because the dispatch time of the events "audio-playback" and "audiochannel-activity" doesn't align in some cases. These two events have totally different usages.

### audio-playback event ###

Event "audio-playback" is used to indicate that the specific window is producing audible sound. The sound might be generated from MediaElement/AudioContext/WebSpeech e.t.c.

The "audio-playback" event should only be dispatched *when the window starts/stops producing audible sound*. In desktop browser, a sound icon would be displayed to notify users about that. We shouldn't display sound icon if the sound generator in that window is muted, ZERO volume, non-playing, without audio track or producing silent data.


### audiochannel-activity event ###

Event "audiochannel-activity" is used to inform the system app there is something start playing and it's considered involving in the audio competing, but it might be not audible when it starts. In b2g, the system app uses this event to decide which window should be playback.

The "audiochannel-activity" event should be dispatched *when the window has something starts playing and it might be producing any audible sound or stops playing*. 

In b2g, the audio competing is very important thing, because we used to focus on one audible sound in the mobile device. If there are multiple sound, the system app usually mute or reduce the volume for one of them. 


### Example for events-no-aligned ###

For example, in b2g we have two video, video A is starting playing now and then we want to play video B during A playing. Their audio channel types are both content type. Something special is that the video B contains lots a silent data, and 0 ~ 5 secs is totally silent.

In the UX sound spec, the A should be paused when B started and be resumed after B ended [1].

In this case, the event "audio-playback" and "audiochannel-activity" should be dispatched at totally different time.

STR.
1. B starts playing 
   - we should pause A, needs "audiochannel-activity"
2. After 5 secs, B starts producing sound 
   - needs "audio-playback-true"
3. When playing B's long silent data
   - shouldn't resume A, but need to inform users there is no any audible sound, "audio-playback-false"

[1] https://bug1068219.bmoattachments.org/attachment.cgi?id=8579177

Comment 15

[Alastor Wu [:alwu]]

Hi, Philipp,
Sorry to bother you, may I get your feedback here?
I want to know what is the precise timing that the sound icon of the tab should be displayed.

The issue here is that the browser shows the sound icon when playing a silent video, and it makes us confused.
IMMO, the sound icon should only be displayed when the web page is producing any *audible* sound.

How do you think about my proposal? Does it make sense?
Very appreciate :)

Comment 16

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alastor Wu [:alwu] from comment #15)
> Hi, Philipp,
> Sorry to bother you, may I get your feedback here?
> I want to know what is the precise timing that the sound icon of the tab
> should be displayed.
> 
> The issue here is that the browser shows the sound icon when playing a
> silent video, and it makes us confused.
> IMMO, the sound icon should only be displayed when the web page is producing
> any *audible* sound.
> 
> How do you think about my proposal? Does it make sense?
> Very appreciate :)

Sure! Your proposal makes a lot of sense!
To be clear that we are on the same page: The audio indicator should show up when the website is attempting to make any actual noise. The only edge case I can think of is if the website is playing audio, but it is not audible because the system volume is down. In that case, the indicator should obviously still show up.

Comment 17

[Alastor Wu [:alwu]]

Hi, Philipp,
Thanks for your reply :)

One more thing, I would list some following cases, could you also help me to verify they are correct or not? Also, do we have a spec which defines the appear/disapper case for the sound icon?

Very appreciate :)

---

First, the system volume shouldn't affect the icon displaying. (per comment16)

* Display the sound icon *
1. Playing audible sound from the web page 

* Don't display the sound icon*
1. When the audible object ended
2. When we pause the audible object 
3. When we adjust the audible object volume to ZERO 
4. When we mute the audible object 
5. When the object is playing totally silent (means it's not audible) [1]

[1] https://reactforbeginners.com/

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alastor Wu [:alwu] from comment #17)
> Hi, Philipp,
> Thanks for your reply :)
> 
> One more thing, I would list some following cases, could you also help me to
> verify they are correct or not? Also, do we have a spec which defines the
> appear/disapper case for the sound icon?
> 
> Very appreciate :)
> 
> ---
> 
> First, the system volume shouldn't affect the icon displaying. (per
> comment16)
> 
> * Display the sound icon *
> 1. Playing audible sound from the web page 
> 
> * Don't display the sound icon*
> 1. When the audible object ended
> 2. When we pause the audible object 
> 3. When we adjust the audible object volume to ZERO 
> 4. When we mute the audible object 
> 5. When the object is playing totally silent (means it's not audible) [1]
> 
> [1] https://reactforbeginners.com/

Yeah, that all makes sense. If I'm interpreting the rest of this thread correctly, there would be a tolerance for total silence, so that it won't flicker on and off when there are e.g. silent pauses in a dialog. I think 10 seconds would be a good starting point here.

Comment 19

[Alastor Wu [:alwu]]

Hi, Philipp,

You mean if user is watching an audible video, the sound icon should be displayed. But, if that video suddenly becomes silent around 10 seconds, then the sound icon should disappear until the video is audible again? If the silence is less than 10 seconds, we should keep the icon display all the time.

Do I understand correctly?
Thanks!

Comment 20

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to Alastor Wu [:alwu] from comment #19)
> Hi, Philipp,
> 
> You mean if user is watching an audible video, the sound icon should be
> displayed. But, if that video suddenly becomes silent around 10 seconds,
> then the sound icon should disappear until the video is audible again? If
> the silence is less than 10 seconds, we should keep the icon display all the
> time.
> 
> Do I understand correctly?
> Thanks!

I think you do :)

Here's a scenario:
- Video starts playing with audio
- Indicator shows up immediately
- Video has a silent segment (e.g. a long pause in a dialog)
- Indicator keeps showing
- Silent segment of the video lasts longer than 10 seconds
- Indicator disappears
- Video plays audible audio again
- Indicator shows up again immediately

Comment 21

[Alastor Wu [:alwu]]

Hi, baku, eshan,
Could you give me any feedback about comment14?
The main idea is that to dispatch the "audio-play" event when there is actually audible sound.
If both you feel well about this proposal, I'll continue to work on it after landing bug1238906.
Thanks!

Comment 22

[(no longer active)]

I don't know how the audiochannel-activity event stuff works, but your overall plan for only dispatching audio-playback when there is audible audio sounds good to me.  That's been definitely my goal all along.

Comment 24

[Alastor Wu [:alwu]]

The bug123890 implement the way that MediaElement can know whether the audio track is silent when the input data is came from decoder.

But I also need to implement another way when input data is came from the MediaStream in bug1240429, it's totally different thing.

Comment 25

[Loic]

Another example here: https://www.blackdesertonline.com/events/ccm/
This mp4 plays in the background with blank audio track:
https://akamai-webcdn.blackdesertonline.com/web/static/video/ccm/feature/face.mp4

Comment 26

[(no longer active)]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #20)
> Here's a scenario:
> - Video starts playing with audio
> - Indicator shows up immediately
> - Video has a silent segment (e.g. a long pause in a dialog)
> - Indicator keeps showing
> - Silent segment of the video lasts longer than 10 seconds
> - Indicator disappears
> - Video plays audible audio again
> - Indicator shows up again immediately

This sounds like a bad idea to me.  Right now in the general case (ignoring the edge case this bug is about for a sec) we have this nice property that when there is audible audio, there is a sound indicator, and when there is no audible audio (such as when the video is paused), there is no sound indicator.  It's really sad if we replace this with "we're going to show the indicator sometimes, but it's not directly correlated to audible sound any more."  :(

How about:

- Video starts playing with audio
- Indicator shows up immediately
- Video has a silent segment (e.g. a long pause in a dialog)
- Indicator keeps showing
- Silent segment of the video lasts longer than 10 seconds
- Indicator <strikethrough>disappears</strikethrough> stays.
- Video plays audible audio again
- Indicator <strikethrough>shows up again immediately</strikethrough> stays.

Basically in terms of the implementation: when you see a new audio stream, wait for it to generate _some_ audible sound, then show the indicator and keep showing it until the audio stream goes away.  We can detect the audio stream being paused and immediately remove the indicator as we do right now.

This will make us show the indicator when a tab starts to emit audio, and then keep it there.

(Note that we can always focus on a pathological case for example of a looping audio element that flips between a second of silence and a second of noise on and on, but designing *for* this kind of edge case seems weird.)

Philipp, what do you think?

Comment 27

[Alastor Wu [:alwu]]

Hi, Ehsan,

If I understand correctly, even we don't apply the suggestion from Philipp, now the sound indicator still can't appear precisely corresponding to the audible sound because we don't detect the silence.

In comment26, if the sound indicator stays when the long silent segment happens, that means it's still not directly correlated to audible sound any more, isn't it?

I'm little confused about the situation now. What's your most concern points? Is the 10-secs too long or the icon flickering issue (but I think the 10-secs can't be called as flickering). What I want to do is to let the sound indicator can reflect the real audible state as far as possible and also not to affect the user-experience.

Here is a case that the input stream doesn't produce any audible data after first 3 secs, and the sound indicator doesn't disappear. 
> http://people.mozilla.org/~alwu/silence_from_inputstream.html

BTW, in bug1238906 and bug1240429, we provide the ways to detect the silence from file and from media stream. It means we can implement the different audible strategy between there two cases if we want. ex. only detect file.

Very appreciate!

Comment 28

[Alastor Wu [:alwu]]

FYI, maybe we also need to think about the situation of bug1240358.

Comment 29

[(no longer active)]

(In reply to Alastor Wu [:alwu] from comment #27)
> Hi, Ehsan,
> 
> If I understand correctly, even we don't apply the suggestion from Philipp,
> now the sound indicator still can't appear precisely corresponding to the
> audible sound because we don't detect the silence.
> 
> In comment26, if the sound indicator stays when the long silent segment
> happens, that means it's still not directly correlated to audible sound any
> more, isn't it?

That's correct!

A better way to state what I think we should do is: We should start showing the audio indicator when the page starts to make audible noise, and then treat the existence of an audio stream as the sign that more audio is probably going to come (even if there is no audible sound at this very moment.)  The reason why this makes sense is that in the majority of cases, there is either never going to be anything audible, or there will be something audible once the first audible sound is emitted for as long as there is an audio stream.  The reason is that short lived audio (such as a beep sound) is typically done by using a small audio file (aka a short lived audio stream), not by a long running audio stream that has embedded silence inside it.

> I'm little confused about the situation now. What's your most concern
> points? Is the 10-secs too long or the icon flickering issue (but I think
> the 10-secs can't be called as flickering). What I want to do is to let the
> sound indicator can reflect the real audible state as far as possible and
> also not to affect the user-experience.

Remember that the icon has two purposes: showing whether there is audio playing, and letting users mute the audio by clicking on the icon.  If we implement something that will take the indicator away such as Philipp's step by step scenario shows, we're taking away the muting feature.

(There is a keyboard shortcut for muting too, but it's hardly discoverable, so I will ignore it for the purpose of this discussion.)

> Here is a case that the input stream doesn't produce any audible data after
> first 3 secs, and the sound indicator doesn't disappear. 
> > http://people.mozilla.org/~alwu/silence_from_inputstream.html

Web Audio is a totally different case than HTML5 <audio>/<video>.  Basically in web audio there is no useful notion of an audio stream, and the page can schedule audio playback to happen at random times.  So for audio generated through Web Audio, the suggestion in comment 20 makes sense, but that's only because there is nothing better that we can do.  For HTML5 <audio> and <video> though we have more information to work with.

> BTW, in bug1238906 and bug1240429, we provide the ways to detect the silence
> from file and from media stream. It means we can implement the different
> audible strategy between there two cases if we want. ex. only detect file.

That's great!  For audio coming from Web Audio (or sources such as the getUserMedia initiated streams) doing the silence detection in the middle of the stream makes sense.  But let's wait for Philipp's feedback before writing more code...

Comment 30

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Ehsan, that all sounds sensible (I didn't know about the different kinds of audio).
In short, the case I want to avoid is where a page like IRC Cloud plays a ping sound for half a second and then the audio indicator stays there forever (which is what sometimes happens today).

Comment 31

[(no longer active)]

Good!  Looks like we're on the same page.

Comment 32

[Alastor Wu [:alwu]]

Hi, Ehsan,
Let me do a short conclusion,
Are the following situations correct?

* For audio file
- show indicator if it's audible
- hide indicator after 10 seconds it became silent

* For media stream (input source stream for MediaElement) 
- always show indicator if the steam is alive
- hide indicator when the stream is removed

* For web audio
- show indicator if it's audible
- hide indicator after 10 seconds it became silent

Comment 33

[(no longer active)]

(In reply to Alastor Wu [:alwu] from comment #32)
> Hi, Ehsan,
> Let me do a short conclusion,
> Are the following situations correct?

Almost.  See below:

> * For audio file
> - show indicator if it's audible
> - hide indicator after 10 seconds it became silent

No, why wait 10 seconds.  We should stop displaying the icon immediately.

> * For media stream (input source stream for MediaElement) 
> - always show indicator if the steam is alive
> - hide indicator when the stream is removed

Or if 10 seconds pass without an audible stream.

> * For web audio
> - show indicator if it's audible
> - hide indicator after 10 seconds it became silent

Yes, that's correct.  Basically treat web audio and the media stream case the same way.

Comment 34

[Alastor Wu [:alwu]]

(In reply to :Ehsan Akhgari from comment #33)
> > * For audio file
> > - show indicator if it's audible
> > - hide indicator after 10 seconds it became silent
> 
> No, why wait 10 seconds.  We should stop displaying the icon immediately.

But if the file is intermittent audible, the indicator would flickering.
Is that ok for you?

Comment 35

[(no longer active)]

(In reply to Alastor Wu [:alwu] from comment #34)
> (In reply to :Ehsan Akhgari from comment #33)
> > > * For audio file
> > > - show indicator if it's audible
> > > - hide indicator after 10 seconds it became silent
> > 
> > No, why wait 10 seconds.  We should stop displaying the icon immediately.
> 
> But if the file is intermittent audible, the indicator would flickering.
> Is that ok for you?

No, please see comment 26 again.  For audio files, we should only look for audible audio to know when to show the icon.  Once we have shown the icon, we should only remove it when the audio finishes playback no matter whether the rest of the audio stream has been audible or not.  This was my main objection to the previous plan.  :-)

Comment 36

[Alastor Wu [:alwu]]

Attachment: part1 : Implement notify media-playback

Because my moz-review is broken for unknown reason, using the regular review.

Comment 37

[Alastor Wu [:alwu]]

Attachment: part 2 : seperate the media-playback from the registration process.

Comment 38

[Alastor Wu [:alwu]]

Attachment: part3 : modify tests

In this test, we shouldn't pause audio in onplaying(), since the audio-playback isn't dispatch immediately after play().

Comment 39

[Alastor Wu [:alwu]]

Attachment: part1 : Implement notify media-playback

Comment 40

[Alastor Wu [:alwu]]

Attachment: part1 : Implement notify media-playback

Comment 41

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91b7378c57ea

Comment 42

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 43

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 44

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 45

[Alastor Wu [:alwu]]

Comment on attachment 8719436 [details] [diff] [review]
part 2 : seperate the media-playback from the registration process.

># HG changeset patch
># Parent 0a9857f6bc6ee2cfb0aef0ba679e48186f257140
># User Alastor Wu <[e-mail]>
>Bug 1235612 - part2 : seperate the media-playback from the registration process.
>
>diff --git a/dom/audiochannel/AudioChannelService.cpp b/dom/audiochannel/AudioChannelService.cpp
>--- a/dom/audiochannel/AudioChannelService.cpp
>+++ b/dom/audiochannel/AudioChannelService.cpp
>@@ -269,23 +269,16 @@ AudioChannelService::RegisterAudioChanne
> 
>   ++winData->mChannels[(uint32_t)aChannel].mNumberOfAgents;
> 
>   // The first one, we must inform the BrowserElementAudioChannel.
>   if (winData->mChannels[(uint32_t)aChannel].mNumberOfAgents == 1) {
>     NotifyChannelActive(aAgent->WindowID(), aChannel, true);
>   }
> 
>-  // If this is the first agent for this window, we must notify the observers.
>-  if (winData->mAgents.Length() == 1) {
>-    RefPtr<MediaPlaybackRunnable> runnable =
>-      new MediaPlaybackRunnable(aAgent->Window(), true /* active */);
>-    NS_DispatchToCurrentThread(runnable);
>-  }
>-
>   // If the window has already been captured, the agent of that window should
>   // also be captured.
>   if (winData->mIsAudioCaptured) {
>     aAgent->WindowAudioCaptureChanged(aAgent->InnerWindowID(),
>                                       winData->mIsAudioCaptured);
>   }
> 
>   MaybeSendStatusUpdate();
>@@ -318,18 +311,18 @@ AudioChannelService::UnregisterAudioChan
> 
> #ifdef MOZ_WIDGET_GONK
>   bool active = AnyAudioChannelIsActive();
>   for (uint32_t i = 0; i < mSpeakerManager.Length(); i++) {
>     mSpeakerManager[i]->SetAudioChannelActive(active);
>   }
> #endif
> 
>-  // If this is the last agent for this window, we must notify the observers.
>-  if (winData->mAgents.IsEmpty()) {
>+  // Notify the observers if this is the last audible agent of this window.
>+  if (winData->IsMediaPlaybackChanged(aAgent, false /* audible */)) {
>     RefPtr<MediaPlaybackRunnable> runnable =
>       new MediaPlaybackRunnable(aAgent->Window(), false /* active */);
>     NS_DispatchToCurrentThread(runnable);
>   }
> 
>   // No need to capture non-audible object.
>   if (winData->mIsAudioCaptured) {
>     aAgent->WindowAudioCaptureChanged(aAgent->InnerWindowID(), false);
>diff --git a/dom/html/HTMLMediaElement.cpp b/dom/html/HTMLMediaElement.cpp
>--- a/dom/html/HTMLMediaElement.cpp
>+++ b/dom/html/HTMLMediaElement.cpp
>@@ -124,16 +124,34 @@ public:
>   }
> 
>   ~AutoNotifyAudioChannelAgent()
>   {
>     mElement->UpdateAudioChannelPlayingState();
>   }
> };
> 
>+class MOZ_STACK_CLASS AutoNotifyMediaPlayback
>+{
>+  RefPtr<mozilla::dom::HTMLMediaElement> mElement;
>+  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER;
>+public:
>+  explicit AutoNotifyMediaPlayback(mozilla::dom::HTMLMediaElement* aElement
>+                                       MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>+    : mElement(aElement)
>+  {
>+    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>+  }
>+
>+  ~AutoNotifyMediaPlayback()
>+  {
>+    mElement->UpdateMediaPlaybackChanged();
>+  }
>+};
>+
> namespace mozilla {
> namespace dom {
> 
> // Number of milliseconds between progress events as defined by spec
> static const uint32_t PROGRESS_MS = 350;
> 
> // Number of milliseconds of no data before a stall event is fired as defined by spec
> static const uint32_t STALL_MS = 3000;
>@@ -3198,16 +3216,20 @@ void HTMLMediaElement::UpdateSrcMediaStr
>     // called and that will remove all listeners/outputs.
> 
>     mWatchManager.Unwatch(*mMediaStreamListener,
>         &HTMLMediaElement::UpdateReadyStateInternal);
> 
>     mMediaStreamListener->Forget();
>     mMediaStreamListener = nullptr;
>   }
>+
>+  // If the input is a media stream, we don't check its data and always regard
>+  // it as audible when it's playing.
>+  NotifyAudibleStateChanged(shouldPlay);
> }
> 
> void HTMLMediaElement::SetupSrcMediaStreamPlayback(DOMMediaStream* aStream)
> {
>   NS_ASSERTION(!mSrcStream && !mMediaStreamListener && !mMediaStreamSizeListener,
>                "Should have been ended already");
> 
>   mSrcStream = aStream;
>@@ -4819,16 +4841,20 @@ HTMLMediaElement::UpdateAudioChannelPlay
> void
> HTMLMediaElement::NotifyAudioChannelAgent(bool aPlaying)
> {
>   // This is needed to pass nsContentUtils::IsCallerChrome().
>   // AudioChannel API should not called from content but it can happen that
>   // this method has some content JS in its stack.
>   AutoNoJSAPI nojsapi;
> 
>+  // We need to notify media-playback event if audible state changing was
>+  // notified before start playing.
>+  AutoNotifyMediaPlayback autoNotify(this);
>+
>   if (aPlaying) {
>     float volume = 0.0;
>     bool muted = true;
>     mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
>     WindowVolumeChanged(volume, muted);
>   } else {
>     mAudioChannelAgent->NotifyStoppedPlaying();
>     mAudioChannelAgent = nullptr;
>@@ -5207,14 +5233,23 @@ HTMLMediaElement::IsCurrentlyPlaying() c
>   return false;
> }
> 
> void
> HTMLMediaElement::NotifyAudibleStateChanged(bool aAudible)
> {
>   if (mIsAudioTrackAudible != aAudible) {
>     mIsAudioTrackAudible = aAudible;
>-    // To do ...
>+    UpdateMediaPlaybackChanged();
>+  }
>+}
>+
>+void
>+HTMLMediaElement::UpdateMediaPlaybackChanged()
>+{
>+  // Always notify agent after start playing.
>+  if (mAudioChannelAgent) {
>+    mAudioChannelAgent->NotifyStartedAudible(mIsAudioTrackAudible);
>   }
> }
> 
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/html/HTMLMediaElement.h b/dom/html/HTMLMediaElement.h
>--- a/dom/html/HTMLMediaElement.h
>+++ b/dom/html/HTMLMediaElement.h
>@@ -398,16 +398,19 @@ public:
> 
>   // Called by the media decoder object, on the main thread,
>   // when the connection between Rtsp server and client gets lost.
>   virtual void ResetConnectionState() final override;
> 
>   // Called by media decoder when the audible state changed.
>   virtual void NotifyAudibleStateChanged(bool aAudible) final override;
> 
>+  // Notify agent when the MediaElement changes its audible state.
>+  void UpdateMediaPlaybackChanged();
>+
>   // XPCOM GetPreload() is OK
>   void SetPreload(const nsAString& aValue, ErrorResult& aRv)
>   {
>     SetHTMLAttr(nsGkAtoms::preload, aValue, aRv);
>   }
> 
>   already_AddRefed<TimeRanges> Buffered() const;
> 
>diff --git a/dom/media/webaudio/AudioContext.cpp b/dom/media/webaudio/AudioContext.cpp
>--- a/dom/media/webaudio/AudioContext.cpp
>+++ b/dom/media/webaudio/AudioContext.cpp
>@@ -122,20 +122,16 @@ AudioContext::AudioContext(nsPIDOMWindow
> 
> nsresult
> AudioContext::Init()
> {
>   // We skip calling SetIsOnlyNodeForContext and the creation of the
>   // audioChannelAgent during mDestination's constructor, because we can only
>   // call them after mDestination has been set up.
>   if (!mIsOffline) {
>-    nsresult rv = mDestination->CreateAudioChannelAgent();
>-    if (NS_WARN_IF(NS_FAILED(rv))) {
>-      return rv;
>-    }
>     mDestination->SetIsOnlyNodeForContext(true);
>   }
> 
>   return NS_OK;
> }
> 
> AudioContext::~AudioContext()
> {
>@@ -949,17 +945,17 @@ AudioContext::Close(ErrorResult& aRv)
>   }
> 
>   if (mAudioContextState == AudioContextState::Closed) {
>     promise->MaybeResolve(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return promise.forget();
>   }
> 
>   if (Destination()) {
>-    Destination()->DestroyAudioChannelAgent();
>+    Destination()->DestroyAndStopAudioChannelAgent();
>   }
> 
>   mPromiseGripArray.AppendElement(promise);
> 
>   // This can be called when freeing a document, and the streams are dead at
>   // this point, so we need extra null-checks.
>   MediaStream* ds = DestinationStream();
>   if (ds) {
>diff --git a/dom/media/webaudio/AudioDestinationNode.cpp b/dom/media/webaudio/AudioDestinationNode.cpp
>--- a/dom/media/webaudio/AudioDestinationNode.cpp
>+++ b/dom/media/webaudio/AudioDestinationNode.cpp
>@@ -382,28 +382,25 @@ AudioDestinationNode::SizeOfExcludingThi
> 
> size_t
> AudioDestinationNode::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
> {
>   return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
> }
> 
> void
>-AudioDestinationNode::DestroyAudioChannelAgent()
>+AudioDestinationNode::DestroyAndStopAudioChannelAgent()
> {
>-  if (mAudioChannelAgent && !Context()->IsOffline()) {
>-    mAudioChannelAgent->NotifyStoppedPlaying();
>-    mAudioChannelAgent = nullptr;
>-  }
>+  NotifyAudioChannelAgent(false);
> }
> 
> void
> AudioDestinationNode::DestroyMediaStream()
> {
>-  DestroyAudioChannelAgent();
>+  DestroyAndStopAudioChannelAgent();
> 
>   if (!mStream)
>     return;
> 
>   mStream->RemoveMainThreadListener(this);
>   MediaStreamGraph* graph = mStream->Graph();
>   if (graph->IsNonRealtime()) {
>     MediaStreamGraph::DestroyNonRealtimeInstance(graph);
>@@ -468,24 +465,24 @@ AudioDestinationNode::Unmute()
> {
>   MOZ_ASSERT(Context() && !Context()->IsOffline());
>   SendDoubleParameterToStream(DestinationNodeEngine::VOLUME, 1.0f);
> }
> 
> void
> AudioDestinationNode::Suspend()
> {
>-  DestroyAudioChannelAgent();
>+  DestroyAndStopAudioChannelAgent();
>   SendInt32ParameterToStream(DestinationNodeEngine::SUSPENDED, 1);
> }
> 
> void
> AudioDestinationNode::Resume()
> {
>-  CreateAudioChannelAgent();
>+  CreateAndStartAudioChannelAgent();
>   SendInt32ParameterToStream(DestinationNodeEngine::SUSPENDED, 0);
> }
> 
> void
> AudioDestinationNode::OfflineShutdown()
> {
>   MOZ_ASSERT(Context() && Context()->IsOffline(),
>              "Should only be called on a valid OfflineAudioContext");
>@@ -583,17 +580,18 @@ AudioDestinationNode::SetMozAudioChannel
>       CheckAudioChannelPermissions(aValue)) {
>     mAudioChannel = aValue;
> 
>     if (mStream) {
>       mStream->SetAudioChannelType(mAudioChannel);
>     }
> 
>     if (mAudioChannelAgent) {
>-      CreateAudioChannelAgent();
>+      NotifyAudioChannelAgent(false);
>+      CreateAndStartAudioChannelAgent();
>     }
>   }
> }
> 
> bool
> AudioDestinationNode::CheckAudioChannelPermissions(AudioChannel aValue)
> {
>   // Only normal channel doesn't need permission.
>@@ -624,38 +622,32 @@ AudioDestinationNode::CheckAudioChannelP
>   permissionManager->TestExactPermissionFromPrincipal(principal,
>     nsCString(NS_LITERAL_CSTRING("audio-channel-") + channel).get(),
>     &perm);
> 
>   return perm == nsIPermissionManager::ALLOW_ACTION;
> }
> 
> nsresult
>-AudioDestinationNode::CreateAudioChannelAgent()
>+AudioDestinationNode::CreateAndStartAudioChannelAgent()
> {
>   if (mIsOffline) {
>     return NS_OK;
>   }
> 
>-  nsresult rv = NS_OK;
>-  if (mAudioChannelAgent) {
>-    rv = mAudioChannelAgent->NotifyStoppedPlaying();
>-    if (NS_WARN_IF(NS_FAILED(rv))) {
>-      return rv;
>-    }
>-  }
>-
>   mAudioChannelAgent = new AudioChannelAgent();
>-  rv = mAudioChannelAgent->InitWithWeakCallback(GetOwner(),
>+  nsresult rv = mAudioChannelAgent->InitWithWeakCallback(GetOwner(),
>                                            static_cast<int32_t>(mAudioChannel),
>                                            this);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>+  NotifyAudioChannelAgent(true);
>+
>   return NS_OK;
> }
> 
> void
> AudioDestinationNode::NotifyStableState()
> {
>   mExtraCurrentTimeUpdatedSinceLastStableState = false;
> }
>@@ -726,28 +718,34 @@ void
> AudioDestinationNode::InputMuted(bool aMuted)
> {
>   MOZ_ASSERT(Context() && !Context()->IsOffline());
> 
>   if (!mAudioChannelAgent) {
>     if (aMuted) {
>       return;
>     }
>-    CreateAudioChannelAgent();
>+    CreateAndStartAudioChannelAgent();
>   }
> 
>-  if (aMuted) {
>-    mAudioChannelAgent->NotifyStoppedPlaying();
>+  mAudioChannelAgent->NotifyStartedAudible(!aMuted);
>+}
>+
>+void
>+AudioDestinationNode::NotifyAudioChannelAgent(bool aPlaying)
>+{
>+  if (mIsOffline || !mAudioChannelAgent) {
>     return;
>   }
> 
>-  float volume = 0.0;
>-  bool muted = true;
>-  nsresult rv = mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
>-  if (NS_WARN_IF(NS_FAILED(rv))) {
>-    return;
>+  if (aPlaying) {
>+    float volume = 0.0;
>+    bool muted = true;
>+    mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
>+    WindowVolumeChanged(volume, muted);
>+  } else {
>+    mAudioChannelAgent->NotifyStoppedPlaying();
>+    mAudioChannelAgent = nullptr;
>   }
>-
>-  WindowVolumeChanged(volume, muted);
> }
> 
> } // namespace dom
> } // namespace mozilla
>diff --git a/dom/media/webaudio/AudioDestinationNode.h b/dom/media/webaudio/AudioDestinationNode.h
>--- a/dom/media/webaudio/AudioDestinationNode.h
>+++ b/dom/media/webaudio/AudioDestinationNode.h
>@@ -67,34 +67,35 @@ public:
> 
>   // An amount that should be added to the MediaStream's current time to
>   // get the AudioContext.currentTime.
>   StreamTime ExtraCurrentTime();
> 
>   // When aIsOnlyNode is true, this is the only node for the AudioContext.
>   void SetIsOnlyNodeForContext(bool aIsOnlyNode);
> 
>-  nsresult CreateAudioChannelAgent();
>-  void DestroyAudioChannelAgent();
>+  nsresult CreateAndStartAudioChannelAgent();
>+  void DestroyAndStopAudioChannelAgent();
> 
>   const char* NodeType() const override
>   {
>     return "AudioDestinationNode";
>   }
> 
>   size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const override;
>   size_t SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const override;
> 
>   void InputMuted(bool aInputMuted);
>   void ResolvePromise(AudioBuffer* aRenderedBuffer);
> 
> protected:
>   virtual ~AudioDestinationNode();
> 
> private:
>+  void NotifyAudioChannelAgent(bool aPlaying);
>   void SetMozAudioChannelType(AudioChannel aValue, ErrorResult& aRv);
>   bool CheckAudioChannelPermissions(AudioChannel aValue);
> 
>   void SetCanPlay(float aVolume, bool aMuted);
> 
>   void NotifyStableState();
>   void ScheduleStableStateNotification();
> 
>diff --git a/dom/plugins/base/nsNPAPIPlugin.cpp b/dom/plugins/base/nsNPAPIPlugin.cpp
>--- a/dom/plugins/base/nsNPAPIPlugin.cpp
>+++ b/dom/plugins/base/nsNPAPIPlugin.cpp
>@@ -2299,16 +2299,18 @@ NPError
>       } else {
>         float volume = 0.0;
>         bool muted = true;
>         rv = agent->NotifyStartedPlaying(&volume, &muted);
>         if (NS_WARN_IF(NS_FAILED(rv))) {
>           return NPERR_NO_ERROR;
>         }
> 
>+        agent->NotifyStartedAudible(true);
>+
>         rv = inst->WindowVolumeChanged(volume, muted);
>         if (NS_WARN_IF(NS_FAILED(rv))) {
>           return NPERR_NO_ERROR;
>         }
>       }
> 
>       return NPERR_NO_ERROR;
>     }

Comment 46

[Alastor Wu [:alwu]]

Attachment: only for obsolete

Comment 47

[Alastor Wu [:alwu]]

Hi, Baku,
Could you help me review these patches?
Very appreciate!!

---

The main idea is to dispatch the "audio-playback" event only when the audio agent owner is actually producing any audible sound. That can help to avoid to show the tab indicator for non-audible video.

However, we have different strategies between (1) play audio from file and (2) play audio from media stream.

(1) play audio from file 
* show tab indicator when it starts *audible*
* hide tab indicator when it ended / volume ZERO / muted / paused / not audible

(2) play audio from media stream
* show tab indicator when it starts *playing*
* hide tab indicator when it ended / volume ZERO / muted / paused / remove stream

Comment 48

[Andrea Marchesini [:baku]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

https://reviewboard.mozilla.org/r/30091/#review35057

Sorry sorry sorry for the delay. Can I see the patch again?
In particular the unregister part is very important because we must be 100% sure we dispatch the runnable if an active agent is removed.
Or add assertions to prevent that an agent is unregister only when inactive.

::: dom/audiochannel/AudioChannelAgent.cpp:263
(Diff revision 3)
> +    service->MaybeDispatchMediaPlayback(this, aAudible);

Call this: MediaPlaybackChanged.

::: dom/audiochannel/AudioChannelService.h:84
(Diff revision 3)
> +  void MaybeDispatchMediaPlayback(AudioChannelAgent* aAgent, bool aAudible);

MediaPlaybackChanged

::: dom/audiochannel/AudioChannelService.h:203
(Diff revision 3)
> +    bool IsMediaPlaybackChanged(AudioChannelAgent* aAgent, bool aAudible)

This is a bad name. What about: MediaPlaybackChanged.

::: dom/audiochannel/AudioChannelService.h:204
(Diff revision 3)
> +    {

MOZ_ASSERT(aAgent);

::: dom/audiochannel/AudioChannelService.h:205
(Diff revision 3)
> +      if (aAudible) {

MOZ_ASSERT(mAgents.Contain(aAgent));
correct?

Then, instead returning true/false, Dispatch the runnable from here directly.

Plus, move this method in the AudioChannelService.cpp file.

::: dom/audiochannel/AudioChannelService.h:229
(Diff revision 3)
> +    nsTObserverArray<AudioChannelAgent*> mAudibleAgents;

In unregisterAgent() remove the agent also from this array. And you must check if runnable has to be dispatched or not.

::: dom/audiochannel/AudioChannelService.cpp:403
(Diff revision 3)
> +{

MOZ_ASSERT(aAgent);

::: dom/audiochannel/AudioChannelService.cpp:412
(Diff revision 3)
> +  if (winData->IsMediaPlaybackChanged(aAgent, aAudible)) {

Just do:

winData->MediaPlaybackChanged(aAgent, aAudible);

and move all the logic in AudioChannelWindow.

Comment 49

[Andrea Marchesini [:baku]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

https://reviewboard.mozilla.org/r/35047/#review35065

Comment 50

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/35047/#review35067

Comment 51

[Andrea Marchesini [:baku]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

https://reviewboard.mozilla.org/r/30093/#review35061

It looks almost perfect. But I have a few questions:

1. it's not about 'audible'. because maybe we are playing with volume 0, or the audiofile is silence.
2. we need tests
3. Instead calling MediaPlaybackChanged() immediately after NotifyStartedPlaying, can NotifyStartPlaying have an additional param that is 'audible'?

::: dom/audiochannel/AudioChannelService.cpp:322
(Diff revision 3)
> -  if (winData->mAgents.IsEmpty()) {
> +  if (winData->IsMediaPlaybackChanged(aAgent, false /* audible */)) {

I wanted to see this in the previous patch.
Can you merge the 2 patches?

::: dom/html/HTMLMediaElement.cpp:132
(Diff revision 3)
> +class MOZ_STACK_CLASS AutoNotifyMediaPlayback

final

::: dom/html/HTMLMediaElement.cpp:4851
(Diff revision 3)
> +  AutoNotifyMediaPlayback autoNotify(this);

Why do you need this? We care just when we call NotifyStartedPlaying() correct?

::: dom/html/HTMLMediaElement.cpp:5249
(Diff revision 3)
> +  if (mAudioChannelAgent) {

If you remove the RAII class, you can assert MOZ_ASSERT(mAudioChannelAgent).

::: dom/media/webaudio/AudioDestinationNode.cpp:638
(Diff revision 3)
>                                             static_cast<int32_t>(mAudioChannel),

indentation

::: dom/media/webaudio/AudioDestinationNode.cpp:735
(Diff revision 3)
> -  if (NS_WARN_IF(NS_FAILED(rv))) {
> +  if (mIsOffline || !mAudioChannelAgent) {

You should still stop and release the Agent if mIsOffline is true, right?

Comment 52

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #51)
> Comment on attachment 8705609 [details]
> MozReview Request: Bug 1235612 - part2 : seperate the media-playback from
> the registration process.
> 
> https://reviewboard.mozilla.org/r/30093/#review35061
> 
> It looks almost perfect. But I have a few questions:
> 
> 1. it's not about 'audible'. because maybe we are playing with volume 0, or
> the audiofile is silence.

Why not 'audible'? The volume-0 and silent audio file are also non-audible, right?

> 2. we need tests

OK, I'll add it.

> 3. Instead calling MediaPlaybackChanged() immediately after
> NotifyStartedPlaying, can NotifyStartPlaying have an additional param that
> is 'audible'?

MediaPlaybackChanged() is not always called immediately after NotifyStartedPlaying(), it depends on whether there is audible data. But it's fine to add an additional param, I'll add it. 

BTW, in the coding design, which one is better in this case? To add an additional param or an additional function?

> ::: dom/html/HTMLMediaElement.cpp:4851
> (Diff revision 3)
> > +  AutoNotifyMediaPlayback autoNotify(this);
> 
> Why do you need this? We care just when we call NotifyStartedPlaying()
> correct?

I just want to make sure the UpdateMediaPlaybackChanged() can be called after NotifyStartedPlaying(), or you think I should call UpdateMediaPlaybackChanged directly instead of using RAII?

> ::: dom/media/webaudio/AudioDestinationNode.cpp:735
> (Diff revision 3)
> > -  if (NS_WARN_IF(NS_FAILED(rv))) {
> > +  if (mIsOffline || !mAudioChannelAgent) {
> 
> You should still stop and release the Agent if mIsOffline is true, right?

The agent won't be created if |mIsOffline| is true.
https://dxr.mozilla.org/mozilla-central/source/dom/media/webaudio/AudioDestinationNode.cpp#634

Comment 53

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 54

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 55

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 56

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 57

[Alastor Wu [:alwu]]

Hi, JW,
Could you help me review this patch?
The bug1232357 would delay hiding the sound indicator, so we don’t need to delay the audible notification, and I also modify the incorrect if-condition.
Very appreciate your help!

Comment 58

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

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

https://reviewboard.mozilla.org/r/39163/#review36115

Comment 59

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

This patch has some incorrect behaviors, cancel review.
I'll update new patch on Monday.

Comment 60

[Alastor Wu [:alwu]]

I'm waiting for the try server result, and would ask for review again if the lights are green.

Comment 61

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 62

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 63

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 64

[Alastor Wu [:alwu]]

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 65

[Alastor Wu [:alwu]]

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

Comment 66

[u480271]

https://reviewboard.mozilla.org/r/30091/#review41657

::: dom/audiochannel/AudioChannelAgent.cpp:204
(Diff revision 5)
>            mWindow.get(), (!!mCallback || !!mWeakCallback)));
>  
>    return NS_OK;
>  }
>  
> -NS_IMETHODIMP AudioChannelAgent::NotifyStartedPlaying(float *aVolume,
> +NS_IMETHODIMP AudioChannelAgent::NotifyStartedPlaying(bool aAudible,

By adding aAudible, I have the feeling that you're overloading NotifyStartedPlaying into a registration phase (aAudible == false) and "this agent is really, playing audio. for real." (aAudible == true).

Think adds two responsibilities to NotifyStartedPlaying - registration of agents with service and notifying the service that an agent is making noise.

I'd suggest that these responsibilities are split into Register/Unregister steps and Start/Stop playing.

:cpearce started doing that in bug 1187778 and I collapsed it into register/start/stop. On reflection, I think it's best to revive :cpearce's patches and build on those instead of adding aAudible here.

Comment 67

[Alastor Wu [:alwu]]

(In reply to Dan Glastonbury :kamidphish from comment #66)
> 
> By adding aAudible, I have the feeling that you're overloading
> NotifyStartedPlaying into a registration phase (aAudible == false) and "this
> agent is really, playing audio. for real." (aAudible == true).
> 
> Think adds two responsibilities to NotifyStartedPlaying - registration of
> agents with service and notifying the service that an agent is making noise.
> 
> I'd suggest that these responsibilities are split into Register/Unregister
> steps and Start/Stop playing.
> 
> :cpearce started doing that in bug 1187778 and I collapsed it into
> register/start/stop. On reflection, I think it's best to revive :cpearce's
> patches and build on those instead of adding aAudible here.

Hi, Dan,
I did that in previous patches, but baku thinks to add the extra parameter to indicate audible state seems better. (see comment51)

Why you think split them up is better, except feeling overloading? Is there any issue let us have to do so?
In addition, could you help me point out what patches :cpearce already done that? I can't find the related codes.

Thanks!

Comment 68

[u480271]

(In reply to Alastor Wu [:alwu] from comment #67)

I realize that I should have made this a comment instead of an issue. Sorry. I'm still new to MozReview. 
(I forgot to push publish and my comment sat idle for 3 days.)

> Hi, Dan,
> I did that in previous patches, but baku thinks to add the extra parameter
> to indicate audible state seems better. (see comment51)

Since Andrea is the owner, I defer to his design decisions. I read comment 51 and what Andrea suggested makes sense to me at the micro-level, but...

> Why you think split them up is better, except feeling overloading? 

...at the macro-level I think it is about separation of concerns.

The current implementation overloads registration of AudioChannelAgents with AudioChannelService, and notification of start/stop making audio in two functions: NotifyStartedPlaying & NotifyStoppedPlaying.

If I understand correctly, this patch adds a bool flag which means "Oh, I'm really starting to play audio" or "No, I'm not playing audio, I just want to register with AudioChannelService and I'll update later"

While working on Bug 1187778, Chris Pearce split the registration from NotifyStartedPlaying/NotifyStoppedPlaying. This was so notification of visibility could be sent to all agents registered with the service for a given window. I inherited the bug from Chris, but I understand that Andrea and Ehsan suggested to use AudioChannelService as a notification system, so WebAudio, etc could also make use of the work being done for HTMLMediaElement.

Note that when I was implementing Bug 1262053 on top of Bug 1242874 I also ran into the issue of NotifyStoppedPlaying being called and unregistering the agent and then never receiving message the "playback is unblocked" notification.

I think there are parallels with this bug.

I can only speak for the HTMLMediaElement case, but I believe that NotifyStartPlaying() should only be called on the agent when element really does make audio, but the element still wants to have a registered agent so the element can receive notification of changes to suspend state, etc.

To me, this is a cleaner design but it's up to Andrea.
 
> Is there any issue let us have to do so?

There's no issue that makes it necessary, no. Besides, the issues I highlighted, it's mostly a logical separation.

> In addition, could you help me point out what patches :cpearce already done
> that? I can't find the related codes.

Bug 1187778, Patch 4.

Comment 69

[Andrea Marchesini [:baku]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

https://reviewboard.mozilla.org/r/30091/#review42317

::: dom/audiochannel/AudioChannelService.h:248
(Diff revision 5)
> +  bool mMuted;
> +
> +  uint32_t mNumberOfAgents;
> +};
> +
> +struct AudioChannelWindow final

make this a class.

::: dom/audiochannel/AudioChannelService.cpp:1087
(Diff revision 5)
> +}
> +
> +bool
> +AudioChannelWindow::IsLastAudibleAgent()
> +{
> +  return (mAudibleAgents.Length() == 0);

IsEmpty()

Comment 70

[Andrea Marchesini [:baku]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

https://reviewboard.mozilla.org/r/30093/#review42323

::: dom/fmradio/FMRadio.cpp:457
(Diff revision 5)
>  {
>    NS_ENSURE_TRUE_VOID(mAudioChannelAgent);
>  
>    float volume = 0.0;
>    bool muted = true;
> -  mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
> +  mAudioChannelAgent->NotifyStartedPlaying(true, &volume, &muted);

true /* aAudible */

::: dom/html/HTMLMediaElement.cpp:5246
(Diff revision 5)
> +
> +void
> +HTMLMediaElement::UpdateMediaPlaybackChanged()
> +{
> +  if (mAudioChannelAgent) {
> +    mAudioChannelAgent->NotifyStartedAudible(mIsAudioTrackAudible);

wher ethis mIsAudioTrackAudible come from?

::: dom/media/webaudio/AudioDestinationNode.cpp:576
(Diff revision 5)
>      if (mStream) {
>        mStream->SetAudioChannelType(mAudioChannel);
>      }
>  
>      if (mAudioChannelAgent) {
> -      CreateAudioChannelAgent();
> +      AudioPlayingChanged(false);

add a comment about this 'false' value

::: dom/media/webaudio/AudioDestinationNode.cpp:703
(Diff revision 5)
> +}
> +
> +void
> +AudioDestinationNode::DestroyAudioChannelAgentIfNeeded()
> +{
> +  AudioPlayingChanged(false);

can we remove this false/true and switch to a simple enum PlayingState { ePlaying, eNotPlaying }

It will make the code easier to read.

::: dom/media/webaudio/AudioDestinationNode.cpp:729
(Diff revision 5)
>    }
>  
> +  if (aPlaying) {
> -  float volume = 0.0;
> +    float volume = 0.0;
> -  bool muted = true;
> +    bool muted = true;
> -  nsresult rv = mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
> +    mAudioChannelAgent->NotifyStartedPlaying(true, &volume, &muted);

Add a comment for this true/false.

::: dom/media/webspeech/synth/nsSpeechTask.cpp:716
(Diff revision 5)
>    mAudioChannelAgent->InitWithWeakCallback(mUtterance->GetOwner(),
>                                             static_cast<int32_t>(AudioChannelService::GetDefaultAudioChannel()),
>                                             this);
>    float volume = 0.0f;
>    bool muted = true;
> -  mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted);
> +  mAudioChannelAgent->NotifyStartedPlaying(true, &volume, &muted);

same here.

::: dom/plugins/base/nsNPAPIPlugin.cpp:2299
(Diff revision 5)
>            return NPERR_NO_ERROR;
>          }
>        } else {
>          float volume = 0.0;
>          bool muted = true;
> -        rv = agent->NotifyStartedPlaying(&volume, &muted);
> +        rv = agent->NotifyStartedPlaying(true, &volume, &muted);

and here.

::: dom/telephony/Telephony.cpp:566
(Diff revision 5)
>      }
>    } else if (!activeCall.IsNull() && !mIsAudioStartPlaying) {
>      mIsAudioStartPlaying = true;
>      float volume;
>      bool muted;
> -    rv = mAudioAgent->NotifyStartedPlaying(&volume, &muted);
> +    rv = mAudioAgent->NotifyStartedPlaying(true, &volume, &muted);

and here.

Comment 71

[Andrea Marchesini [:baku]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

https://reviewboard.mozilla.org/r/35047/#review42325

Comment 72

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/35047/#review42327

Comment 73

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/35047/#review42329

Comment 74

[Alastor Wu [:alwu]]

Hi, Dan,
After the offline discussion with baku, I would split them up :)

Comment 75

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

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

Comment 76

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

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

Comment 77

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 78

[Alastor Wu [:alwu]]

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 79

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #70)
> ::: dom/html/HTMLMediaElement.cpp:5246
> (Diff revision 5)
> > +
> > +void
> > +HTMLMediaElement::UpdateMediaPlaybackChanged()
> > +{
> > +  if (mAudioChannelAgent) {
> > +    mAudioChannelAgent->NotifyStartedAudible(mIsAudioTrackAudible);
> 
> where this mIsAudioTrackAudible come from?
> 

https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.h#1584

Comment 80

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30091/diff/6-7/

Comment 81

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30093/diff/6-7/

Comment 82

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 83

[Alastor Wu [:alwu]]

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 84

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7840e73bb0df&selectedJob=20008701

Comment 85

[Andrea Marchesini [:baku]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

https://reviewboard.mozilla.org/r/30091/#review45995

::: dom/audiochannel/AudioChannelAgent.cpp:250
(Diff revision 7)
>  
>    mIsRegToService = false;
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP AudioChannelAgent::NotifyStartedAudible(bool aAudible)

NS_IMETHODIMP\n
AudioChannelAgent::...

::: dom/audiochannel/AudioChannelAgent.cpp:257
(Diff revision 7)
> +  MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug,
> +         ("AudioChannelAgent, NotifyStartedAudible, this = %p, "
> +          "audible = %d\n", this, aAudible));
> +
> +  RefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate();
> +  if (service) {

what about if we do:

if (NS_WARN_IF(!service)) {
  return NS_ERROR_FAILURE;
}

...

::: dom/audiochannel/AudioChannelService.h:36
(Diff revision 7)
>  
>  class TabParent;
>  
>  #define NUMBER_OF_AUDIO_CHANNELS (uint32_t)AudioChannel::EndGuard_
>  
> +enum AudibleState : bool {

move this in some class. Maybe AudioPlayingConfig?

::: dom/audiochannel/AudioChannelService.h:117
(Diff revision 7)
> +  /**
> +   * Called this method when the audible state of the audio playback changed,
> +   * it would dispatch the playback event to observers which want to know the
> +   * actual audible state of the window.
> +   */
> +  void AudioPlaybackChanged(AudioChannelAgent* aAgent, bool aAudible);

AudibleState instead bool

::: dom/audiochannel/AudioChannelService.h:240
(Diff revision 7)
>      {
>        // Workaround for bug1183033, system channel type can always playback.
>        mChannels[(int16_t)AudioChannel::System].mMuted = false;
>      }
>  
> +    void AudioPlaybackChanged(AudioChannelAgent* aAgent, bool aAudible);

AudibleState instead bool

::: dom/audiochannel/AudioChannelService.h:259
(Diff revision 7)
> +    enum AudioCaptureState : bool {
> +      eCapturing = true,
> +      eNotCapturing = false
> +    };
> +
> +    void AudioCapturedChanged(AudioChannelAgent* aAgent, bool isCapture);

AudioCaptureState instead isCapture

::: dom/audiochannel/AudioChannelService.h:267
(Diff revision 7)
> +    void RemoveAudibleAgentIfContained(AudioChannelAgent* aAgent);
> +
> +    void AppendAgentAndIncreaseAgentsNum(AudioChannelAgent* aAgent);
> +    void RemoveAgentAndReduceAgentsNum(AudioChannelAgent* aAgent);
> +
> +    bool IsFirstAudibleAgent();

const ?

::: dom/audiochannel/AudioChannelService.h:268
(Diff revision 7)
> +
> +    void AppendAgentAndIncreaseAgentsNum(AudioChannelAgent* aAgent);
> +    void RemoveAgentAndReduceAgentsNum(AudioChannelAgent* aAgent);
> +
> +    bool IsFirstAudibleAgent();
> +    bool IsLastAudibleAgent();

const ?

::: dom/audiochannel/AudioChannelService.cpp:103
(Diff revision 7)
>  IsParentProcess()
>  {
>    return XRE_GetProcessType() == GeckoProcessType_Default;
>  }
>  
> -class MediaPlaybackRunnable : public nsRunnable
> +class AudioPlaybackRunnable : public nsRunnable

final

::: dom/audiochannel/AudioChannelService.cpp:114
(Diff revision 7)
>    {}
>  
>   NS_IMETHOD Run()
>   {
>      nsCOMPtr<nsIObserverService> observerService =
>        services::GetObserverService();

I know that is not part of your patch, but:

if (NS_WARN_IF(!observerService)) {
  return NS_ERROR_FAILURE;
}

::: dom/audiochannel/AudioChannelService.cpp:251
(Diff revision 7)
>  }
>  
>  void
> -AudioChannelService::RegisterAudioChannelAgent(AudioChannelAgent* aAgent,
> +AudioChannelService::RegisterAudioChannelAgent(AudioChannelAgent* aAgent)
> -                                               AudioChannel aChannel)
>  {

MOZ_ASSERT(aAgent);

::: dom/audiochannel/AudioChannelService.cpp:258
(Diff revision 7)
>    AudioChannelWindow* winData = GetWindowData(windowID);
>    if (!winData) {
>      winData = new AudioChannelWindow(windowID);
>      mWindows.AppendElement(winData);
>    }
>  

// write a comment why...
RefPtr<AudioChannelAgent> kungFuDeathGrip(aAgent);

::: dom/audiochannel/AudioChannelService.cpp:265
(Diff revision 7)
>    MaybeSendStatusUpdate();
>  }
>  
>  void
>  AudioChannelService::UnregisterAudioChannelAgent(AudioChannelAgent* aAgent)
>  {

MOZ_ASSERT(aAgent);

::: dom/audiochannel/AudioChannelService.cpp:270
(Diff revision 7)
>  {
>    AudioChannelWindow* winData = GetWindowData(aAgent->WindowID());
>    if (!winData) {
>      return;
>    }
> +  winData->RemoveAgent(aAgent);

// write a comment why...
RefPtr<AudioChannelAgent> kungFuDeathGrip(aAgent);

::: dom/audiochannel/AudioChannelService.cpp:349
(Diff revision 7)
>  }
>  
> +void
> +AudioChannelService::AudioPlaybackChanged(AudioChannelAgent* aAgent,
> +                                          bool aAudible)
> +{

This will be all AudioAudible

::: dom/audiochannel/AudioChannelService.cpp:1020
(Diff revision 7)
> +  int32_t channel = aAgent->AudioChannelType();
> +  mAgents.AppendElement(aAgent);
> +
> +  ++mChannels[channel].mNumberOfAgents;
> +
> +  // The first one, we must inform the BrowserElementAudioChannel.

Do we have any plan to remove BrowserElementAudioChannel?

::: dom/audiochannel/AudioChannelService.cpp:1035
(Diff revision 7)
> +{
> +  MOZ_ASSERT(aAgent);
> +  MOZ_ASSERT(mAgents.Contains(aAgent));
> +
> +  int32_t channel = aAgent->AudioChannelType();
> +  // aAgent can be freed after this call.

remove this comment.

::: dom/audiochannel/AudioChannelService.cpp:1050
(Diff revision 7)
> +  }
> +}
> +
> +void
> +AudioChannelService::AudioChannelWindow::AudioCapturedChanged(AudioChannelAgent* aAgent,
> +                                                              bool isCapture)

this will use the enum

::: dom/audiochannel/AudioChannelService.cpp:1061
(Diff revision 7)
> +  }
> +}
> +
> +void
> +AudioChannelService::AudioChannelWindow::AudioPlaybackChanged(AudioChannelAgent* aAgent,
> +                                                              bool aAudible)

enum here too

::: dom/audiochannel/AudioChannelService.cpp:1100
(Diff revision 7)
> +    }
> +  }
> +}
> +
> +bool
> +AudioChannelService::AudioChannelWindow::IsFirstAudibleAgent()

const

::: dom/audiochannel/AudioChannelService.cpp:1106
(Diff revision 7)
> +{
> +  return (mAudibleAgents.Length() == 1);
> +}
> +
> +bool
> +AudioChannelService::AudioChannelWindow::IsLastAudibleAgent()

const

::: dom/audiochannel/AudioChannelService.cpp:1117
(Diff revision 7)
> +AudioChannelService::AudioChannelWindow::NotifyAudioPlaybackChanged(nsPIDOMWindowOuter* aWindow,
> +                                                                    bool aActive)
> +{
> +  RefPtr<AudioPlaybackRunnable> runnable =
> +    new AudioPlaybackRunnable(aWindow, aActive);
> +  NS_DispatchToCurrentThread(runnable);

nsresult rv = NS_DispatchToCurrentThread(...)
NS_WARN_IF(NS_FAILED(rv));

::: dom/audiochannel/AudioChannelService.cpp:1127
(Diff revision 7)
> +                                                             AudioChannel aChannel,
> +                                                             bool aActive)
> +{
> +  RefPtr<nsRunnable> runnable =
> +    new NotifyChannelActiveRunnable(aWindowID, aChannel, aActive);
> +  NS_DispatchToCurrentThread(runnable);

same here

Comment 86

[Andrea Marchesini [:baku]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

https://reviewboard.mozilla.org/r/30093/#review46001

::: dom/fmradio/FMRadio.cpp:23
(Diff revision 7)
>  #include "nsDOMClassInfo.h"
>  #include "nsIDocShell.h"
>  #include "nsIInterfaceRequestorUtils.h"
>  #include "nsIAudioManager.h"
>  
> +#include "AudioChannelService.h"

move it with the other headers.

::: dom/html/HTMLMediaElement.cpp:5073
(Diff revision 7)
> +    // element as early as we can, we would hear sound leaking if we block it
> +    // too late. In that case (block autoplay in non-visited-tab), we need to
> +    // create a connection before decoding, because we don't want user hearing
> +    // any sound.
>      AudioPlaybackConfig config;
>      mAudioChannelAgent->NotifyStartedPlaying(&config);

I really prefer:

mAudioChannelAgent->NotifyStartedPlaying(&config, mIsAudioTrackAudible);

This doesn't mean that we have to remove NotifyStartedAudible(). We still want them but I think that 'config' should be populated based on the audible state of the agent.

And in case audible is false, maybe config should have volume == 0 and muted == true.

::: dom/media/webaudio/AudioContext.cpp
(Diff revision 7)
>  {
> -  // We skip calling SetIsOnlyNodeForContext and the creation of the
> -  // audioChannelAgent during mDestination's constructor, because we can only
> +  // We skip calling SetIsOnlyNodeForContext during mDestination's constructor,
> +  // because we can only call them after mDestination has been set up.
> -  // call them after mDestination has been set up.
>    if (!mIsOffline) {
> -    nsresult rv = mDestination->CreateAudioChannelAgent();

can you tell me why it's OK to remove this CreateAudioChannelAgent() ?

::: dom/media/webaudio/AudioDestinationNode.cpp:693
(Diff revision 7)
> +{
> +  if (mIsOffline || mAudioChannelAgent) {
> +    return;
> +  }
> +
> +  mAudioChannelAgent = new AudioChannelAgent();

I need more information about why you did these changes.

Comment 87

[Andrea Marchesini [:baku]]

https://reviewboard.mozilla.org/r/35047/#review46017

Comment 88

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #85)
> ::: dom/audiochannel/AudioChannelService.cpp:1020
> (Diff revision 7)
> > +  int32_t channel = aAgent->AudioChannelType();
> > +  mAgents.AppendElement(aAgent);
> > +
> > +  ++mChannels[channel].mNumberOfAgents;
> > +
> > +  // The first one, we must inform the BrowserElementAudioChannel.
> 
> Do we have any plan to remove BrowserElementAudioChannel?
> 

If we don't need them anymore, I would file a bug to clean them up.

Comment 89

[Alastor Wu [:alwu]]

(In reply to Andrea Marchesini (:baku) from comment #86)
> ::: dom/html/HTMLMediaElement.cpp:5073
> (Diff revision 7)
> > +    // element as early as we can, we would hear sound leaking if we block it
> > +    // too late. In that case (block autoplay in non-visited-tab), we need to
> > +    // create a connection before decoding, because we don't want user hearing
> > +    // any sound.
> >      AudioPlaybackConfig config;
> >      mAudioChannelAgent->NotifyStartedPlaying(&config);
> 
> I really prefer:
> 
> mAudioChannelAgent->NotifyStartedPlaying(&config, mIsAudioTrackAudible);
> 
> This doesn't mean that we have to remove NotifyStartedAudible(). We still
> want them but I think that 'config' should be populated based on the audible
> state of the agent.
> 
> And in case audible is false, maybe config should have volume == 0 and muted
> == true.

OK.

> ::: dom/media/webaudio/AudioContext.cpp
> (Diff revision 7)
> >  {
> > -  // We skip calling SetIsOnlyNodeForContext and the creation of the
> > -  // audioChannelAgent during mDestination's constructor, because we can only
> > +  // We skip calling SetIsOnlyNodeForContext during mDestination's constructor,
> > +  // because we can only call them after mDestination has been set up.
> > -  // call them after mDestination has been set up.
> >    if (!mIsOffline) {
> > -    nsresult rv = mDestination->CreateAudioChannelAgent();
> 
> can you tell me why it's OK to remove this CreateAudioChannelAgent() ?
> 
> ::: dom/media/webaudio/AudioDestinationNode.cpp:693
> (Diff revision 7)
> > +{
> > +  if (mIsOffline || mAudioChannelAgent) {
> > +    return;
> > +  }
> > +
> > +  mAudioChannelAgent = new AudioChannelAgent();
> 
> I need more information about why you did these changes.

Actually, I already forgot why I did that.... It have been a long while ;p
But from the changeset, it seems I did too much works on AudioDestionationNode.
I would remove them and get it more simple.

Comment 90

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part5 : rename NotifyAudibleStateChanged.

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

Comment 91

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part6 : add and modify test cases.

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

Comment 92

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30091/diff/7-8/

Comment 93

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30093/diff/7-8/

Comment 94

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

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

Comment 95

[Alastor Wu [:alwu]]

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 96

[Andrea Marchesini [:baku]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

https://reviewboard.mozilla.org/r/30093/#review46239

::: dom/audiochannel/AudioChannelAgent.cpp:217
(Diff revision 8)
>    RefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate();
>    if (mAudioChannelType == AUDIO_AGENT_CHANNEL_ERROR ||
>        service == nullptr || mIsRegToService) {
>      return NS_ERROR_FAILURE;
>    }
>  

Add a static cast assertion about AudibleState == 0 nonAudible and 1 audible.

Comment 97

[Andrea Marchesini [:baku]]

Comment on attachment 8746354 [details]
MozReview Request: Bug 1235612 - part6 : add and modify test cases.

https://reviewboard.mozilla.org/r/49385/#review46249

Comment 98

[Alastor Wu [:alwu]]

Now I have some test cases fail on Android, I'm trying to fix them...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4596c7d05d9c&selectedJob=20075183

Comment 99

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

Comment on attachment 8746353 [details]
MozReview Request: Bug 1235612 - part5 : rename NotifyAudibleStateChanged.

https://reviewboard.mozilla.org/r/49383/#review46385

Comment 100

[Alastor Wu [:alwu]]

Attachment: MozReview Request: Bug 1235612 - part7 : modify the samplerate of the audio.ogg

After landing bug1264199, the audible-checking has possibility to fail when the input needs to be resamling.
Resampling only happens when the input sampling rate is not equals to device out sampling rate and we don't want the sampling problem affecting our audio-playback tests, so we change the sampling rate of these ogg file to avoid resampling.

In addition, the bug1269672 would modify the audible-checking method.

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

Comment 101

[Alastor Wu [:alwu]]

Comment on attachment 8705608 [details]
MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30091/diff/8-9/

Comment 102

[Alastor Wu [:alwu]]

Comment on attachment 8705609 [details]
MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30093/diff/8-9/

Comment 103

[Alastor Wu [:alwu]]

Comment on attachment 8719684 [details]
MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35047/diff/6-7/

Comment 104

[Alastor Wu [:alwu]]

Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

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

Comment 105

[Alastor Wu [:alwu]]

Comment on attachment 8746353 [details]
MozReview Request: Bug 1235612 - part5 : rename NotifyAudibleStateChanged.

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

Comment 106

[Alastor Wu [:alwu]]

Comment on attachment 8746354 [details]
MozReview Request: Bug 1235612 - part6 : add and modify test cases.

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

Comment 107

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10944b54ae3a

Comment 108

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

(In reply to Alastor Wu [:alwu] from comment #100)
> Created attachment 8748105 [details]
> MozReview Request: Bug 1235612 - part7 : modify the samplerate of the
> audio.ogg
> 
> 
> After landing bug1264199, the audible-checking has possibility to fail when
> the input needs to be resamling.
> Resampling only happens when the input sampling rate is not equals to device
> out sampling rate and we don't want the sampling problem affecting our
> audio-playback tests, so we change the sampling rate of these ogg file to
> avoid resampling.
> 
> In addition, the bug1269672 would modify the audible-checking method.
> 
> Review commit: https://reviewboard.mozilla.org/r/50157/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/50157/

1. Can you brief how sample rate is changed for each file? What are they before and after changes?
2. Does the changed sample rate match the device output rate on all platforms to ensure no resampling happens on all platforms?

Comment 109

[Alastor Wu [:alwu]]

Hi, JW,

(1) Sample rate changing
All the file is changed from "11127 hz" to "44100 hz".

(2) Default sampling rate
In [1], we use the 44100 hz as our default sampling rate if we can't get preference sampling rate from platform. 

This change is not used to avoid no re-sampling on all platforms, it's just used for solving the test case fails on Android. The re-sampling problem would be solved in bug1269672 because I think we should separate these problems.

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/CubebUtils.cpp#100

Comment 110

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

(In reply to Alastor Wu [:alwu] from comment #109)
> This change is not used to avoid no re-sampling on all platforms, it's just
> used for solving the test case fails on Android. The re-sampling problem
> would be solved in bug1269672 because I think we should separate these
> problems.

Do you mean:
1. the test case will not fail on other platforms even if re-sampling happens.
2. the test case will fail on Android if re-sampling happens.
???

Comment 111

[Alastor Wu [:alwu]]

Yes, it seems the timing issue so it would only happen on Android.

Comment 112

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

OK. Please describe this workaround in the comment message and promise to revert the sample rate changes after fixing bug 1269672.

Comment 113

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

Comment on attachment 8748105 [details]
MozReview Request: Bug 1235612 - part7 : modify the samplerate of the audio.ogg

https://reviewboard.mozilla.org/r/50157/#review47161

OK. Please describe this workaround in the comment message and promise to revert the sample rate changes after fixing bug 1269672.

Comment 114

[Alastor Wu [:alwu]]

Comment on attachment 8748105 [details]
MozReview Request: Bug 1235612 - part7 : modify the samplerate of the audio.ogg

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

Comment 115

[Alastor Wu [:alwu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10944b54ae3a

Comment 116

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a36228e91f76
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b1e1b55ea12
https://hg.mozilla.org/integration/mozilla-inbound/rev/f743ff82280e
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c54c2e2806
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d0d31393e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e10006e03102
https://hg.mozilla.org/integration/mozilla-inbound/rev/6154035e6d90

Comment 117

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a36228e91f76
https://hg.mozilla.org/mozilla-central/rev/0b1e1b55ea12
https://hg.mozilla.org/mozilla-central/rev/f743ff82280e
https://hg.mozilla.org/mozilla-central/rev/55c54c2e2806
https://hg.mozilla.org/mozilla-central/rev/f3d0d31393e6
https://hg.mozilla.org/mozilla-central/rev/e10006e03102
https://hg.mozilla.org/mozilla-central/rev/6154035e6d90

Comment 119

[Sorina Florean [:sflorean]]

Verified as fixed in build 49.0a1 2016-05-27;
Device: Asus ZenPad 8 (Android 5.0.2) and LG G4 (Android 5.1).