805333 - Need a Policy & Mechanism for Audio Competing & Control

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Marco Chen [:mchen]]

Now the audio streams from each application are all independent so there will be multiple audio streams playing in the same time.
Ex:
  1. Music is playing then phone call coming/outgoing.
  2. Music is playing then Video is playing too.
  3. Music is playing then game with music is started too.

For reference on Android. Each audio applications need to register a AudioFocus for granting the right of playing audio then use listenAudioFocusChange to monitor whether it needs to stop or resume music.

For B2G, currently each content process played it's audio stream independently. There is no centraiized audio service for monitoring all audio streams

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

We may refer the AudioFocus concept and implement it in gecko.
1. handle cs voice call (need to notify others apps and stop the playback behavior)
2. application take over the audioFocus and notify others mechanism.
3. application dead.

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

Long before I am considering a gaia side solution to this, since we have decided to solve it in gecko side, but the mechanism may be similar:

0. Use mozApp iframe as audio/video units.
1. Gecko must know each mozApp iframe's audio/video status.
2. When a mozApp iframe is starting to playing audio/video, an audio manager would recieve a start event or sth. like that.
3. Sound manager must determines who should stop playing, who should continous playing. It will have an internal priority queue.
4. Priority: voicecall > FM > Media
4.1 Priority: Foreground app > background app, if they are using the same media channel.
5. For UX sake, we could determine when a high-priority app is terminated, if we are going to choose another second-high-priority app to resume the audio playing. Use case: When Ringtone ends -> Music continues to play.
6. Because audio manager itself knows current channel type, when it recieves hardware buttons controls events(volume up key, volume down key), it could determine which mozSettings should be changed according to the hardware keys.
For example, if the fucosed mozApp iframe is in voicecall channel, when the user presses the hardware volume key, the sound manager(IMO here is gaia's system app's sound manager) would get volumeup event, so it then changes 'audio.volume.voicecall' to +1 instead of changing 'audio.volume.system' to +1.
7. The app could register channel by webapp.manifest. An app registers to voicecall must have phonecall permission.
(8. Sound manager -- if it's implemented in gaia -- sends mozContentEvent back to gecko to let some mozApp iframe stop/resume playing sound.)

Please considering how to communicate with gaia if you are going to solve those volume issues in gecko.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive] from comment #2)
> Please considering how to communicate with gaia if you are going to solve
> those volume issues in gecko.

If you determine not to expose focused audio information to gaia, but to handle hardware keys in gecko -- because the only one knows the current focused channel type is gecko -- please inform me/gaia dev, some audio related codes in system app must be removed or refactored.

But I think expose that info to system app is needed. By mozChromeEvent or whatever.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Marco Chen [:mchen] from comment #0)
> Now the audio streams from each application are all independent so there
> will be multiple audio streams playing in the same time.
> Ex:
>   1. Music is playing then phone call coming/outgoing.
>   2. Music is playing then Video is playing too.
>   3. Music is playing then game with music is started too.
> 
> For reference on Android. Each audio applications need to register a
> AudioFocus for granting the right of playing audio then use
> listenAudioFocusChange to monitor whether it needs to stop or resume music.
> 
> For B2G, currently each content process played it's audio stream
> independently. There is no centraiized audio service for monitoring all
> audio streams

AudioFocus looks like a reasonable way to do it for apps, but we should aim for not breaking the web by asking every website with <audio> to call special API before audio.play().

Also, I am not sure implementing AudioFocus is doable within the scope of v1. Can we not implementing that but still figure out a way out of the current usability issue?

Comment 5

[Marco Chen [:mchen]]

https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU

Please refer to link as above for our proposal now.
And welcome for any feedback. Thanks in advance.

Comment 6

[Sotaro Ikeda [:sotaro]]

IMHO, an app playing audio need to get higher oom_adj than normal background apps.
Otherwise, the app could easily killed when oom.

Comment 7

[Sotaro Ikeda [:sotaro]]

> https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU

The document thinks about to stop only app's audios, do not stop web contents audios. IMHO, web contents' audios also needs to be stopped. There are pros and cons. Though it could provide better user experience, I think. I briefly checked about it in android case in bug 796333 comment #29

If it is controlled by gaia's system app, OEM vendors/operators can choose, when to forcibly stop/pause audio. From my experince, operators in Japan want to control when audio payback to stop from user's availability point of view.

Anyway, when web browser go to background, video tags in web contents need to be stopped and video codecs need to be freed. In mobile memory and hw codecs are very scarce resource.

Comment 8

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> IMHO, an app playing audio need to get higher oom_adj than normal background
> apps.
> Otherwise, the app could easily killed when oom.

Good point, will take it into consideration.

Comment 9

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU
> 
> The document thinks about to stop only app's audios, do not stop web
> contents audios. IMHO, web contents' audios also needs to be stopped. There
> are pros and cons. Though it could provide better user experience, I think.
> I briefly checked about it in android case in bug 796333 comment #29
> 

The browser in Firefox OS is an App too. So based on this proposal if browser's manifest didn't have "background_play" permission then it's audios will be stop once in background.

> If it is controlled by gaia's system app, OEM vendors/operators can choose,
> when to forcibly stop/pause audio. From my experince, operators in Japan
> want to control when audio payback to stop from user's availability point of
> view.
> 

If possible, could you provide the use case that this proposal may not to handle?
It will be very helpful for us. Thanks for your kindly reply.

> Anyway, when web browser go to background, video tags in web contents need
> to be stopped and video codecs need to be freed. In mobile memory and hw
> codecs are very scarce resource.

I tested that video App will stop after it goes into background.
But I am not sure the resource management performed by Video app.

Comment 10

[Sotaro Ikeda [:sotaro]]

> > Anyway, when web browser go to background, video tags in web contents need
> > to be stopped and video codecs need to be freed. In mobile memory and hw
> > codecs are very scarce resource.
> 
> I tested that video App will stop after it goes into background.
> But I am not sure the resource management performed by Video app.

I misunderstood about how video is played on FirefoxOSv1. An Video tag in a web content is played only in video app via WebActivity, the video is not played in web browser.

My comment is about video playback within web browser. If FirefoxOS supports video playback or WebRTC in web browser. It is necessary.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #8)
> (In reply to Sotaro Ikeda [:sotaro] from comment #6)
> > IMHO, an app playing audio need to get higher oom_adj than normal background
> > apps.
> > Otherwise, the app could easily killed when oom.
> 
> Good point, will take it into consideration.

FYI:

oom_adj is changed by hal::SetProcessPriority(). It changes oom_adj and nice. But currently, it do not change scheduling policy.

In android case, when an app goes into background, scheduling policy is also changged via android_os_Process_setProcessGroup().
http://androidxref.com/4.0.4/xref/frameworks/base/core/jni/android_util_Process.cpp#181

when a process' scheduling policy is set to background, CPU usage for all background processes is bounded at 5%.
http://en.wikipedia.org/wiki/Background_process
http://web.archive.org/web/20101017123040/http://gizmodo.com/5527407/giz-explains-how-multitasking-works-on-a-phone

Scheduling policy initialization is done in init.rc
http://androidxref.com/4.0.4/xref/system/core/rootdir/init.rc#72

Comment 12

[Sotaro Ikeda [:sotaro]]

> > If it is controlled by gaia's system app, OEM vendors/operators can choose,
> > when to forcibly stop/pause audio. From my experince, operators in Japan
> > want to control when audio payback to stop from user's availability point of
> > view.
> > 
> 
> If possible, could you provide the use case that this proposal may not to
> handle?
> It will be very helpful for us. Thanks for your kindly reply.

I thought there are use cases that music needs to be stopped without changing app's stage forground/background or display on/off. From the document, it is not clear for me about it. It could be already solved in the document.

Followings are a few examples that I could think off the top of my head.

[1] during music playback by a music app, music do not stop when a user launch telephone app.
     But stop music when making a call.

[2] user select to power off via menu
In android, when a user select to power off, music playback is stopped and shutdown animation is shown during shutdown. But current FirefoxOS do not show animation during shutdown. It seems just quickly power off. Therefore currently, it is not related to FirefoxOS...

[3] emergency earthquake alert system
In Japan, there are emergency earthquake alert systems. When massive earthquake occurrs, alert is sent via area mails to a mobile phones in affected areas. It is like 20 seconds before an earthquake. During warning, the app is not come to forground, just notification is present in status bar and played alert sound. 
In this case, "ENFORCED_AUDIBLE" audio track could be used and could be outside of this problem.

Comment 13

[Sotaro Ikeda [:sotaro]]

> https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU

From the document, it is not clear for me about the priority of contention.
Telephone should have most high priority. It should not be stopped even when other app's request the audio.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Marco Chen [:mchen] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #7)
> > > https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU
> 
> The browser in Firefox OS is an App too. So based on this proposal if
> browser's manifest didn't have "background_play" permission then it's audios
> will be stop once in background.

In web browser app, web contents run in a different process than the app' process.
Does it work also in this case?

Comment 15

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> I thought there are use cases that music needs to be stopped without
> changing app's stage forground/background or display on/off. 

The proposal noted that two roles will be joined competing permission.
  Role 1: who have "background_play" permission.
  Role 2: Audio Competing Service will actively listen the phone and FM state because they didn't have audio stream sent from Gaia.

Refer to your example 1, according to that competing decision will be triggered by timing of phone state, the music should be stopped when detecting in_call state.

Example 2, the menu will let music App fall into background then music is stopped.

Comment 16

[Marco Chen [:mchen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #13)
> > https://docs.google.com/open?id=0BxKpB0y8NZSHYjhJeUtQUkRQVUU
> 
> From the document, it is not clear for me about the priority of contention.
> Telephone should have most high priority. It should not be stopped even when
> other app's request the audio.

Telephony's audio is not came from Gaia but from modem processor/chip to audio codec directly.
So telephony will never be stopped by this audio competing policy.

Comment 17

[Sotaro Ikeda [:sotaro]]

Maximum number of audio out streams(AudioTrack) of AudioFligner is about 32. They are also scarce resources. Therefore, it is better that a web browser app releases android::AudioTracks in web contents when the app goes to background, I think.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I sketched out an API here: https://wiki.mozilla.org/WebAPI/AudioChannels

It's probably wrong in all sorts of ways, so please do provide feedback.

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

When user pick-up the phone call, I think the music/video player could stop instead just of mute it because
1. Can save more power
2. the UI seek bar can keep at the position of the timing that user answer the phone call.

Comment 20

[Marco Chen [:mchen]]

Use Case 1: A game with music is lunched then screen is off due to timeout of idle timer.

  What happened in this new proposal if a game didn't actively pause the music according to 
  "onmutechange" event?
  I said that user will see screen is off and no sound appeared so user misunderstand that 
  device is really in suspend (sleep) mode. But actually the muting PCM stream from game's 
  music will keep device in early suspend mode due to wakelock acquired by audio driver or 
  HAL (ex: Otoro).

  Result: After a few hours ago, user tried to wake up the phone but it is dead already.

Use Case 2: The music player is playing a music in the background then user want to play video.

  Both of audio stream played from music and video player will be in the same audio 
  channel (ex: normal). So in this scenario both of music and video player will keep to play.

  Result: In popular smart phones now, user will expect early one (music player) should be 
  stopped.

Use Case 3: During a phone call, the SMS or alarm is coming.

  Due to telephony channel will mute notification channel, user didn't have chance to be 
  notified about SMS or alarm.

Discussion 4: Maybe Wrong Description in proposal on "For now all audio channels except "telephony" never use the built-in earpiece."

  Actually during phone call the audio routing path is set to built-in earphone, it means 
  that the system tones or music played will also be routed to earphone.
  Note: In some platforms it may just not allow other PCM streams mixed with voice_call but 
  this is a HW limitation. And otoro can allow it.

Discussion 5: Based on use case 1, if there are many broken App launched by user and put on background, the all musics are just muted but they still waste the CPU power and battery life.

  This loose mechanism seems hide the audio competing issue and user's ear but broken apps still play the music and user has no chance to find out these bad Apps.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Absolutely. That's why the API fires the mutechange event, to give the app an opportunty to pause any audio. I don't think that we can automatically pause any playing audio since that can mess up the application. For example it might have animations or other things synchronized with the sound. Web pages generally don't rxpect the platform to mess arohnd with their internal state.

However note that there is no reason we couldn't stop decoding, or even doing IO, for the audio since that is completely transparent to the app.

Comment 22

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #21)
> However note that there is no reason we couldn't stop decoding, or even
> doing IO, for the audio since that is completely transparent to the app.

1. This seems to reply for use case 1 and discussion 5.
   What is the difference between stop decoding you mentioned above and pause audio directly ? 
   And if you just stop decoding, this still effect the synchronization between App state and decoding state machine.

2. How about the use case 2 for competing between the same audio channel and use case 3 for case of mixing from different audio channels?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Marco Chen [:mchen] from comment #22)
> (In reply to Jonas Sicking (:sicking) from comment #21)
> > However note that there is no reason we couldn't stop decoding, or even
> > doing IO, for the audio since that is completely transparent to the app.
> 
> 1. This seems to reply for use case 1 and discussion 5.

Actually, I mid-aired. My comment was in reply to comment 19.

>    What is the difference between stop decoding you mentioned above and
> pause audio directly ? 
>    And if you just stop decoding, this still effect the synchronization
> between App state and decoding state machine.

I might be using the wrong wording here.

Basically, my goal is to to reduce the chance of "breaking" a website or webapp when we are muting a set of channels. We should expect that most apps will not be written such that they are aware that they can get muted when there's an incoming phone call or alarm. So we should try to ensure that an app that takes not action when that happens doesn't end up in an inconsistent state or otherwise starts behaving different than it otherwise would.

The way I was trying to accomplish that is by making the API exposed through HTMLMediaElement behave the same way even when the channel that it uses is muted. The only thing that we need to keep consistent is the API. So that for example if the app is waiting for the .currentTime property to reach a specific value before it stops an animation that that still happens.

This doesn't mean that we have to actually do any decoding or indeed do any IO (at least if we're playing a local file). As long as that isn't visible through the API.

We should still enable the app to stop the music by calling HTMLMediaElement.pause(), that's why I'm suggesting that we fire an event like the "mutedchange" event. But we shouldn't call HTMLMediaElement.pause() ourselves since we should basically consider the currentTime property of the media element as internal application state, and messing around by that in ways that the app wasn't expecting (even if we have documented it) is likely to cause apps to break.


However I think that we have complete freedom in what we do internally in Gecko. So if we want, and have the time to, we could put the element into some sort of "faking state" where it basically doesn't do any more processing than it would have if we had called .pause(), except that it still fakes moving the time .currentTime etc forward. As long as we do that, we shouldn't break any synchronization that the app is doing.


Things do get a bit more complicated on elements where the webpage is actively accessing the audio data, for example through HTMLMediaElement.mozCaptureStream(). In that case we would still need to decode the audio since we have no idea what the element is doing with the data.


I hope that describes my basic reasoning? Let me know if you disagree.


With that in mind, let me answer comment 20:

> Use Case 1: A game with music is lunched then screen is off due to timeout
> of idle timer.
> 
>   What happened in this new proposal if a game didn't actively pause the
> music according to 
>   "onmutechange" event?
>   I said that user will see screen is off and no sound appeared so user
> misunderstand that 
>   device is really in suspend (sleep) mode. But actually the muting PCM
> stream from game's 
>   music will keep device in early suspend mode due to wakelock acquired by
> audio driver or 
>   HAL (ex: Otoro).
> 
>   Result: After a few hours ago, user tried to wake up the phone but it is
> dead already.

I hope that my description above explains things. Basically, I think we should do whatever battery saving measures that you had in mind, as long as we move the .currentTime forward for all media elements.

So the audio driver shouldn't need to hold the wakelock here.

Does that solve your concern here?

> Use Case 2: The music player is playing a music in the background then user
> want to play video.
> 
>   Both of audio stream played from music and video player will be in the
> same audio 
>   channel (ex: normal). So in this scenario both of music and video player
> will keep to play.
> 
>   Result: In popular smart phones now, user will expect early one (music
> player) should be 
>   stopped.

This is a tricky case. I'm not sure that we always do want to pause background music when a app is playing audio through the "normal" channel.

How do we tell apart a video app playing something where it makes sense to mute/pause background music vs. puzzle game making small sound effects which should not pause background music?

I'm interested in hearing if anyone has ideas for how to solve this. Maybe introducing an explicit "music" channel might make sense?

> Use Case 3: During a phone call, the SMS or alarm is coming.
> 
>   Due to telephony channel will mute notification channel, user didn't have
> chance to be 
>   notified about SMS or alarm.

The alarm app can tell that the alarm channel has been muted and so can hold off on playing the alarm until the channel is unmuted.

We should also introduce a way for background apps to notify the user using vibration, but I'm not sure that we'll have time for that in v1 since it's not something that we've started on yet.

> Discussion 4: Maybe Wrong Description in proposal on "For now all audio
> channels except "telephony" never use the built-in earpiece."
> 
>   Actually during phone call the audio routing path is set to built-in
> earphone, it means 
>   that the system tones or music played will also be routed to earphone.
>   Note: In some platforms it may just not allow other PCM streams mixed with
> voice_call but 
>   this is a HW limitation. And otoro can allow it.

Isn't the common case here that when you are using the earpiece for telephony audio, all other audio is muted/paused and so won't be playing anywhere?

> Discussion 5: Based on use case 1, if there are many broken App launched by
> user and put on background, the all musics are just muted but they still
> waste the CPU power and battery life.
> 
>   This loose mechanism seems hide the audio competing issue and user's ear
> but broken apps still play the music and user has no chance to find out
> these bad Apps.

I hope my description above answers this?

Comment 24

[Marco Chen [:mchen]]

(In reply to Jonas Sicking (:sicking) from comment #23)
> (In reply to Marco Chen [:mchen] from comment #22)
> > (In reply to Jonas Sicking (:sicking) from comment #21)
> > Use Case 1: A game with music is lunched then screen is off due to timeout
> > of idle timer.
> > 
> 
> I hope that my description above explains things. Basically, I think we
> should do whatever battery saving measures that you had in mind, as long as
> we move the .currentTime forward for all media elements.
> 
> So the audio driver shouldn't need to hold the wakelock here.
> 
> Does that solve your concern here?
> 

I think until now we both agreed that Gecko should stop the PCM stream from MediaElement to audio driver for avoiding wakelock which will block device into suspend but early suspend only. So you mentioned about the "fake" pause in Gecko for preventing App from breaking state.

In the opposite, this "fake" pause may break the MediaElement's state. So it puts more complicated case for MediaElement to distinguish this pause a real or a fake one.

Then once the App "feels" it still be in playing state. what will be happened when the current song is end (by listen onend event from mediaelement)? 
Ex: in our music.js, it create a timer to count the end of a song then jump to next while timeout. 

Do we really need to ideally cover all scenarios from App in order to cover the Bad App in this real world?
Or just define a API and mechanism to let App follow.

> > Use Case 2: The music player is playing a music in the background then user
> > want to play video.
> > 
> I'm interested in hearing if anyone has ideas for how to solve this. Maybe
> introducing an explicit "music" channel might make sense?
> 

Please refer to our proposal, which can solve this kind of problem.
The basic point is that our proposal created a singleton audio service in b2g process.
Then each mediaelement (belonged to an App who has background play permission) will register to audio service via IPC. Then audio service can adjudge to stop early one (music player) and keep latter one (video player).

So our proposal is based on unit of media element not channel.

> > Use Case 3: During a phone call, the SMS or alarm is coming.
> > 
> The alarm app can tell that the alarm channel has been muted and so can hold
> off on playing the alarm until the channel is unmuted.
> 

Use the same ideal thought, how about an alarm app didn't follow this proposal?
Then this alarm will be missed.

It seems only App itself can handle this situation.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Jonas Sicking (:sicking) from comment #21)
> I don't think that we can automatically
> pause any playing audio since that can mess up the application. For example
> it might have animations or other things synchronized with the sound.

This isn't really correct. For example, generally Web applications cannot guarantee that a 10s audio clip will take exactly 10s to play back. The Web browser is allowed to temporarily pause playback for internal reasons (e.g. the data has to be (re)downloaded).

So, if we want to pause playback for some reason, I think we can do that.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think there's a few separate questions here:
1) What should the default behavior be when a app or Web page's audio needs to be interrupted by higher-priority audio?
2) What API should we provide so apps can provide their own behavior for handling interruptions?
3) What API and policy should we have for determining when to interrupt application audio?

1): I think automatically pausing media elements would be a pretty good default, better than the other option of muting.

Imagine that an app (or webpage) wants to play sound while it's in the background, and a phone call happens. Here are some scenarios:
-- the sound is an alert to attract the user's attention. Pausing (and automatically resuming at the end of the phone call) would be better than muting.
-- the sound is music or an audiobook. Pausing would be better than muting.
-- the sound is sound effects for a game. The game should probably be completely paused while it's in the background, so this shouldn't arise.
-- the sound is background music for a game. Pausing would be just as good as muting.
I can't think of a situation where muting would be better than pausing.

Note that we'd need to handle the case where a stream starts playing during an interruption. It needs to pause immediately and resume after the interruption has ended.

2): Firing an event on a media element when it is about to be automatically paused or resumed, and allowing preventDefault() on that event, sounds like a good way for apps to override the automatic pause behavior if they need to and have the proposed "play-background" privilege. Maybe we could call the events "begininterruption" and "endinterruption".

3): Quite a few options would work here.

A list of channels ordered by priority, like Jonas suggested, sounds like a reasonable approach. When a stream associated with a channel starts playing, that's when we can interrupt all lower-priority streams. We probably also need to define priorities for streams with the same channel (e.g. most recently foreground application wins). I don't know that we really need an AudioChannelManager interface; it might be enough to just have a channel="..." attribute on media elements, and handle the rest in Gecko. Non-media-element channels like telephony would easily fit into this.

Comment 27

[Marco Chen [:mchen]]

From my point of view, there is three topic needed to discuss or decision.

1. Mute or Pause?
Thanks for roc's information. So could we have a decision on pausing the unused audio streams but muting or "fake pause"?

2. How to solve the audio competing between audios from the same mozChannel?
  a. Jonas's proposal seems that can't solve this issue.
  b. Refer to ours proposal, it covered this case.
So any improment on Jonas's proposal to keep this case working?

3. Solve this Bug 805333 on Web API level?
  a. roc throw a question about this and refer to our proposal, we just need one more web API in media element.
So any comparison be done between ours and Jonas?

Thanks.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> I think there's a few separate questions here:
> 1) What should the default behavior be when a app or Web page's audio needs
> to be interrupted by higher-priority audio?
> 2) What API should we provide so apps can provide their own behavior for
> handling interruptions?
> 3) What API and policy should we have for determining when to interrupt
> application audio?
> 
> 1): I think automatically pausing media elements would be a pretty good
> default, better than the other option of muting.
> 
> Imagine that an app (or webpage) wants to play sound while it's in the
> background, and a phone call happens. Here are some scenarios:
> -- the sound is an alert to attract the user's attention. Pausing (and
> automatically resuming at the end of the phone call) would be better than
> muting.
> -- the sound is music or an audiobook. Pausing would be better than muting.
> -- the sound is sound effects for a game. The game should probably be
> completely paused while it's in the background, so this shouldn't arise.

Note that we don't yet pause applications when they are put in the background. It's something I wish that we did, but so far the decision seems to be not to.

So this will definitely happen at least for now.

> -- the sound is background music for a game. Pausing would be just as good
> as muting.
> I can't think of a situation where muting would be better than pausing.

I'm still very worried that applications will not expect many audio streams to be paused mid-stream. I agree that it can happen for http-streams, but games in particular are likely to be playing audio from blobs and won't expect any pausing.

Also note that there are two scenarios where we've been talking about pausing or muting audio: When a higher-priority audio needs to be played, and when the user switches to a different app. Note that in neither case do we currently pause any other aspect of the app.

Are you suggesting that we in both these cases pause any media elements?

> 2): Firing an event on a media element when it is about to be automatically
> paused or resumed, and allowing preventDefault() on that event, sounds like
> a good way for apps to override the automatic pause behavior if they need to
> and have the proposed "play-background" privilege. Maybe we could call the
> events "begininterruption" and "endinterruption".

Preventing the pausing should only be allowed when we're pausing due to the user leaving the app, not when we're pausing due to a higher-priority audio channel being used. I.e. we don't want to allow applications to opt-in to playing music while you're on the phone, especially when you're dialing an emergency call.

So we need pages to be able to tell the two apart if we're going with the events+preventDefault() solution.

> 3): Quite a few options would work here.
> 
> A list of channels ordered by priority, like Jonas suggested, sounds like a
> reasonable approach. When a stream associated with a channel starts playing,
> that's when we can interrupt all lower-priority streams. We probably also
> need to define priorities for streams with the same channel (e.g. most
> recently foreground application wins). I don't know that we really need an
> AudioChannelManager interface; it might be enough to just have a
> channel="..." attribute on media elements, and handle the rest in Gecko.
> Non-media-element channels like telephony would easily fit into this.

We can probably do without an AudioChannelManager, though we still need someplace to communicate information about whether headphones are plugged in.

Note that simply using the "more recently used" solution won't work for same-channel scenarios. We don't want simple sound effects in one app to pause any background app playing music.

So far none of the proposed solutions I've seen is able to differentiate the following two usecases:
* A simple sound effect playing in the foreground app shouldn't override a
  background app playing music, but rather mix with it.
* A video app starting to play music in the forground app should mute a background
  app playing music.

Comment 29

[Andrea Marchesini [:baku]]

Attachment: feedback needed

> 1. Mute or Pause?

Mute for sure. Pause can break the logic of the app.

I think that the audio mute/unmute should be transparent for the application.
I wrote a demo with these ideas:

1. a new attribute for a media element: mozAudioChannel.
The value is what Jonas describes as audio channels: normal, effect, content, alarm, ...
Default value: normal.

2. When an app is not visible on screen, all its MediaElement mozAudioChannel == normal|effect are muted.
I'm using a 'separated' muted, so the app doesn't know that it's been muted.
I meant: audio.muted is still 'false' but maybe the audio element is acutally been muted. This is because changing the mute state, can break the app logic.

3. If the app wants to follow the new mute state, we have 2 new events for the media element: mozmuted and mozunmuted

4. any media element is registerd with a AudioChannelService. This is a singleton.
I'm adding the support for IPC too.

5. the AudioChannelService notifies any media element when they should be muted/unmuted following these rules:

a) If there is at least 1 mediaElement.mozAudioChannel == dialer, any others are muted
b) If there is not a mozAudioChannel == dialer, but at least 1 alarm, any others are muted
c) Contents, effects, normals can run together
d) This logic was just a proof of concept and it can be improved.

> 2. How to solve the audio competing between audios from the same mozChannel?

My approach should fix this problem. Is it? And this is hidden from the app developer.
Special apps (as such as games, etc) they should listen to visibilitychange event and mute/stop/pause themselves.

I'll attach what I wrote for feedback. I wrote it flying back home, so take it as a proof of concept :)

Comment 30

[Andrea Marchesini [:baku]]

https://etherpad.mozilla.org/audio-bug80533 this is the current policy implemented by my patch

Comment 31

[Andrea Marchesini [:baku]]

Attachment: Patch 1 - mozAudioChannel for MediaElement

This patch adds mozAudioChannel attribute to HTML Media elements.
On top of this patch we can implement separately:

. the Gonk volume support - Marco
. AudioChannelService - Baku
. Pause for HTML Media Element - Baku/Marco (?)

Comment 32

[Andrea Marchesini [:baku]]

Attachment: Patch 1 - mozAudioChannel for MediaElement

Comment 33

[Andrea Marchesini [:baku]]

Attachment: Patch 1 - mozAudioChannel for MediaElement

UUID for nsHTMLMediaElement.idl

Comment 34

[Andrea Marchesini [:baku]]

Attachment: Patch 1 - mozAudioChannel for MediaElement

1. muted -> paused
2. ringer removed

Comment 35

[Andrea Marchesini [:baku]]

Attachment: Patch 1 - mozAudioChannel for MediaElement

just other detail.

Comment 36

[Marco Chen [:mchen]]

Comment on attachment 680564 [details] [diff] [review]
Patch 1 - mozAudioChannel for MediaElement

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

Dear Baku,

I am not a reviewer so I change the review to feedback+.
And will based on this patch to work on Bug 795237.
And feedback will discuss with you too.

Thanks.

Comment 37

[Marco Chen [:mchen]]

Discussd with Baku about re-assign owner to him.

Comment 38

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation

This patch is still using the 'mute' approach. I need info for implementing the pause in nsHTMLMediaElement.

Comment 39

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - IPC

This is IPC support for AudioChannelService.

Comment 40

[Andrea Marchesini [:baku]]

I uploaded what I have written until now.
This is far to be perfect but it works. The current open points are:

1. muted -> paused. Now I'm still muting the media element instead pausing it.

2. I saw some crash closing the browser if the media element is playing

3. multiple content audio channel on display (this is not supported yet).

Comment 41

[Marco Chen [:mchen]]

Dear Baku & Reviewer,

1. Sync the status from bug 795237.
   About the IDL of new attribute called "mozAudioChannelType" is review+ already. About integrating mozAudioChannelType to Gonk layer is review+  too.

   So please ignore that part in this review and reserve the effort into audio competing policy and mechanism based on audio channel types.

2. I think there are a lot works here. So if Baku is ok, I can help on work of permission check for audio channel type then keep your focus on this policy.

Comment 42

[Andrea Marchesini [:baku]]

> 2. I think there are a lot works here. So if Baku is ok, I can help on work
> of permission check for audio channel type then keep your focus on this
> policy.

The permission check should be already implemented. I'm testing it right now.
Any app can have a set of audio channel permitted from its manifest.
I'll write an update here about my testing for permissions asap.

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 681469 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation

Roc, or someone on his team, is likely a better reviewer for this.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 681469 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation

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

Assigning to Matthew since he looked at the related work in bug 795237

Comment 45

[Dietrich Ayala (:dietrich)]

Looks like a couple of patches waiting for feedback? Can these be expedited? Andrea, you should contact the reviewers directly if they're not responding.

Tomorrow is the last day that we can easily land feature work such as this, so would be great to finalize this asap.

Comment 46

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 681470 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - IPC

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

This looks ok to me, but bent should probably review it.

::: dom/apps/src/PermissionsInstaller.jsm
@@ -262,5 @@
>                               app: DENY_ACTION,
>                               privileged: DENY_ACTION,
> -                             certified: ALLOW_ACTION
> -                             access: { "content", "notification", "alarm",
> -                                       "telephony", "publicnotification" }

I don't think we should use the 'access' property for this. We can add a "channels" property instead. It's totally ok to modify the code that reads from this table so that it has smarts about a "channels" property.

Comment 47

[Matthew Gregan [:kinetik]]

Comment on attachment 681469 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation

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

This looks good, thanks.  Someone on the content team should review this, since I'm not familiar with the permission handling stuff.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +388,5 @@
>      return mSrcStream->GetStream();
>    }
>  
> +  // Notification from the AudioChannelService.
> +  nsresult AudioChannelNotification();

The name isn't clear to me.  NotifyAudioChannelStateChanged or something?

@@ +652,5 @@
> +  // Check if the app has the permission to use this type.
> +  nsresult MozAudioChannelPermission(mozilla::dom::AudioChannelType aType);
> +
> +  // This method does the check for muting/unmuting the audio channel.
> +  nsresult MozMutedCheck();

This doesn't need a "Moz" prefix either.  Maybe call this UpdateChannelMuteState or something?

@@ +921,5 @@
> +
> +  // The audiochannel has been muted
> +  bool mMozMuted;
> +
> +  // The audiochannel has been muted

hidden?

@@ +922,5 @@
> +  // The audiochannel has been muted
> +  bool mMozMuted;
> +
> +  // The audiochannel has been muted
> +  bool mMozHidden;

This and mMozMuted don't need "Moz" in the names.  Maybe name them mChannelMuted and mElementHidden?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3890,5 @@
> +  // We have to mute this channel:
> +  if (mute && !mMozMuted) {
> +    mMozMuted = true;
> +    SetMutedInternal();
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozpaused"));

I'm not sure if paused/unpaused is the right name for these events if we're only muting.  If we're also going to use them for situations where playback is completely paused it might be okay.  I'd prefer a clearer name for them, though.

::: dom/audiochannel/AudioChannelService.cpp
@@ +82,5 @@
> +
> +bool
> +AudioChannelService::GetMuted(AudioChannelType aType, bool aMozHidden)
> +{
> +  // We are not visibile, maybe we have to mute:

visible

Comment 48

[Andrea Marchesini [:baku]]

> I don't think we should use the 'access' property for this. We can add a
> "channels" property instead. It's totally ok to modify the code that reads
> from this table so that it has smarts about a "channels" property.

I agree with you. But changing how we read the table will be a too complex patch. I prefer to split it. So now I'm uploading a patch just with the implementation of the mozAudioChannel without permissions. The permission check is going to be a separated patch.

Comment 49

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation (b)

Comment 50

[Andrea Marchesini [:baku]]

Attachment: Patch 3 - mozAudioChannel for MediaElement - IPC (b)

Comment 51

[Andrea Marchesini [:baku]]

Attachment: patch 4 - permission for mozAudioChannel attribute

This patch must be extended in order to implement what Jonas suggested in the comment 46.

Comment 52

[Marco Chen [:mchen]]

Dear Andrea & Jonas,

About the work of patch 1, it is alreayd covered by Bug 795237 (landed).
So you can save the effor on this one.

Thanks.

Comment 53

[Marco Chen [:mchen]]

Dear Andea & Jonas,

I saw that there are two new events called mozPaused and mozUnpaused proposed for notifying Web Apps about audio state changing. But as I knew that there were two events called 'pause' and 'play' already.

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1393
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1865

So do we need to have these two new events?

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't want to fire 'pause' and 'play' here.

I think in the API, instead of referring to 'pause' or 'mute' since we're going to change the behavior here, we could refer to 'interrupt' instead. For example the events could be called 'interruptbegin' and 'interruptend'.

Comment 55

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation (c)

mozpaused/mozunpaused renamed to mozinterruptbegin/mozinterruptend

Comment 56

[Alex Keybl [:akeybl]]

Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.

Comment 57

[Marco Chen [:mchen]]

Dear Andreas,

One more thing here, I seems not to see AudioChannelService to monitor the status of Telephony and FM state. According to that voice call and FM didn't have a media element in Gecko, AudioChannelService will not be noticed about them.

Comment 58

[Andrea Marchesini [:baku]]

Good point. I'll add this an a separated patch.

Comment 59

[Marco Chen [:mchen]]

Dear Andreas & Jonas,

1. Another use case coming and can't be solved in current implementation. (bug 811649)

  a. Start to play a song from music player.
  b. Then start video recording to record something.
  
  Expectation: The music player can stop the music during step b.

  Q: Do we need to add an API (ex: Request/ReleaseAudioChannel) into AudioChannelManager for Apps which need to ask others to stop using audio channel?

2. Maybe we need to get first version of this bug to land then we can optimize it by different people and different bugs quickly? (ex: split "pause/mute", "monitor telephony or FM" or "new other API" into another bugs.) 

   Could we define the patch list now is a final version of this bug then optimize it by another bugs?

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Marco Chen [:mchen] from comment #59)
>   a. Start to play a song from music player.
>   b. Then start video recording to record something.
>   
>   Expectation: The music player can stop the music during step b.

Can we achieve this by playing silence on the "alarm" channel?

Comment 61

[Kai-Chih Hu [:khu]]

I created bug 815046 to trace comment #58.

Comment 62

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 680564 [details] [diff] [review]
Patch 1 - mozAudioChannel for MediaElement

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

sr=me, but someone that knows this code better than me needs to review it. Someone in roc's team most likely.

Comment 63

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 683567 [details] [diff] [review]
Patch 3 - mozAudioChannel for MediaElement - IPC (b)

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

someone that knows the media code better than me needs to review.

Comment 64

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> (In reply to Marco Chen [:mchen] from comment #59)
> >   a. Start to play a song from music player.
> >   b. Then start video recording to record something.
> >   
> >   Expectation: The music player can stop the music during step b.
> 
> Can we achieve this by playing silence on the "alarm" channel?

From the tech of view, it can be achieved. But from the arch design of view, this seems to be a workaround.
Refer to bug 811649, it just notify us that there will be another Apps needed to join audio competing policy not only video recording or who created media elements. Ex: audio recording, VOIP application and voice recognize.
So I suggest that we need to have a way for these Apps without media elements can join audio policy.
If this is really needed to do then it will be traced by another bugs not here.

Comment 65

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 683992 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (c)

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

Same here unfortunately.

There seems to be some permissions stuff missing here though? Or are you planning on putting that in later?

Comment 66

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Marco Chen [:mchen] from comment #59)
> Dear Andreas & Jonas,
> 
> 1. Another use case coming and can't be solved in current implementation.
> (bug 811649)
> 
>   a. Start to play a song from music player.
>   b. Then start video recording to record something.
>   
>   Expectation: The music player can stop the music during step b.
> 
>   Q: Do we need to add an API (ex: Request/ReleaseAudioChannel) into
> AudioChannelManager for Apps which need to ask others to stop using audio
> channel?
> 
> 2. Maybe we need to get first version of this bug to land then we can
> optimize it by different people and different bugs quickly? (ex: split
> "pause/mute", "monitor telephony or FM" or "new other API" into another
> bugs.) 
> 
>    Could we define the patch list now is a final version of this bug then
> optimize it by another bugs?

Yeah, lets address this in a separate bug. There are several solutions that I can think of, including "we'll solve that in the next version", so lets get a separate bug on file and discuss there.

Comment 67

[Andrea Marchesini [:baku]]

> There seems to be some permissions stuff missing here though? Or are you
> planning on putting that in later?

We have a separated patch for the permissions.

Comment 68

[Matthew Gregan [:kinetik]]

Comment on attachment 680564 [details] [diff] [review]
Patch 1 - mozAudioChannel for MediaElement

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

It looks like this is similar to the patch landed in bug 795237, so I think this patch is obsoleted by that bug.  Also note that mozAudioChannel was named mozAudioChannelType in the final version of that patch.

Comment 69

[Matthew Gregan [:kinetik]]

Comment on attachment 683567 [details] [diff] [review]
Patch 3 - mozAudioChannel for MediaElement - IPC (b)

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

This looks fine to me, but I'm not an IPC reviewer.  I'll f+ this, I think an IPC reviewer should do the final r?.

::: dom/audiochannel/AudioChannelServiceChild.cpp
@@ +51,5 @@
> +{
> +}
> +
> +bool
> +AudioChannelServiceChild::GetMuted(AudioChannelType aType, bool aMozHidden)

Can you please rename aMozHidden everywhere it appears in this patch?  The "Moz" prefix isn't necessary.  I think the name would be clearer if it indicated what was hidden, so perhaps something like aInHiddenDocument (or aDocumentVisible, with the logic reversed everywhere)?

Comment 70

[Matthew Gregan [:kinetik]]

Comment on attachment 683992 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (c)

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

Per my earlier comment, please change the relevant bits of mMozAudioChannel to mAudioChannelType per the patch that landed in bug 795237.

r- due to the concerns about media element play state tracking, and "leaking" the play state inside the service.

::: content/base/src/nsDocument.cpp
@@ +9517,5 @@
>                                           NS_LITERAL_STRING("mozvisibilitychange"),
>                                           /* bubbles = */ true,
>                                           /* cancelable = */ false);
> +
> +    EnumerateFreezableElements(NotifyMediaElementVisibilityChanged, nullptr);

Someone on the content team needs to review this part.  I'm surprised that media elements are the first thing that have been wired up to a visibility notification, so there may be an existing way to receive these.

Same with the nsLayoutModule/service changes, they look fine but I'm not the right person to review them.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +185,5 @@
>    // events. This allows us to not perform the copy and thus reduce overhead
>    // in the common case where we don't have a "MozAudioAvailable" listener.
>    void NotifyAudioAvailableListener();
>  
> +  // Called when a "MozVisibilityChanged" event is dispacted by the document.

The comment is true, but what we're concerned about is that the document visibility has changed.  So make the comment something like:

// Called when the document visibility changes.  This allows
// us to pause and resume elements based on their
// AudioChannelType.

@@ +651,5 @@
>  
> +  // This method does the check for muting/unmuting the audio channel.
> +  nsresult UpdateChannelMuteState();
> +
> +  // Mute the audio channel for real

// Set the mute state of the decoder based on mMuted and mChannelMuted.

@@ +919,5 @@
> +  // The audiochannel has been muted
> +  bool mChannelMuted;
> +
> +  // The document is hidden
> +  bool mElementHidden;

// The document that owns this element has been hidden.

And call it mInHiddenDocument or mDocumentVisible (with the logic reversed).

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1894,5 @@
>    UpdatePreloadAction();
>  
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();
> +  if (audioChannelService) {
> +     audioChannelService->MediaElementStarted(mMozAudioChannel);

Looks like the indentation is off here.

Is the Started/Stopped tracking here intended to capture the user/app's intention of which media elements to play?

The element can start playing without Play() being called (look for other instances of mDecoder->Play()).  For example, an element which has autoplay=true will start playing once enough data is available but Play() will never be called.  See other instances of mDecoder->Play() in this file.

How will this work for those?  For example, if there are multiple media elements with autoplay=true, they'll start playing in a random (determined by network load) order.

@@ +3018,5 @@
>    DispatchAsyncEvent(NS_LITERAL_STRING("ended"));
> +
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();
> +  if (audioChannelService) {
> +     audioChannelService->MediaElementStopped(mMozAudioChannel);

Indentation off here too.

Per my comments above, it's also possible that an element will never reach the ended state.  It could hit a decode error, or simple be paused and never resumed, or destroyed while playing.  It seems like the start/stop state can "leak" in the mChannelCounters in the AudioChannelService in all of these cases.

@@ +3791,5 @@
> +void nsHTMLMediaElement::NotifyVisibilityChanged()
> +{
> +  bool mozHidden = false;
> +  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(OwnerDoc());
> +  nsresult rv = domDoc->GetMozHidden(&mozHidden);

I'm pretty sure you can use GetHidden() here, but this is a question for someone who knows the content code.

@@ +3816,5 @@
> +  // We have to mute this channel:
> +  if (mute && !mChannelMuted) {
> +    mChannelMuted = true;
> +    SetMutedInternal();
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptbegin"));

Are these events documented anywhere?  I think they were in the IDL at one point, but they may have been dropped.

::: dom/audiochannel/AudioChannelService.cpp
@@ +133,5 @@
> +  return false;
> +}
> +
> +void
> +AudioChannelService::MediaElementStarted(AudioChannelType aType)

Rather than the media element having to call start/stop and duplicating the element's play state in the service, how about if the elements are registered with the AudioChannelService as they are now, but the service calls back to the element to check the play state?

@@ +183,5 @@
> +  }
> +}
> +
> +bool
> +AudioChannelService::SomethingHigherThan(AudioChannelType aType)

This needs a more descriptive name, please.  Long, but ChannelsActiveWithHigherPriorityThan(), perhaps?

::: dom/audiochannel/AudioChannelService.h
@@ +28,5 @@
> +  static AudioChannelService*
> +  GetAudioChannelService();
> +
> +  /**
> +   * Register/Unregister a document.

s/document/media element/.

Comment 71

[Andrea Marchesini [:baku]]

> Is the Started/Stopped tracking here intended to capture the user/app's
> intention of which media elements to play?

Is there a way to have notifications when the decoder starts/stops playing?

Comment 72

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 683992 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (c)

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

::: content/base/src/nsDocument.cpp
@@ +9517,5 @@
>                                           NS_LITERAL_STRING("mozvisibilitychange"),
>                                           /* bubbles = */ true,
>                                           /* cancelable = */ false);
> +
> +    EnumerateFreezableElements(NotifyMediaElementVisibilityChanged, nullptr);

We already call nsHTMLMediaElement::NotifyOwnerDocumentActivityChanged --- why do we need this NotifyVisibilityChnaged as well?

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We can build whatever notifications you want, if we don't already have them :-).

What exactly do you want to track? For example, if the network stalls and we go into a buffering state, do you want to treat that as "stops playing" or not?

Can you define exactly what you want in terms of the HTML5 media element API?
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#media-elements
E.g. do you just want events when "paused" changes? Or do you want to track the same thing that the HTML5 "playing", "waiting" and "pause" events track? http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#event-media-playing

Comment 74

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 683569 [details] [diff] [review]
patch 4 - permission for mozAudioChannel attribute

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3816,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +nsHTMLMediaElement::AudioChannelPermission(AudioChannelType aType)

Rather than have a separate method (with another branch) why not just move the permission checks into SetMozAudioChannel?

And then you could just do:

  TestPermissionFromPrincipal("audio-channel-" + aString)

::: dom/apps/src/PermissionsInstaller.jsm
@@ +262,5 @@
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION,
> +                             access: [ "content", "notification", "alarm",
> +                                       "telephony", "publicnotification" ]

The access field is supposed to be things like "readonly"... Let's just make individual permissions for each.

Comment 75

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think that we definitely need a new pair of events which are specifically used to indicate "this media element is paused due to a higher-privilege audio channel being used" and "this media element is against free to play due to a higher-privilege audio channel no longer being used".

It would be great if there were also some sort of generic "media element is paused for reasons other than that .pause() was called", but I couldn't find any such event in the spec.

So I think for now we should just fire the first set.


Or am I talking out of my ass and we're talking about something entirely different?

Comment 76

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think there are two sets of notifications being talked about here:
1) notifications from media elements to the AudioChannelService when the media elements start and stop playing
2) notifications from the AudioChannelService to media elements (and then on to the application) when the AudioChannelService needs to start/stop the media elements playing

Comment #75 is about notifications #2.

I think comment #71 is about notifications #1.

Comment 77

[Dietrich Ayala (:dietrich)]

What needs to happen next here?

Comment 78

[Andrea Marchesini [:baku]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #73)
> We can build whatever notifications you want, if we don't already have them
> :-).

1) notifications from media elements to the AudioChannelService when the media elements start and stop playing

I need to have a notification when a media element is actually producing 'audio' and a second one when it stops. I can implement this, but I need an introduction first.

The audio channel service must know the audio channel type of the media elements that are playing so that it can stop/mute/pause the others.

Comment 79

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should define your "playing-through-the-AudioChannel" state as: mReadyState >= HAVE_FUTURE_DATA && nsHTMLMediaElement::IsPotentiallyPlaying().

So I would create a method UpdateAudioChannelPlayingState() which checks if the playing-through-the-AudioChannel state has changed (caching the last state in a bool on the nsHTMLMediaElement). When the state changes, you'd call your notification methods.

Then you'd need to call UpdateAudioChannelPlayingState() after each change to mReadyState (which are all in ChangeReadyState), any change to mPaused, and any change to IsPlaybackEnded. I believe any changes to the latter always go through AddRemoveSelfReference, so you could simply call UpdateAudioChannelPlayingState from there.

Comment 80

[Marco Chen [:mchen]]

Attachment: patch 4 - permission for mozAudioChannel attribute WIPv2

1. Get the permission from Andrea so update the permission patch for further progress.

2. Following the comment and feet the permission format into Jonas' proposal.

Comment 81

[Marco Chen [:mchen]]

Attachment: patch 4 - permission for mozAudioChannel attribute WIPv2

Remove trailer spaces.

Comment 82

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation (d)

Comment 83

[Andrea Marchesini [:baku]]

Attachment: Patch 3 - mozAudioChannel for MediaElement - IPC (c)

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 686560 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (d)

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

It looks to me like you can call MediaElementStarted with one channel type, change the channel type, then call MediaElementStopped with a different channel type, messing up the channel counts. We have to fix that somehow; either prevent changes to the channel type once it has been set, or handle channel type changes. Probably the latter, since we definitely have to handle the case where the channel type changes from the initial value to something else.

We really need tests here.

::: content/base/src/nsDocument.cpp
@@ +9494,5 @@
>                                           NS_LITERAL_STRING("mozvisibilitychange"),
>                                           /* bubbles = */ true,
>                                           /* cancelable = */ false);
> +
> +    EnumerateFreezableElements(NotifyActivityChanged, nullptr);

You do not need to do this, I think. OnPageShow and OnPageHide already call NotifyActivityChanged.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3553,5 @@
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptbegin"));
> +  } else if (!mute && mChannelMuted) {
> +    mChannelMuted = false;
> +    SetMutedInternal(mMuted);
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend"));

We're firing these events even for elements that aren't playing? Is that really what we want?

::: dom/audiochannel/AudioChannelService.h
@@ +51,5 @@
> +  bool ChannelsActiveWithHigherPriorityThan(AudioChannelType aType);
> +
> +  nsAutoPtr<nsTHashtable<nsPtrHashKey<nsHTMLMediaElement> > > mMediaElements;
> +
> +  int32_t mChannelCounters[AUDIO_CHANNEL_PUBLICNOTIFICATION];

I think it might make more sense and be simpler in the end to have AudioChannelService only know about elements that are playing. If we do that, then you dont need mChannelCounters; the number of simultaneously playing elements should be fairly small, and you can just walk the list and ask each element what channel it uses. That simplifies handle channel changes, since then on a channel change you can just reevaluate the status of every playing element and notify them all.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +145,5 @@
>  
> +   // In addition the media element has this new events:
> +   // * onmozinterruptbegin - called when the media element is interrupted
> +   //   because of the audiochannel manager.
> +   // * onmozinterruptend - called when the interruption is concluded

This is misleading. You haven't added the code to make element.onmozinterrupted work. addEventListener has to be used. Put that in the comment.

Comment 85

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 686462 [details] [diff] [review]
patch 4 - permission for mozAudioChannel attribute WIPv2

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +113,5 @@
>  #include "nsChannelPolicy.h"
>  
>  #include "mozilla/Preferences.h"
>  
> +#include "nsIPermission.h"

Nit: This looks unnecessary?

@@ +3468,5 @@
>  
>  NS_IMETHODIMP
>  nsHTMLMediaElement::SetMozAudioChannelType(const nsAString& aString)
>  {
> +  // Only normal chhanel doesn't need permission.

Nit: "chhanel"

@@ +3478,5 @@
> +
> +    nsCOMPtr<nsIPermissionManager> permissionManager =
> +      do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> +    if (!permissionManager)
> +      return NS_ERROR_FAILURE;

Nit: You need braces here.

@@ +3481,5 @@
> +    if (!permissionManager)
> +      return NS_ERROR_FAILURE;
> +
> +    uint32_t perm;
> +    permissionManager->TestPermissionFromPrincipal(subjectPrincipal,

You're neither initializing 'perm' nor checking TestPermissionFromPrincipal for failure. You must do at least one.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +274,5 @@
> + * @param string aPermName
> + * @param string aChannel
> + * @returns string
> + **/
> +this.expandAudioPermission = function expandAudioPermission(aPermName, aChannel) {

I'd really rather we not make this any more complicated than it already is... The 'access' fields are a little special because lots of different permissions can probably use them. This 'channel' field is really specific to just the audio permissions.

Can't you just add entries for 'audio-channel-normal', etc. just like device-storage does now?

@@ +292,5 @@
> +         aPermName + " " + aChannel + "\n");
> +    throw new Error("PermissionsTable.jsm: expandAudioPermissions: Invalid Manifest: " +
> +                    aPermName + " " + aChannel + "\n");
> +  }
> + 

Nit: trailing whitespace

Comment 86

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 686563 [details] [diff] [review]
Patch 3 - mozAudioChannel for MediaElement - IPC (c)

Use a C++ enum for the channel type instead of int32_t.

r=me with that change.  (Let me know if you need help making that change.)

Comment 87

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 686462 [details] [diff] [review]
patch 4 - permission for mozAudioChannel attribute WIPv2

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

I think what you want to do here instead is to modify the expandPermissions function such that if a "channels" property exist, you combine that with the permission name into the appropriate set of permissions names.

Comment 88

[Marco Chen [:mchen]]

Attachment: patch 4 - permission for mozAudioChannel attribute WIPv3

1. Following bent's comments on nsHTMLMediaElement.
2. Following bent's comment and try to write a simple and brief version in PermissionsInstaller.js
3. But in order to follow the proposal from link as below, I didn't directly follow the sample from device-storage. Please help to review this.

https://wiki.mozilla.org/WebAPI/AudioChannels#Security_model

Comment 89

[Marco Chen [:mchen]]

Also add Jonas to feedback list because the review feedback from comment 87.

Comment 90

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 680564 [details] [diff] [review]
Patch 1 - mozAudioChannel for MediaElement

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

The way this is done, the page can't set the audio channel using markup. I.e. something like <audio mozaudiochannel="..."> won't work.

What you should do instead is to create a "mapped attribute" (there's many definitions of this term unfortunately. The one we need here is unrelated to nsMappedAttributes :) ).

What you want here is something similar to the "preload" attribute handled here:
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1953
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#1937

By adding an enum like that we'll automatically parse the channel attribute and store the enum value whenever the attribute is set through markup.

Then do something like
http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLMediaElement.cpp#461

to expose a DOM-exposed getter/setter for the attribute.

You'll want something like
NS_IMPL_ENUM_ATTR_DEFAULT_VALUE(nsHTMLMediaElement, MozAudioChannel, mozaudiochannel, "normal")

You'll also have to add "mozaudiochannel" to nsGkAtomList.h

::: content/html/content/public/nsHTMLMediaElement.h
@@ -559,5 @@
>    /**
>     * Called asynchronously to release a self-reference to this element.
>     */
>    void DoRemoveSelfReference();
> -  

Please remove whitespace cleanup like this from the patch. It makes it much harder to spot what's actually being changed.

If you do want to do whitespace cleanup (because your editor creates them or whatnot), please attach them as separate patches.

Comment 91

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 686937 [details] [diff] [review]
patch 4 - permission for mozAudioChannel attribute WIPv3

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3467,5 @@
>  
>  NS_IMETHODIMP
>  nsHTMLMediaElement::SetMozAudioChannelType(const nsAString& aString)
>  {
> +  // Only normal channel doesn't need permission.

Adding the security checks to SetMozAudioChannelType won't work once we do the changes that I requested to the "part 1" patch.

Instead you'll have to add checks to ParseAttribute. Probably by keeping a local mAudioChannel attribute which is set to the parsed value if and only if the security checks pass.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +449,5 @@
>          }
> +
> +        let candidates = newManifest.permissions[permName].channel;
> +        if ("audio" == permName && candidates) {
> +          let allowChannels = PermissionsTable[permName].channel;

This is still not the right place to add this.

You should add it to the expandPermissions function. I.e. somewhere around here

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/PermissionsInstaller.jsm?rev=80f4c2456dee#335

I.e. add an something like:

else if(aPermName == "audio" && tableEntry.channels) {
  expandedPerms = expandedPerms.concat(tableEntry.channels.map(function(v) { return "audio-channel-" + v; }));
}

I'm not fully sure that the above snippet is right, but you get the general idea.


Or does that to expandPermissions not work?

Comment 92

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I calked with mchen and he felt that we should do the work that's currently in patch one in a separate bug. I.e. we already have a mozAudioChannelType DOM-property and so we can use that for now. But I strongly feel that we should separately hook up a markup HTML attribute in the way described in comment 90. But we can do that separately.

Comment 93

[Marco Chen [:mchen]]

Attachment: patch 4 - permission for mozAudioChannel attribute WIPv4

1. Following Jonas's comments.

2. Implement it into expandPermissions(...).

3. There are four places to call expandPermissions now.
  a. reinstall
  b. install
  c. AllPossiblePermissions
  (The list above are all in PermissionsInstaller.jsm)
  d. askPermission in PermissionPromptHelpter.jsm.
  (But this one is no necessary to be called see bug 816900)

4. Keep the permission checking on setter function only because the property support will be separated to another bug. (Bug 816872)

Comment 94

[Andrea Marchesini [:baku]]

> I think it might make more sense and be simpler in the end to have
> AudioChannelService only know about elements that are playing. If we do
> that, then you dont need mChannelCounters; the number of simultaneously
> playing elements should be fairly small, and you can just walk the list and
> ask each element what channel it uses. That simplifies handle channel
> changes, since then on a channel change you can just reevaluate the status
> of every playing element and notify them all.

Unfortunately this is not so easy. Because the MediaElement can be in separated processes and we don't have/want a synchronous method from the main process to the children.
This is the reason why I implemented this mChannelCounters.

Comment 95

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 687032 [details] [diff] [review]
patch 4 - permission for mozAudioChannel attribute WIPv4

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

r=me with those things changed.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3472,5 @@
> +  if (!aString.EqualsASCII("normal")) {
> +    nsCOMPtr<nsIPrincipal> subjectPrincipal;
> +    nsresult rv = nsContentUtils::GetSecurityManager()->
> +      GetSubjectPrincipal(getter_AddRefs(subjectPrincipal));
> +    NS_ENSURE_SUCCESS(rv, rv);

You can just use NodePrincipal() instead of all this.

@@ +3481,5 @@
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    uint32_t perm = nsIPermissionManager::UNKNOWN_ACTION;
> +    permissionManager->TestPermissionFromPrincipal(subjectPrincipal,

So just change this to

permissionManager->TestPermissionFromPrincipal(NodePrincipal(),

And you should use TestExactPermissionFromPrincipal. I'm sorry, the permissionmanager API is really crappy :(

@@ +3482,5 @@
> +    }
> +
> +    uint32_t perm = nsIPermissionManager::UNKNOWN_ACTION;
> +    permissionManager->TestPermissionFromPrincipal(subjectPrincipal,
> +      NS_ConvertUTF16toUTF8(NS_LITERAL_STRING("audio-channel-") + aString).get(), &perm);

Use nsCString(NS_LITERAL_CSTRING("audio-channel-") + aString).get() instead.

::: dom/apps/src/PermissionsInstaller.jsm
@@ +259,5 @@
>                               certified: ALLOW_ACTION
>                             },
> +                           audio: {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,

I think privileged apps should be able to use audio channels. So change this to ALLOW_ACTION

@@ +261,5 @@
> +                           audio: {
> +                             app: DENY_ACTION,
> +                             privileged: DENY_ACTION,
> +                             certified: ALLOW_ACTION,
> +                             channel: ["normal", "content", "notification",

This should be "channels". This needs to be changed in a number of places in the patch.

Comment 96

[Andrea Marchesini [:baku]]

Attachment: patch 2 - AudioChannelService

Comment 97

[Matthew Gregan [:kinetik]]

Comment on attachment 687045 [details] [diff] [review]
patch 2 - AudioChannelService

I said on the call today that I could review this since roc is offline for a couple of days.

This looks fine, except that RegisterMediaElement/UnregisterMediaElement is still subject to a variant of the potential misuse that roc mentioned at the top of comment 84, i.e. that an element can be registered with one type and unregistered with another, resulting in mismatched counts in mChannelCounters.

We can avoid doing that from the caller, but I'm concerned that it's fragile.  Maybe that's paranoid, but since this is a service it seems like making taking extra care with the API might be worthwhile.  One option is to have the mMediaElements hashtable store a pair of (element, type), so that unregister can retrieve the type from the hashtable rather than requiring the caller to pass it in.

Comment 98

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService (b)

Matthew, thanks for reviewing this patch.
I like your idea to store the audio channel in the hashtable. This makes the code less fragile.

Comment 99

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation (e)

Here the media element patch updated.

Comment 100

[Andrea Marchesini [:baku]]

Comment on attachment 680564 [details] [diff] [review]
Patch 1 - mozAudioChannel for MediaElement

This patch is obsolete.

Comment 101

[Andrea Marchesini [:baku]]

Attachment: Patch 3 - mozAudioChannel for MediaElement - IPC (d)

I don't ask a review for this patch yet. I want to see the first 2 patches reviewed just because the code can change again.

Comment 102

[Marco Chen [:mchen]]

Hi Andrea,

May I ask some points with concerns on patch-1 audio chanel service(b)?

1. In line 78 of patch,
   |if (aType >= AUDIO_CHANNEL_NOTIFICATION && !higher) {|

   a. when we register a notification channel after a existing alarm channel, here didn't call notify(). So notification channel didn't be notified to stop it self.
   b. When we register a content channel after a existing alarm channel, here didn't call notify() too. So content channel didn't be stopped.
   c. Same concern is when two content channels is registerred one by one.

Comment 103

[Andrea Marchesini [:baku]]

> May I ask some points with concerns on patch-1 audio chanel service(b)?

Sure! :)

> 1. In line 78 of patch,
>    |if (aType >= AUDIO_CHANNEL_NOTIFICATION && !higher) {|
>    a. when we register a notification channel after a existing alarm
> channel, here didn't call notify(). So notification channel didn't be
> notified to stop it self.
>    b. When we register a content channel after a existing alarm channel,
> here didn't call notify() too. So content channel didn't be stopped.

In the patch 2 this step is done by the HTMLMediaElement.
We can change the order so that audio channel service calls NotifyAudioChannelStateChanged() immediately.

Comment 104

[Marco Chen [:mchen]]

(In reply to Andrea Marchesini (:baku) from comment #103)
> In the patch 2 this step is done by the HTMLMediaElement.
> We can change the order so that audio channel service calls
> NotifyAudioChannelStateChanged() immediately.

What I mentioned these behaivors doesn't be done in the single media element.
I assumed there are two media elements (even from two process via IPC) to register different channels on case a and b. In this case the order depends on user scenarios not our control.

And may I know your thought on case of c? 

thanks.

Comment 105

[Matthew Gregan [:kinetik]]

Comment on attachment 687076 [details] [diff] [review]
patch 1 - AudioChannelService (b)

Thanks!

Comment 106

[Andrea Marchesini [:baku]]

Before landing this fist patch, I want to see it running on try.

Comment 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 687086 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (e)

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

These event changes need review by smaug

::: content/base/src/nsDocument.cpp
@@ +9494,5 @@
>                                           NS_LITERAL_STRING("mozvisibilitychange"),
>                                           /* bubbles = */ true,
>                                           /* cancelable = */ false);
> +
> +    EnumerateFreezableElements(NotifyActivityChanged, nullptr);

You do not need to do this, I think. OnPageShow and OnPageHide already call NotifyActivityChanged.
(I gave you this comment already)

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3544,5 @@
> +  } else if (!mute && mChannelMuted) {
> +    mChannelMuted = false;
> +    SetMutedInternal(mMuted);
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend"));
> +  }

We're firing these events even for elements that aren't playing? Is that really what we want?
(I gave you this comment already)

Comment 108

[Olli Pettay [:smaug][[e-mail]]]

If this code will land to aurora or beta, remember to #ifdef MOZ_B2G

Comment 109

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 687086 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (e)

>diff --git a/dom/interfaces/core/nsIInlineEventHandlers.idl b/dom/interfaces/core/nsIInlineEventHandlers.idl
>--- a/dom/interfaces/core/nsIInlineEventHandlers.idl
>+++ b/dom/interfaces/core/nsIInlineEventHandlers.idl
>@@ -43,16 +43,18 @@ interface nsIInlineEventHandlers : nsISu
We can't change this interface in beta.
Do you really need element.onmoz* properties and support
for <element onmoz*="event handler code"> attributes?
Couldn't using element.addEventListener("moz*...", listener) be enough?
I know feature testing can be more difficult without oneventname properties :(

If we need this in beta (using #ifdef MOZ_B2G), I'd prefer the absolute minimum
patch.

Anyhow, if we're going to add this stuff non-ifdef'ed to m-c, this needs tests.

Comment 110

[Joe Cheng [:jcheng] (please needinfo)]

How are we with landing Attachment #687076 [details] [diff] patch 1 - AudioChannelService (b)?? Are we ready to land AudioChannelSercice first? Thanks

Comment 111

[Marco Chen [:mchen]]

Attachment: patch 4 - permission for mozAudioChannel attribute Check-In-Version

1. Following Jonas's comment.
2. Add reviewer name.
3. This patch can't be landed now because we need to wait for Gaia updating their manifest first.

Comment 112

[Matthew Gregan [:kinetik]]

(In reply to Marco Chen [:mchen] from comment #102)
> May I ask some points with concerns on patch-1 audio chanel service(b)?
> 
> 1. In line 78 of patch,
>    |if (aType >= AUDIO_CHANNEL_NOTIFICATION && !higher) {|
> 
>    a. when we register a notification channel after a existing alarm
> channel, here didn't call notify(). So notification channel didn't be
> notified to stop it self.
>    b. When we register a content channel after a existing alarm channel,
> here didn't call notify() too. So content channel didn't be stopped.
>    c. Same concern is when two content channels is registerred one by one.

Good catch, I missed this during review.  We need to make sure Notify() is called any time the elements need to re-evaluate their mute/pause/etc. state.  As Marco points out, if an alarm is playing and a notification is registered, higher will be false, so Notify() won't be called--but the Notify() is necessary to ensure the alarm is muted/paused for the duration of the notification.  The simplest fix would be to call Notify() on every register/unregister.

Comment 113

[Andrea Marchesini [:baku]]

> You do not need to do this, I think. OnPageShow and OnPageHide already call
> NotifyActivityChanged.
> (I gave you this comment already)

Sorry. I forgot to reply about this. Removing this, the NotifyActivityChanged() is not called when visibility changes. This is the reason why this line of code is still there.

OnPageShow/Hide are probably not related to the mozvisibilitychanged event?

> We're firing these events even for elements that aren't playing? Is that
> really what we want?
> (I gave you this comment already)

I changed it. Now the media element is registered to the audio channel service only when playing. This patch calls NotifyAudioChannelStateChanged() only in 3 conditions:

1. when the element is registerd to the audio channel and something happens.
2. UpdateAudioChannelPlayingState() and there is the check: if (mPlayingThroughTheAudioChannel) { ...
3. NotifyOwnerDocumentActivityChanged() and there is the check here too.

If we want I can add an assert: MOZ_ASSERT(mPlayingThroughTheAudioChannel).

Comment 114

[Matthew Gregan [:kinetik]]

Comment on attachment 687086 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (e)

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

::: content/base/src/nsDocument.cpp
@@ +9494,5 @@
>                                           NS_LITERAL_STRING("mozvisibilitychange"),
>                                           /* bubbles = */ true,
>                                           /* cancelable = */ false);
> +
> +    EnumerateFreezableElements(NotifyActivityChanged, nullptr);

I discussed this with roc, and we decided this change is necessary after all.  For anyone else following along, the visibility according to the visibility API (on nsIDOMDocument) which this patch is using via GetHidden() can return a different result than IsVisible() on nsDocument, e.g. for a document in a background tab, GetHidden() returns true and IsVisible() also returns true.  The change here ensures the freezable elements receive notifications when the visibility API's state changes.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +3083,5 @@
>        NS_DispatchToMainThread(event);
>      }
>    }
> +
> +  // Let's update the audio channel playing state.

Don't think this comment is necessary.

@@ +3523,5 @@
> +}
> +
> +nsresult nsHTMLMediaElement::UpdateChannelMuteState()
> +{
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();

Move this line down to where it's first used (just before calling GetMuted).

@@ +3525,5 @@
> +nsresult nsHTMLMediaElement::UpdateChannelMuteState()
> +{
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();
> +
> +  bool mozHidden = false;

Remove the moz prefix from the variable name.

@@ +3526,5 @@
> +{
> +  nsRefPtr<AudioChannelService> audioChannelService = AudioChannelService::GetAudioChannelService();
> +
> +  bool mozHidden = false;
> +  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(OwnerDoc());

Null check domDoc?

@@ +3544,5 @@
> +  } else if (!mute && mChannelMuted) {
> +    mChannelMuted = false;
> +    SetMutedInternal(mMuted);
> +    DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend"));
> +  }

Per roc's comment, I think we only want these events if the element was actually interrupted (i.e. it was playing, registered with the audio channel service, and the service requested the interrupt).

@@ +3551,5 @@
> +}
> +
> +void nsHTMLMediaElement::UpdateAudioChannelPlayingState()
> +{
> +  bool state = (mReadyState >= nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA &&

Rather than calling this state, call it playingThroughTheAudioChannel.  This is a convention we use in this file (and some other places) that makes it clear we're comparing the current state of something with the cached state.

::: toolkit/content/widgets/videocontrols.xml
@@ +642,5 @@
>                                  this.controlsSpacer.removeAttribute("hideCursor");
>                              }
>                              break;
> +                        case "mozinterruptbegin":
> +                        case "mozinterruptend":

We'll need to add tests to content/media/test for these events (and for mozAudioChannelType, too), and possibly modify some existing of the existing tests to expect these events.

Comment 115

[Matthew Gregan [:kinetik]]

(In reply to Matthew Gregan [:kinetik] from comment #114)
> Per roc's comment, I think we only want these events if the element was
> actually interrupted (i.e. it was playing, registered with the audio channel
> service, and the service requested the interrupt).

Ignore this comment, splinter review didn't warn me about your new comment when I submitted my review comments.  Since that's addressed, the rest of my comments are minor, so r+ with the changes I mentioned.

Comment 116

[Marco Chen [:mchen]]

Hi Andrea,

We discussed about that to implement nsIDOMHTMLMediaElement2 for avoiding to impact release process on beta and FF18 and keep changes on B2G only.

The Chris Double kindly replied about a concern for this. I forward it to here.

-------
Won't that approach effectively change the interfaces of nsIDOMHTMLVideoElement and nsIDOMHTMLAudioElement since they derive from nsIDOMHTMLMediaElement and would need to change to nsIDOMHTMLMediaElement2 to pick up the new attributes?
-------

Comment 117

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Marco Chen [:mchen] from comment #116 
> We discussed about that to implement nsIDOMHTMLMediaElement2 for avoiding to
> impact release process on beta and FF18 and keep changes on B2G only.
We should not add new nsIDOM* interfaces for this, but nsI*. That is just because
nsIDOM* is magical and such interfaces end up to global scope.

Comment 118

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: Patch 4 - Gaia patch about audio permissions (github pull request 6796)

https://github.com/mozilla-b2g/gaia/pull/6796
Patch proposal:
Add permission to all apps who join audio competing

Note:
This is blocking the landing of bug 805333 gecko part. If gecko is landing without this gaia patch, music/sms/video/camera/alarm will fail to start.(throw exception).

Comment 119

[Faramarz Rashed]

hey guys... can we take care of the reviews and landing this asap please?  this is blocking other high priority bugs!  thanks.

Comment 120

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 688089 [details]
Patch 4 - Gaia patch about audio permissions (github pull request 6796)

r=me

Comment 121

[Andreas Gal :gal]

Who is following up on comment 119? Tim, please find someone in Taipei. Probably best timezone match. We need this landed.

Comment 122

[James Ho]

There has been a daily follow-up meeting on audio related bugs in Taipei since last week. Joe Cheng is tracking this.

Comment 123

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Andreas Gal :gal from comment #121)
> Who is following up on comment 119? Tim, please find someone in Taipei.
> Probably best timezone match. We need this landed.

I believe mchen is doing final testing with #118 's gaia patch. As long as no problem occurs, he will make this land. So landing is coming soon(I strongly hope).

Comment 124

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 687086 [details] [diff] [review]
Patch 2 - mozAudioChannel for MediaElement - implementation (e)

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

Let's land this for now without the nsIInlineEventHandlers changes. I'm working separately on getting an assessment on if we can make an exception for that interface since it's unlikely that someone is using it from C++. Alternatively we can add it after B2G branches from mozilla-beta.

So for now lets land without the nsIInlineEventHandlers changes and file a separate bug.

Comment 125

[Marco Chen [:mchen]]

Attachment: patch 0 - permission for mozAudioChannel attribute Check-In-Version

Add a=blocking-basecamp into commit message.

Comment 126

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think we need the inline event handlers at all.

Comment 127

[Andrea Marchesini [:baku]]

Attachment: patch 6 - everything should be disable ifndef MOZ_B2G

Comment 128

[Marco Chen [:mchen]]

Attachment: Patch 7 - mozAudioChannelType on B2G only

To prevent permission checking and assignment of mozAudioChannelType from getting error on non B2G build.

Comment 129

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService (c)

rebased on top of patch 0

Comment 130

[Andrea Marchesini [:baku]]

Attachment: Patch 2 - mozAudioChannel for MediaElement - implementation (f)

rebased on top of patch 0

Comment 131

[Andrea Marchesini [:baku]]

Attachment: Patch 5 - mozAudioChannel for MediaElement - IPC (e)

rebased + shutdown procedure.

Comment 132

[Andrea Marchesini [:baku]]

Attachment: patch 3 - everything should be disable ifndef MOZ_B2G

all of these patches have been rebased on top of patch 0 and Bug 817043

Comment 133

[Andrea Marchesini [:baku]]

Patches number 1, 2, 3 and 5 are based on patch for Bug 817043.

Comment 134

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=f9d638931e62 green on try.

Comment 135

[Andrea Marchesini [:baku]]

Attachment: patch 1 - AudioChannelService (d)

Just move where the hashtable is initialized. Now we do this in the constructor.

Comment 136

[Andrea Marchesini [:baku]]

Attachment: Patch 5 - mozAudioChannel for MediaElement - IPC (f)

Comment 137

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed patches 0, 1, 2 and 3.

https://hg.mozilla.org/integration/mozilla-inbound/rev/721ba8214185
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff2a302cebda
https://hg.mozilla.org/integration/mozilla-inbound/rev/3769c967ce4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/974411e8e567

Comment 138

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Interdiff for patch 5 (MediaElement IPC) from version c->f

Comment 139

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 688420 [details] [diff] [review]
Patch 5 - mozAudioChannel for MediaElement - IPC (f)

Please use a C++ enum instead of uint32_t.

Comment 140

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/721ba8214185
https://hg.mozilla.org/mozilla-central/rev/ff2a302cebda
https://hg.mozilla.org/mozilla-central/rev/3769c967ce4b
https://hg.mozilla.org/mozilla-central/rev/974411e8e567

Comment 141

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Landed 0-3 on beta branch:

https://hg.mozilla.org/releases/mozilla-beta/rev/3cb188687c1b
https://hg.mozilla.org/releases/mozilla-beta/rev/def98df7720d
https://hg.mozilla.org/releases/mozilla-beta/rev/0b3d4bed6057
https://hg.mozilla.org/releases/mozilla-beta/rev/4f1bd4d305c3

Comment 142

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Andrea, once you have addressed the review comment from cjones for the IPC patch, could you land it on mozilla-inbound so that we can reuse that IPC infrastructure for bug 815445

Comment 143

[Marco Chen [:mchen]]

Comment on attachment 688420 [details] [diff] [review]
Patch 5 - mozAudioChannel for MediaElement - IPC (f)

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

::: dom/audiochannel/AudioChannelServiceChild.cpp
@@ +77,5 @@
> +AudioChannelServiceChild::RegisterMediaElement(nsHTMLMediaElement* aMediaElement,
> +                                               AudioChannelType aType)
> +{
> +  AudioChannelService::RegisterMediaElement(aMediaElement, aType);
> +

May I know that here should add the code like "mMediaElements.put(aMediaElement, aType)"?

Comment 144

[Andrea Marchesini [:baku]]

> > +  AudioChannelService::RegisterMediaElement(aMediaElement, aType);
> > +
> 
> May I know that here should add the code like
> "mMediaElements.put(aMediaElement, aType)"?

This is done by AudioChannelService::RegisterMediaElement

Comment 145

[Andrea Marchesini [:baku]]

(In reply to Jonas Sicking (:sicking) from comment #142)
> Andrea, once you have addressed the review comment from cjones for the IPC
> patch, could you land it on mozilla-inbound so that we can reuse that IPC
> infrastructure for bug 815445

I tried but I'm having problem using the enum with IPDL. cjones can you tell me more?

Comment 147

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

What's the problem?

Comment 148

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Note, I'm also fine with addressing that comment in a followup.

Comment 149

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I updated the patch but hit bug 818753 when trying to test it.

Comment 150

[Ryan VanderMeulen [:RyanVM]]

Can you please land this on Aurora too?

Comment 151

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Part 5 with review comment addressed

Comment 152

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/mozilla-central/rev/74987fe38fd2

Comment 153

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/releases/mozilla-beta/rev/d17808170910

Comment 154

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Ryan VanderMeulen from comment #150)
> Can you please land this on Aurora too?

Jonas, do you mind doing the honors?  I don't trust myself to get all the pieces put together properly.

Comment 155

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Cursed inconsistent -Werror

https://hg.mozilla.org/mozilla-central/rev/3c81e1c0d252
https://hg.mozilla.org/releases/mozilla-beta/rev/657afc996643

Comment 156

[[:fabrice] Fabrice Desré]

Why is this patch on central and beta but not on aurora? This makes landing other patches touching the same files quite a mess.

Comment 157

[Gene Lian (I already quit Mozilla)]

Sorry for bothering but we need to confirm comment 156 fast. Lots of PermissionsInstaller.jsm changes/issues are waiting/coming recently. If central/beta/aurora are not sync'ed cleanly, we'd get trouble to manually rebase patches every time in the future.

Btw, just wondering do we have any mechanism to ask synchronization for specific file(s) which is a must-to-sync one? Relying on bb+/non-bb+ sometimes breaks the sync because it's feature dependent.

Comment 158

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm working on syncing to aurora right now, but I'm running into bugs with mercurial which is slowing me down.

Comment 159

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09f16c3f088d
https://hg.mozilla.org/releases/mozilla-aurora/rev/680aa1bc9be6
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3af243701cc
https://hg.mozilla.org/releases/mozilla-aurora/rev/1ed75bdbc62c
https://hg.mozilla.org/releases/mozilla-aurora/rev/4dc82515e37d

Comment 160

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #158)
> I'm working on syncing to aurora right now, but I'm running into bugs with
> mercurial which is slowing me down.

Thanks Jonas for all the hard work!

Comment 161

[Gene Lian (I already quit Mozilla)]

Comment on attachment 688131 [details] [diff] [review]
patch 0 - permission for mozAudioChannel attribute Check-In-Version

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

::: dom/apps/src/PermissionsInstaller.jsm
@@ +344,5 @@
>    if (PermissionsTable[permName].access) {
>      AllPossiblePermissions =
>        AllPossiblePermissions.concat(expandPermissions(permName, READWRITE));
> +  } else if (PermissionsTable[permName].channels) {
> +      AllPossiblePermissions.concat(expandPermissions(permName, null, PermissionsTable[permName].channels));

I'm afraid this is wrong.

AllPossiblePermissions = AllPossiblePermissions.concat();

Fire Bug 819264.

Comment 162

[vend_jinzhu.yang]

hi，matthew，this modify part willl cause music play be paused when set to backgroud，
is this correct？(In reply to Matthew Gregan [:kinetik] from comment #114)
> Comment on attachment 687086 [details] [diff] [review]
Patch 2 -
> mozAudioChannel for MediaElement - implementation (e)

Review of attachment
> 687086 [details] [diff] [review]:
> -----------------------------------------------------------------

:::
> content/base/src/nsDocument.cpp
@@ +9494,5 @@
>                             
> NS_LITERAL_STRING("mozvisibilitychange"),
>                                 
> /* bubbles = */ true,
>                                           /*
> cancelable = */ false);
> +
> +   
> EnumerateFreezableElements(NotifyActivityChanged, nullptr);

I discussed
> this with roc, and we decided this change is necessary after all.  For
>

Comment 163

[Marco Chen [:mchen]]

(In reply to vend_jinzhu.yang from comment #162)
> hi，matthew，this modify part willl cause music play be paused when set to
> backgroud，
> is this correct？

The media element just sent visibility to audio channel service for asking whether it should be paused or not. And only mozaudiochannel = normal will be paused when it is in background.

On the other way, you can assign channel to 'content'. Then it can continue to play in the background.

Comment 164

[Deepa Subramanian]

The user continues to hear FM radio right after a call. 

Issue found on
Inari Build ID: 20130515070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/d06cfe7d67c2
Gaia: 0ddb515f15cbc6b74fc2742b7599d6ae74c6413f

Other details:
UCID user-sound-039: As a user when I finish my phone call, I will have to manually start the radio again to resume listening.

This fails test case :https://moztrap.mozilla.org/runtests/run/1301/env/315/?pagenumber=1&pagesize=20&sortfield=order&sortdirection=asc&filter-id=5967

Comment 165

[Marco Chen [:mchen]]

(In reply to Deepa Subramanian from comment #164)
> Other details:
> UCID user-sound-039: As a user when I finish my phone call, I will have to
> manually start the radio again to resume listening.

Hi Deepa,

The user can continue to hear FM Radio right after a call is as design now. May I know any bug related to this behavior so we can discuss with UX then make a decision? Thanks.

Comment 166

[Leo]

Hi Marco Chen and pzhang,

I have a scenario with respect to FM radio.
1. Play FM
2. Open Music App and start playing Music
3. FM automatically pauses and Music starts playing.
4. Now, when Music is manually stopped or paused.

What should be the expected behaviour?
1. FM should automatically start playing or 
2. User has to manually play it.

Thanks,

Comment 167

[Marco Chen [:mchen]]

(In reply to Leo from comment #166)

Hi Leo,

The current design is "FM will be automatically resumed after it is back to foreground".
From technical view, if a content channel is paused by another content channel then it will be automatically resumed when it is on the foreground. (because policy allowed foreground app to play always)

If a content channel is paused by another higher priority channel then it will be resumed after higher one is stopped.