Need a Policy & Mechanism for Audio Competing & Control

RESOLVED FIXED in Firefox 18

Status

defect
P1
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mchen, Assigned: baku)

Tracking

({feature})

unspecified
B2G C2 (20nov-10dec)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

Details

(Whiteboard: inarirun2)

Attachments

(8 attachments, 27 obsolete attachments)

86 bytes, text/html
timdream
: review+
Details
7.48 KB, patch
mchen
: review+
airpingu
: review-
Details | Diff | Splinter Review
15.27 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.35 KB, patch
sicking
: review+
Details | Diff | Splinter Review
16.55 KB, patch
baku
: review+
Details | Diff | Splinter Review
18.67 KB, patch
cjones
: review+
Details | Diff | Splinter Review
10.12 KB, patch
Details | Diff | Splinter Review
21.11 KB, patch
Details | Diff | Splinter Review
Reporter

Description

7 years ago
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
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.
Reporter

Updated

7 years ago
Blocks: 806054
Summary: Need a Policy & Mechansim for Audio Competing & Control → Need a Policy & Mechanism for Audio Competing & Control
Reporter

Updated

7 years ago
Blocks: 806361
Reporter

Updated

7 years ago
Blocks: 796628
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.
(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.
(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?
Reporter

Updated

7 years ago
Blocks: 796333
Reporter

Comment 5

7 years ago
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.
blocking-basecamp: --- → ?
IMHO, an app playing audio need to get higher oom_adj than normal background apps.
Otherwise, the app could easily killed when oom.
> 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.
Reporter

Comment 8

7 years ago
(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.
Reporter

Comment 9

7 years ago
(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.
> > 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.
(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
> > 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.
> 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.
(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?
Reporter

Comment 15

7 years ago
(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.
Reporter

Comment 16

7 years ago
(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.
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.
blocking-basecamp: ? → +
Keywords: feature
Priority: -- → P1

Updated

7 years ago
Blocks: 809106
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.
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.
Reporter

Comment 20

7 years ago
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.
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.
Reporter

Comment 22

7 years ago
(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?
(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?
Reporter

Comment 24

7 years ago
(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.
(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.
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.
Reporter

Comment 27

7 years ago
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.
Flags: needinfo?(jonas)
(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.
Flags: needinfo?(jonas)
Assignee

Comment 29

7 years ago
Posted patch feedback needed (obsolete) — Splinter Review
> 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 :)
Attachment #680535 - Flags: feedback?
Assignee

Comment 30

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

Comment 31

7 years ago
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 (?)
Attachment #680554 - Flags: review?(mchen)
Assignee

Comment 32

7 years ago
Attachment #680554 - Attachment is obsolete: true
Attachment #680554 - Flags: review?(mchen)
Attachment #680555 - Flags: review?(mchen)
Assignee

Comment 33

7 years ago
UUID for nsHTMLMediaElement.idl
Attachment #680555 - Attachment is obsolete: true
Attachment #680555 - Flags: review?(mchen)
Attachment #680557 - Flags: review?(mchen)
Assignee

Comment 34

7 years ago
1. muted -> paused
2. ringer removed
Attachment #680557 - Attachment is obsolete: true
Attachment #680557 - Flags: review?(mchen)
Attachment #680562 - Flags: review?(mchen)
Assignee

Comment 35

7 years ago
just other detail.
Attachment #680562 - Attachment is obsolete: true
Attachment #680562 - Flags: review?(mchen)
Attachment #680564 - Flags: review?(mchen)
Reporter

Comment 36

7 years ago
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.
Attachment #680564 - Flags: review?(mchen) → feedback+
Reporter

Comment 37

7 years ago
Discussd with Baku about re-assign owner to him.
Assignee: mchen → amarchesini
Assignee

Comment 38

7 years ago
This patch is still using the 'mute' approach. I need info for implementing the pause in nsHTMLMediaElement.
Attachment #680535 - Attachment is obsolete: true
Attachment #680535 - Flags: feedback?
Attachment #681469 - Flags: feedback?(jonas)
Assignee

Comment 39

7 years ago
This is IPC support for AudioChannelService.
Attachment #681470 - Flags: feedback?(jonas)
Assignee

Comment 40

7 years ago
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).
Reporter

Updated

7 years ago
Depends on: 795237
Reporter

Comment 41

7 years ago
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.
Assignee

Comment 42

7 years ago
> 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 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.
Attachment #681469 - Flags: feedback?(jonas) → feedback?(roc)
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
Attachment #681469 - Flags: feedback?(roc) → feedback?(kinetik)
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 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.
Attachment #681470 - Flags: feedback?(jonas) → feedback+
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
Attachment #681469 - Flags: feedback?(kinetik) → feedback+
Assignee

Updated

7 years ago
Attachment #680564 - Flags: review?(jonas)
Assignee

Comment 48

7 years ago
> 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.
Assignee

Comment 49

7 years ago
Attachment #681469 - Attachment is obsolete: true
Attachment #683561 - Flags: review?(jonas)
Assignee

Comment 50

7 years ago
Attachment #681470 - Attachment is obsolete: true
Attachment #683567 - Flags: review?(jonas)
Assignee

Comment 51

7 years ago
This patch must be extended in order to implement what Jonas suggested in the comment 46.
Attachment #683569 - Flags: feedback?(bent.mozilla)
Reporter

Comment 52

7 years ago
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.
Reporter

Comment 53

7 years ago
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?
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'.
Assignee

Comment 55

7 years ago
mozpaused/mozunpaused renamed to mozinterruptbegin/mozinterruptend
Attachment #683561 - Attachment is obsolete: true
Attachment #683561 - Flags: review?(jonas)
Attachment #683992 - Flags: review?(jonas)
Marking for C2, given this meets the criteria of known P1/P2 blocking-basecamp+ bugs at the end of C1.
Target Milestone: --- → B2G C2 (20nov-10dec)
Reporter

Comment 57

7 years ago
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.
Assignee

Comment 58

7 years ago
Good point. I'll add this an a separated patch.
Reporter

Comment 59

7 years ago
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?
(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?
I created bug 815046 to trace comment #58.

Updated

7 years ago
Blocks: 815046
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.
Attachment #680564 - Flags: review?(jonas) → review?(roc)
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.
Attachment #683567 - Flags: review?(jonas) → review?(roc)
Reporter

Comment 64

7 years ago
(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 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?
Attachment #683992 - Flags: review?(jonas) → review?(roc)
(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.
Reporter

Updated

7 years ago
Blocks: 815069
Assignee

Comment 67

7 years ago
> 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.
Attachment #680564 - Flags: review?(roc) → review?(kinetik)
Attachment #683567 - Flags: review?(roc) → review?(kinetik)
Attachment #683992 - Flags: review?(roc) → review?(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.
Attachment #680564 - Flags: review?(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)?
Attachment #683567 - Flags: review?(kinetik) → feedback+
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/.
Attachment #683992 - Flags: review?(kinetik) → review-
Reporter

Updated

7 years ago
Blocks: 815569
Assignee

Comment 71

7 years ago
> 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 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?
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 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.
Attachment #683569 - Flags: feedback?(bent.mozilla) → feedback-
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?
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.
No longer blocks: 815326
What needs to happen next here?
Assignee

Comment 78

7 years ago
(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.
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.
Reporter

Comment 80

7 years ago
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.
Attachment #683569 - Attachment is obsolete: true
Attachment #686461 - Flags: feedback?(bent.mozilla)
Reporter

Comment 81

7 years ago
Remove trailer spaces.
Attachment #686461 - Attachment is obsolete: true
Attachment #686461 - Flags: feedback?(bent.mozilla)
Attachment #686462 - Flags: feedback?(bent.mozilla)
Reporter

Updated

7 years ago
Attachment #683567 - Flags: review?(jones.chris.g)
Assignee

Comment 82

7 years ago
Attachment #683992 - Attachment is obsolete: true
Attachment #686560 - Flags: review?(roc)
Assignee

Comment 83

7 years ago
Attachment #683567 - Attachment is obsolete: true
Attachment #683567 - Flags: review?(jones.chris.g)
Attachment #686563 - Flags: review?(jones.chris.g)
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 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
Attachment #686462 - Flags: feedback?(bent.mozilla) → feedback-
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.)
Attachment #686563 - Flags: review?(jones.chris.g) → review+
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.
Reporter

Comment 88

7 years ago
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
Attachment #686462 - Attachment is obsolete: true
Attachment #686937 - Flags: feedback?(jonas)
Attachment #686937 - Flags: feedback?(bent.mozilla)
Reporter

Comment 89

7 years ago
Also add Jonas to feedback list because the review feedback from comment 87.
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.
Attachment #680564 - Flags: review-
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?
Attachment #686937 - Flags: review-
Attachment #686937 - Flags: feedback?(jonas)
Attachment #686937 - Flags: feedback?(bent.mozilla)
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.
Reporter

Comment 93

7 years ago
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)
Attachment #686937 - Attachment is obsolete: true
Attachment #687032 - Flags: review?(jonas)
Assignee

Comment 94

7 years ago
> 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 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.
Attachment #687032 - Flags: review?(jonas) → review+
Assignee

Comment 96

7 years ago
Posted patch patch 2 - AudioChannelService (obsolete) — Splinter Review
Attachment #686560 - Attachment is obsolete: true
Attachment #686560 - Flags: review?(roc)
Attachment #687045 - Flags: review?(roc)
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.
Attachment #687045 - Flags: review?(roc) → review?(kinetik)
Assignee

Comment 98

7 years ago
Matthew, thanks for reviewing this patch.
I like your idea to store the audio channel in the hashtable. This makes the code less fragile.
Attachment #687045 - Attachment is obsolete: true
Attachment #687045 - Flags: review?(kinetik)
Attachment #687076 - Flags: review?(kinetik)
Assignee

Comment 99

7 years ago
Here the media element patch updated.
Attachment #687086 - Flags: review?(kinetik)
Assignee

Comment 100

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

This patch is obsolete.
Attachment #680564 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #687076 - Attachment description: patch 2 - AudioChannelService (b) → patch 1 - AudioChannelService (b)
Assignee

Updated

7 years ago
Attachment #687086 - Attachment description: Patch 2b - mozAudioChannel for MediaElement - implementation (e) → Patch 2 - mozAudioChannel for MediaElement - implementation (e)
Assignee

Comment 101

7 years ago
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.
Attachment #686563 - Attachment is obsolete: true
Reporter

Comment 102

7 years ago
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.
Assignee

Comment 103

7 years ago
> 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.
Reporter

Comment 104

7 years ago
(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 on attachment 687076 [details] [diff] [review]
patch 1 - AudioChannelService (b)

Thanks!
Attachment #687076 - Flags: review?(kinetik) → review+
Assignee

Comment 106

7 years ago
Before landing this fist patch, I want to see it running on try.
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)
Attachment #687086 - Flags: review?(bugs)
If this code will land to aurora or beta, remember to #ifdef MOZ_B2G
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.
Attachment #687086 - Flags: review?(bugs) → review-
How are we with landing Attachment #687076 [details] [diff] patch 1 - AudioChannelService (b)?? Are we ready to land AudioChannelSercice first? Thanks
Reporter

Comment 111

7 years ago
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.
Attachment #687032 - Attachment is obsolete: true
Attachment #687676 - Flags: review+
(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.
Assignee

Comment 113

7 years ago
> 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 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.
(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.
Reporter

Comment 116

7 years ago
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?
-------
(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.
Assignee

Updated

7 years ago
Depends on: 817589
Attachment #687086 - Flags: review?(kinetik) → review+
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).
Attachment #688089 - Flags: review?(ehung)

Comment 119

7 years ago
hey guys... can we take care of the reviews and landing this asap please?  this is blocking other high priority bugs!  thanks.
Comment on attachment 688089 [details]
Patch 4 - Gaia patch about audio permissions (github pull request 6796)

r=me
Attachment #688089 - Flags: review?(ehung) → review+
Who is following up on comment 119? Tim, please find someone in Taipei. Probably best timezone match. We need this landed.

Comment 122

7 years ago
There has been a daily follow-up meeting on audio related bugs in Taipei since last week. Joe Cheng is tracking this.
(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 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.
Reporter

Comment 125

7 years ago
Add a=blocking-basecamp into commit message.
Attachment #687676 - Attachment is obsolete: true
Attachment #688131 - Flags: review+
I don't think we need the inline event handlers at all.
Reporter

Updated

7 years ago
Attachment #688218 - Flags: feedback?(mchen) → feedback+
Reporter

Comment 128

7 years ago
To prevent permission checking and assignment of mozAudioChannelType from getting error on non B2G build.
Attachment #688224 - Flags: review?(jonas)
Assignee

Updated

7 years ago
Attachment #688131 - Attachment description: patch 4 - permission for mozAudioChannel attribute Check-In-Version → patch 0 - permission for mozAudioChannel attribute Check-In-Version
Assignee

Comment 129

7 years ago
rebased on top of patch 0
Attachment #687076 - Attachment is obsolete: true
Attachment #688251 - Flags: review+
Assignee

Comment 130

7 years ago
rebased on top of patch 0
Attachment #687086 - Attachment is obsolete: true
Attachment #688252 - Flags: review+
Assignee

Comment 131

7 years ago
rebased + shutdown procedure.
Attachment #687093 - Attachment is obsolete: true
Attachment #688259 - Flags: review?(jones.chris.g)
Assignee

Updated

7 years ago
Attachment #688089 - Attachment description: Patch 5 - Gaia patch about audio permissions (github pull request 6796) → Patch 4 - Gaia patch about audio permissions (github pull request 6796)
Assignee

Comment 132

7 years ago
all of these patches have been rebased on top of patch 0 and Bug 817043
Attachment #688218 - Attachment is obsolete: true
Attachment #688224 - Attachment is obsolete: true
Attachment #688224 - Flags: review?(jonas)
Attachment #688261 - Flags: review?(jonas)
Assignee

Updated

7 years ago
Depends on: 817043
Assignee

Comment 133

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

Updated

7 years ago
Attachment #688259 - Attachment description: Patch 3 - mozAudioChannel for MediaElement - IPC (e) → Patch 5 - mozAudioChannel for MediaElement - IPC (e)
Assignee

Updated

7 years ago
Attachment #688261 - Attachment description: patch 5 - everything should be disable ifndef MOZ_B2G → patch 3 - everything should be disable ifndef MOZ_B2G
Assignee

Comment 135

7 years ago
Just move where the hashtable is initialized. Now we do this in the constructor.
Attachment #688356 - Flags: review+
Assignee

Updated

7 years ago
Attachment #688251 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Blocks: 818220
Assignee

Comment 136

7 years ago
Attachment #688259 - Attachment is obsolete: true
Attachment #688259 - Flags: review?(jones.chris.g)
Attachment #688420 - Flags: review?(jones.chris.g)
Comment on attachment 688420 [details] [diff] [review]
Patch 5 - mozAudioChannel for MediaElement - IPC (f)

Please use a C++ enum instead of uint32_t.
Attachment #688420 - Flags: review?(jones.chris.g) → review+
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
Reporter

Comment 143

7 years ago
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)"?
Assignee

Comment 144

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

This is done by AudioChannelService::RegisterMediaElement
Assignee

Comment 145

7 years ago
(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?
Duplicate of this bug: 763925
Note, I'm also fine with addressing that comment in a followup.
I updated the patch but hit bug 818753 when trying to test it.
Can you please land this on Aurora too?
https://hg.mozilla.org/mozilla-central/rev/74987fe38fd2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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.
Whiteboard: [leave open] → [checkin-needed:aurora]
Why is this patch on central and beta but not on aurora? This makes landing other patches touching the same files quite a mess.
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.
Flags: needinfo?(jonas)
I'm working on syncing to aurora right now, but I'm running into bugs with mercurial which is slowing me down.
Flags: needinfo?(jonas)
(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!
Whiteboard: [checkin-needed:aurora]
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.
Attachment #688131 - Flags: review-
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
>
Reporter

Comment 163

7 years ago
(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.
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
Whiteboard: inarirun2
Reporter

Comment 165

6 years ago
(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

6 years ago
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,
Flags: needinfo?(pzhang)
Flags: needinfo?(mchen)
Reporter

Comment 167

6 years ago
(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.
Flags: needinfo?(mchen)
Flags: needinfo?(pzhang)
You need to log in before you can comment on or make changes to this bug.