Closed Bug 1235612 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 49
Tracking Status
firefox49 --- verified

People

(Reporter: miketaylr, Assigned: alwu)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [parity-chrome][parity-safari])

Attachments

(7 files, 7 obsolete files)

58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
jwwang
: review+
Details
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.
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.
Whiteboard: [parity-chrome][parity-safari]
> 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.
(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
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.
Duplicate of this bug: 1236084
Assignee: nobody → alwu
Flags: needinfo?(amarchesini)
The implementation of patch3 wasn't correct, I'll update new patch.
Attachment #8705655 - Attachment is obsolete: true
Depends on: 1238906
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/
Attachment #8705608 - Attachment description: MozReview Request: Bug 1235612 - part1 : check whether audio data is audible. → MozReview Request: Bug 1235612 - part 1 : Implement notify media-playback.
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/
Attachment #8705609 - Attachment description: MozReview Request: Bug 1235612 - part2 : notify audible state from MDSM to ME. → MozReview Request: Bug 1235612 - part 2 : seperate the media-playback from the registration process.
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.
[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
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 :)
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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/
Flags: needinfo?(philipp)
(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.
Flags: needinfo?(philipp)
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!
Flags: needinfo?(philipp)
(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
Flags: needinfo?(philipp)
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!
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(ehsan)
Duplicate of this bug: 1240369
Depends on: 1240429
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.
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
(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?
Flags: needinfo?(philipp)
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!
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
(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...
Flags: needinfo?(ehsan)
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).
Flags: needinfo?(philipp)
Good!  Looks like we're on the same page.
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
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(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.  :-)
Flags: needinfo?(ehsan)
Because my moz-review is broken for unknown reason, using the regular review.
Attachment #8705608 - Attachment is obsolete: true
Attached patch part3 : modify tests (obsolete) — Splinter Review
In this test, we shouldn't pause audio in onplaying(), since the audio-playback isn't dispatch immediately after play().
Attachment #8719437 - Attachment is patch: true
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/
Attachment #8705608 - Attachment description: MozReview Request: Bug 1235612 - part 1 : Implement notify media-playback. → MozReview Request: Bug 1235612 - part1 : Implement notify media-playback.
Attachment #8705608 - Attachment is obsolete: false
Attachment #8705608 - Flags: review?(amarchesini)
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/
Attachment #8705609 - Attachment description: MozReview Request: Bug 1235612 - part 2 : seperate the media-playback from the registration process. → MozReview Request: Bug 1235612 - part2 : seperate the media-playback from the registration process.
Attachment #8705609 - Attachment is obsolete: false
Attachment #8705609 - Flags: review?(amarchesini)
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 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.
Attachment #8705608 - Flags: review?(amarchesini)
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
Attachment #8719684 - Flags: review?(amarchesini) → review+
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?
Attachment #8705609 - Flags: review?(amarchesini)
(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 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/
Attachment #8705608 - Flags: review?(amarchesini)
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/
Attachment #8705609 - Flags: review?(amarchesini)
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/
Attachment #8719684 - Attachment description: MozReview Request: Bug 1235612 - part3 : modify test cases. → MozReview Request: Bug 1235612 - part3 : add and modify test cases.
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!
Attachment #8719684 - Flags: review+ → review?(amarchesini)
Attachment #8719686 - Attachment is obsolete: true
Comment on attachment 8728871 [details]
MozReview Request: Bug 1235612 - part4 : modify check audible method.

https://reviewboard.mozilla.org/r/39163/#review36115
Attachment #8728871 - Flags: review?(jwwang) → review+
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.
Attachment #8705609 - Flags: review?(amarchesini)
Attachment #8719684 - Flags: review?(amarchesini)
Attachment #8705608 - Flags: review?(amarchesini)
I'm waiting for the try server result, and would ask for review again if the lights are green.
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/
Attachment #8705608 - Flags: review?(amarchesini)
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/
Attachment #8705609 - Flags: review?(amarchesini)
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/
Attachment #8719684 - Flags: review?(amarchesini)
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/
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.
(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!
Flags: needinfo?(dglastonbury)
(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.
Flags: needinfo?(dglastonbury) → needinfo?(amarchesini)
Attachment #8705608 - Flags: review?(amarchesini) → review+
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 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.
Attachment #8705609 - Flags: review?(amarchesini) → review+
Attachment #8719684 - Flags: review?(amarchesini) → review+
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
Hi, Dan,
After the offline discussion with baku, I would split them up :)
Flags: needinfo?(amarchesini)
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 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 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 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/
(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
Attachment #8705608 - Flags: review+ → review?(amarchesini)
Attachment #8705609 - Flags: review+ → review?(amarchesini)
Blocks: 1240423
Blocks: 1266681
Depends on: 1242874
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/
Attachment #8719684 - Attachment description: MozReview Request: Bug 1235612 - part3 : add and modify test cases. → MozReview Request: Bug 1235612 - part3 : modify check audible method.
Attachment #8728871 - Attachment description: MozReview Request: Bug 1235612 - part4 : modify check audible method. → MozReview Request: Bug 1235612 - part4 : add and modify test cases.
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 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 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 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
Attachment #8705608 - Flags: review?(amarchesini) → review+
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.
Attachment #8705609 - Flags: review?(amarchesini)
(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.
(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.
Review commit: https://reviewboard.mozilla.org/r/49383/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49383/
Attachment #8705609 - Attachment description: MozReview Request: Bug 1235612 - part2 : seperate the media-playback from the registration process. → MozReview Request: Bug 1235612 - part2 : notify audible state in NotifyStartedPlaying
Attachment #8719684 - Attachment description: MozReview Request: Bug 1235612 - part3 : modify check audible method. → MozReview Request: Bug 1235612 - part3 : implement the logic of audible state notification for agent owners.
Attachment #8728871 - Attachment description: MozReview Request: Bug 1235612 - part4 : add and modify test cases. → MozReview Request: Bug 1235612 - part4 : modify check audible method.
Attachment #8746353 - Flags: review?(jwwang)
Attachment #8746354 - Flags: review?(amarchesini)
Attachment #8705609 - Flags: review?(amarchesini)
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 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 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 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/
Attachment #8705609 - Flags: review?(amarchesini) → review+
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.
Attachment #8746354 - Flags: review?(amarchesini) → review+
Comment on attachment 8746354 [details]
MozReview Request: Bug 1235612 - part6 : add and modify test cases.

https://reviewboard.mozilla.org/r/49385/#review46249
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 on attachment 8746353 [details]
MozReview Request: Bug 1235612 - part5 : rename NotifyAudibleStateChanged.

https://reviewboard.mozilla.org/r/49383/#review46385
Attachment #8746353 - Flags: review?(jwwang) → review+
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/
Attachment #8748105 - Flags: review?(jwwang)
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 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 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 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 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 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/
(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?
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
Flags: needinfo?(jwwang)
(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.
???
Flags: needinfo?(jwwang)
Yes, it seems the timing issue so it would only happen on Android.
OK. Please describe this workaround in the comment message and promise to revert the sample rate changes after fixing bug 1269672.
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.
Attachment #8748105 - Flags: review?(jwwang) → review+
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/
Duplicate of this bug: 1274027
Verified as fixed in build 49.0a1 2016-05-27;
Device: Asus ZenPad 8 (Android 5.0.2) and LG G4 (Android 5.1).
Status: RESOLVED → VERIFIED
Depends on: 1284830
Depends on: 1307428
Depends on: 1344509
You need to log in before you can comment on or make changes to this bug.