First WIP. With this we expose the API to browserElement interface. The next patch will change how AudioChannelAgent works, in order to manage the appId of the app where this object is running into. This second patch disables the current AudioChannel policy completely. I hope to have something ready for the next week or the next-next one.

Looking good!, Although I have some work similar as yours. If this work can speed up the API progress for audio channel, please help it and I will help for the b2g integrating part with gaia guys. BTW, I have some option for that. 1. The BrowserElementAudioChannel looks like want to retrieve audiochannels status in browser element, the allowedAudioChannels naming may make me confuse. Maybeb we can have another name for that? 2. Should we change the API in BrowserElementAudioChannel from DOMRequest to promise?

This patch removes the existing AudioChannel logic from gecko introducing a better way to manage AudioChannelAgent.

This second patch exposes some IDL methods.

I'm almost done with the last 2 patches: 1. using the IDL methods in the BrowserElement API. 2. testing. They should be ready for this week. Before Christmas.

Changed many things.

wrong patch.

Comment on attachment 8540239 [details] [diff] [review] patch 3 - Audio in BrowserElement API Hi Baku, I'm not the mozBrowser reviewer, Transfer to kanru.

Comment on attachment 8540239 [details] [diff] [review] patch 3 - Audio in BrowserElement API Review of attachment 8540239 [details] [diff] [review]: ----------------------------------------------------------------- The newly added webidl needs a DOM peers review. r+ with that and comments addressed. ::: dom/browser-element/BrowserElementAudioChannel.h @@ +13,5 @@ > +#include "nsIBrowserElementAPI.h" > +#include "nsISupports.h" > +#include "nsWrapperCache.h" > + > +class nsIBrowserElementAPI; This and the include above doesn't seem to be used. ::: dom/html/nsBrowserElement.cpp @@ +13,5 @@ > #include "mozilla/dom/DOMRequest.h" > #include "mozilla/dom/ScriptSettings.h" > #include "mozilla/dom/ToJSValue.h" > > +#include "AudioChannelService.h" included twice. @@ +152,5 @@ > } > + > + uint64_t childID; > + rv = frameLoader->GetChildID(&childID); > + NS_ENSURE_SUCCESS_VOID(rv); Not sure what the childID is for but note that childID is per-process while many Apps(TabParents) could share one process. @@ +572,5 @@ > + > + // If empty, it means that this is the first call of this method. > + if (mBrowserElementAudioChannels.IsEmpty()) { > + nsCOMPtr<nsIFrameLoader> frameLoader = GetFrameLoader(); > + NS_ENSURE_TRUE_VOID(frameLoader); Throw?

Here the comment applied plus the 'notification' event. About childID, any iframe can run 1 single app, and that has 1 single childID. Correct?

Here some tests.

Comment on attachment 8540581 [details] [diff] [review] patch 4 - tests Review of attachment 8540581 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: dom/browser-element/mochitest/browserElement_AudioChannel.js @@ +1,4 @@ > +/* Any copyright is dedicated to the public domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +// Bug 757859 - Test the getContentDimensions functionality of mozbrowser // Bug 1113086 - tests for AudioChannel API into BrowserElement @@ +72,5 @@ > + return; > + } > + > + if (!running) { > + info("it was npt running, and now: " + ac.isActive); s/npt/not/

On FxOS platform, we also need to change: dom/system/gonk/AudioManager.cpp dom/system/gonk/AudioChannelManager.cpp dom/fmradio/FMRadio.cpp dom/camera/DOMCameraControl.cpp BTW, we should find another way to hook telephony channel, The original place to active telephony channel is on b2g process without window object.

Hi Andrea, I would like to suggest Randy to be the reviewer of audio channel mechanism because 1. I haven't followed the audio channel a long time. 2. Randy will be the one focused on audio channel for FxOS project. Still think audio channel is a great work for Firefox and FxOS.

This review needs to wait on bug 940273 being reviewed first. I may not get to it this week...

> dom/system/gonk/AudioManager.cpp > dom/system/gonk/AudioChannelManager.cpp I'm not familiar with these 2 components. Do you mind to take care of it? At the moment, my patches do not send audio.volume.* events. I think we should talk about if these are still needed and how to replace them with something else in case. > dom/fmradio/FMRadio.cpp This is done. > dom/camera/DOMCameraControl.cpp Separated patch for this. > BTW, we should find another way to hook telephony channel, > The original place to active telephony channel is on b2g process without > window object. With the new API we don't need a particular hook for telephony. Correct? I didn't touch AudioChannelAgent.

Here the gonk/audio*Manager, camera and fmradio patch for b2g.

It seems to me that more or less gecko side is done. System app has to implement the new logic.

I had a discuss with alive and he suggested that the audio channel scope should be in iframe instead of application. He wants to deal with the apps with multi-frame/multi-audiochannel problem in system app. Hi Alive, Could you describe it in detail?

(In reply to Andrea Marchesini (:baku) from comment #18) > > dom/system/gonk/AudioManager.cpp > > dom/system/gonk/AudioChannelManager.cpp > > I'm not familiar with these 2 components. Do you mind to take care of it? > At the moment, my patches do not send audio.volume.* events. I think we > should talk about if these are still needed and how to replace them with > something else in case. > > > dom/fmradio/FMRadio.cpp > > This is done. > > > dom/camera/DOMCameraControl.cpp > > Separated patch for this. > > > BTW, we should find another way to hook telephony channel, > > The original place to active telephony channel is on b2g process without > > window object. > > With the new API we don't need a particular hook for telephony. Correct? > I didn't touch AudioChannelAgent. Hi Baku, Thanks for your hard working for this topic. Right, we will take the responsibility on b2g platform and I'm doing the testing on flame right now. The review would slower and gaia guys also need to write the competition logic in system apps. For the telephony part in GSM call now, it would initialize the audiochannelAgent in b2g process, Could the system app control/get notification via kind of channel?

(In reply to Randy Lin [:rlin] from comment #21) > I had a discuss with alive and he suggested that the audio channel scope > should be in iframe instead of application. He wants to deal with the apps > with multi-frame/multi-audiochannel problem in system app. > Hi Alive, > Could you describe it in detail? In case of music app, when there is a music app playing at either foreground or background, the user may be able to use the utility tray to open another music activity from the download/transfer notification. After that we will have index.html and open.html of music app running at the same time. We cannot just mute one of them if you don't split the channel info of them into their own frame. Now we are using a hack in gaia - use localstorage to communicate between the two frames from the same app. We need to fix this from API design.

Comment on attachment 8540579 [details] [diff] [review] patch 3 - Audio in BrowserElement API Review of attachment 8540579 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementAudioChannel.cpp @@ +130,5 @@ > +bool > +BrowserElementAudioChannel::GetMuted(ErrorResult& aRv) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + AssertIsInMainProcess(); Does those function calls (muted/volume/...) must invoked in main process(b2g)?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #23) > (In reply to Randy Lin [:rlin] from comment #21) > > I had a discuss with alive and he suggested that the audio channel scope > > should be in iframe instead of application. He wants to deal with the apps > > with multi-frame/multi-audiochannel problem in system app. > > Hi Alive, > > Could you describe it in detail? > > In case of music app, when there is a music app playing at either foreground > or background, > the user may be able to use the utility tray to open another music activity > from the download/transfer notification. After that we will have index.html > and open.html of music app running at the same time. > > We cannot just mute one of them if you don't split the channel info of them > into their own frame. > > Now we are using a hack in gaia - use localstorage to communicate between > the two frames from the same app. We need to fix this from API design. For the telephony used by GSM call, which iframe object should we provided for system to control the telephony channel? Maybe we could have an dummy media element to register audiochannel for this kind of application to use...

(In reply to Randy Lin [:rlin] from comment #24) > Comment on attachment 8540579 [details] [diff] [review] > patch 3 - Audio in BrowserElement API > > Review of attachment 8540579 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementAudioChannel.cpp > @@ +130,5 @@ > > +bool > > +BrowserElementAudioChannel::GetMuted(ErrorResult& aRv) > > +{ > > + MOZ_ASSERT(NS_IsMainThread()); > > + AssertIsInMainProcess(); > > Does those function calls (muted/volume/...) must invoked in main > process(b2g)? Yes. BrowserElement API is used by system app running on the main-process. Right? If this is not true, we can change this.

> We cannot just mute one of them if you don't split the channel info of them > into their own frame. I need more info about this. Jonas, the current patch uses the childID to mute/stop/whatever the Media Elements. This chidID is taken from the frameLoader. Any idea about how to improve this to cover what Alive wants?

(In reply to Andrea Marchesini (:baku) from comment #26) > (In reply to Randy Lin [:rlin] from comment #24) > > Comment on attachment 8540579 [details] [diff] [review] > > patch 3 - Audio in BrowserElement API > > > > Review of attachment 8540579 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/browser-element/BrowserElementAudioChannel.cpp > > @@ +130,5 @@ > > > +bool > > > +BrowserElementAudioChannel::GetMuted(ErrorResult& aRv) > > > +{ > > > + MOZ_ASSERT(NS_IsMainThread()); > > > + AssertIsInMainProcess(); > > > > Does those function calls (muted/volume/...) must invoked in main > > process(b2g)? > > Yes. BrowserElement API is used by system app running on the main-process. > Right? > If this is not true, we can change this. It's true.

(In reply to Andrea Marchesini (:baku) from comment #27) > > We cannot just mute one of them if you don't split the channel info of them > > into their own frame. > > I need more info about this. Jonas, the current patch uses the childID to > mute/stop/whatever the Media Elements. This chidID is taken from the > frameLoader. Any idea about how to improve this to cover what Alive wants? How about mDocument->InnerWindowID()?

Comment on attachment 8540579 [details] [diff] [review] patch 3 - Audio in BrowserElement API Review of attachment 8540579 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/nsBrowserElement.cpp @@ +647,5 @@ > + if (allowed) { > + channels.AppendElement( > + new BrowserElementAudioChannel(frameElement, > + (AudioChannel)audioChannelTable[i].value, > + childID)); If the system app call this API, how could it access others process's BrowserElementAudioChannel object?. I mean the childID would be zero.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #29) > (In reply to Andrea Marchesini (:baku) from comment #27) > > > We cannot just mute one of them if you don't split the channel info of them > > > into their own frame. > > > > I need more info about this. Jonas, the current patch uses the childID to > > mute/stop/whatever the Media Elements. This chidID is taken from the > > frameLoader. Any idea about how to improve this to cover what Alive wants? > > How about mDocument->InnerWindowID()? The problem with the InnerWindowID is that the audio Element into the app doesn't have any clue about the innnerWindowID in the parent process. I can change ContentParent/Child in order to share the innerWindowID of the FrameLoader, but I really need a feedback from jonas before doing this changes.

> If the system app call this API, how could it access others process's > BrowserElementAudioChannel object?. I mean the childID would be zero. ChildID is taken from the nsIFrameLoader and it's the childID of the ContentParent. It is the childID of the remote process.

This bug is long so I didn't read through the whole thing. Am I right in the following: * Alive wants the API on the browser element to control audio policies for the contents of that <iframe>. I.e. the API wouldn't affect the audio policy of the *app*, but rather the policy of the page that runs inside that particular iframe. * This is tricky to implement because we currently track audio policies per appid rather than per <iframe>. Let me know if the above is wrong. If so, the below might not be relevant. First off, I agree that it sounds better to track audio policies per <iframe> rather than per app. So we should figure out a way to make that happen. I think generally the way to do that is to pass PBrowser references between the parent and child, rather than passing appids. I don't fully know if the parent or the child is actually doing the audio playing. I.e. are audio buffers + appid passed from the child to the parent and then the parent plays them after looking up what volume the given appid should use? Or does the parent pass to the child what volume the given appid should use. And then the child uses that volume as it's playing its audio buffers? Either way, rather than passing an appid, could we pass a PBrowser? The child should know which PBrowser a given audio element belongs to, and the parent should know what volumes to use for a given PBrowser. So as we're communicating audio buffers or audio policies across the IPC boundary, can we pass along the PBrowser that it's connected to?

(In reply to Andrea Marchesini (:baku) from comment #32) > > If the system app call this API, how could it access others process's > > BrowserElementAudioChannel object?. I mean the childID would be zero. > > ChildID is taken from the nsIFrameLoader and it's the childID of the > ContentParent. It is the childID of the remote process. I checked the child ID from https://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp#2157 mChildID = mRemoteBrowser->Manager()->ChildID(); But it would return 0 on B2G platform.

> * This is tricky to implement because we currently track audio policies per > appid rather than per <iframe>. > > Let me know if the above is wrong. If so, the below might not be relevant. You are almost right: instead using appIDs the current audiochannel policy is based on childIDs. > I don't fully know if the parent or the child is actually doing the audio > playing. I.e. are audio buffers + appid passed from the child to the parent > and then the parent plays them after looking up what volume the given appid > should use? No. We have an AudioChannelAgent that is an object connected to the AudioChannelService. We have 1 AudioChannelAgent per media element (or for each AudioDestinationNode for webIDL). This AudioChannelAgent runs on the main-thread of the child process (if the app is remote) and it speaks with the AudioChannelService using IPC. This AudioChannelAgent receives notifications from AudioChannelService when something "could" be changed. At this point the AudioChannelAgent asks the state of its childId and its AudioChannel and the visibility of the document. Then it mutes/stops/does something. The notifications are sent by the AudioChannelService when the visibility changes, when a new AudioChannelAgent is created, and for other reasons. This is what we have now. What we are going to implement will be different :) > Either way, rather than passing an appid, could we pass a PBrowser? The > child should know which PBrowser a given audio element belongs to, and the > parent should know what volumes to use for a given PBrowser. So as we're > communicating audio buffers or audio policies across the IPC boundary, can > we pass along the PBrowser that it's connected to? Do we use PBrowser also for no-remote frameLoader? In case, can we do something similar to what we have done for PContent with the childID? An incremental unique ID.

We don't have a PBrowser for non-remote iframes. That also means that we don't have a PBrowser for iframes that live inside the child process. Based on your description, i'm still not sure where audio playing and volume scaling happens. I.e. if it happens in the child or in the parent. It sounds like maybe the audio scaling happens in the child, is that correct? If so, and if we'd like to keep it that way, I suggest we do the following: On each docshell track the volumes and mute state for all channels. When the browser API is used to set volumes/mute: * If it's a remote browser, use the PBrowser ipdl protocol to send that information to the docshell in the inner process. * If it's not a remote browser, simply grab the docshell and set the information directly. When getting the volume for a given <audio> or AudioContext, grab the docshell of the page. If no volume information has been set, walk up the docshell tree until the information is found. Would that work? Does it solve the problem being asked here?

Comment on attachment 8540579 [details] [diff] [review] patch 3 - Audio in BrowserElement API Review of attachment 8540579 [details] [diff] [review]: ----------------------------------------------------------------- I think comment 36 is the way to go here. But that would change this patch, so clearing the review request for now.

Comment on attachment 8540238 [details] [diff] [review] patch 2 - IDL As c37, clear the review for this one.

Test the muted API in BrowserElementAudioChannel on flame. 01-07 17:44:54.795 I/Log( 7411): SetAudioChannelMuted = false<-- set the audiochannel[i].muted = false in system app. 01-07 17:44:54.915 I/LOg( 7897): <----Resume play in media element. 01-07 17:50:58.085 I/Log( 7411): SetAudioChannelMuted = false 01-07 17:50:58.145 I/Log( 7897): Resume.... 01-07 17:51:47.115 I/Log( 7411): SetAudioChannelMuted = false 01-07 17:51:47.195 I/Log( 7897): Resume....

Randy Lin, thanks for your testing. I'm working on a new version of the patch with the Jonas' and Alive's comments. It's 1 single patch instead 5 because it was easier for me to work in a huge refactoring instead splitting it in several patches. The review process will be a bit more complex, sorry. There are still 2 things unfinished, but it will be done for this or max next week.

As promised, here the single huge page of the new API. If I can give a suggestion to the reviewer, ignore the old AudioChannelService code and read just the new. There are still a couple of TODO. Those will be fixed in a separate patch if this first block looks good: for those I need to send info from the child to the main-process using IPC. Nothing magic, but it's something built on top of the new logic and I would like to have a feedback first. There is a OOP test and a INPROC test, both working. I'm asking Ehsan to review this code because he knows about window management, threads, processes and IPC. I also would like to have a review from rlin after.

s/page/patch/

Andrea, can you please add some high level info on the architecture and different bits of the patch? Anything that can help make it easier to review this big patch. :-) Thanks!

The basic idea is: We have 1 AudioChannelAgent per MediaElement and AudioDestinationNode. This agent knows the top window where the audio object is running. When the audio object is active, the AudioChannelAgent is registered into the AudioChannelService. We have 1 AudioChannelService per process. This service stores all the AudioChannelAgent into an hashtable where the key is the windowID. This values of these keys are AudioChannelWindow objects. In these AudioChannelWindow objects we have: the list of AudioChannelAgents and an AudioChannelConfig for each possible channel. AudioChannelConfig has the number of active elements, a volume an a muted value. AudioChannelService exposes some generic methods to retrieve and set the volume and the muted values of a window. Going up in the hierarchy: for instance if the top window has volume 0.8, and the sub window has volume 0.2, the real volume of the sub window is 0.8 * 0.2. When a new volume/muted is set, AudioChannelService refreshes all the audioChannelAgent calling RefreshAgentsVolume() for that particular window and all the elements will have the new volume/muted values. BrowserElementAudioChannels are exposed from iframe.allowedAudioChannels attribute in any iframe mozbrowser (with some permission check). This is a fixed array because we know the app running into the iframe. From the app manifest we know which audiochannel is allowed by permissions and we populate the array. Each BrowserElementAudioChannel allows to set/get volume/muted and to know if we have some audio object active for that particular audioChannel. For instance I can do: iframe.allowedAudioChannels[0].isActive().onsuccess = function(e) { dump("Something active for channel " + iframe.allowedAudioChannels[0].name + ": " + e.target.result); } All the methods are async because it can happen that iframe runs an app into a separated process. At this point we use the BrowserElementAPI to go to the child process and send a message to the AudioChannelService for that process. I guess this is all. I hope it makes sense.

Comment on attachment 8546000 [details] [diff] [review] patch Review of attachment 8546000 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +345,5 @@ > (mPhoneState == PHONE_STATE_RINGTONE)) { > return; > } > > + nsRefPtr<AudioChannelService> service = AudioChannelService::GetOrCreateAudio(); AudioChannelService::GetOrCreate();

Comment on attachment 8546000 [details] [diff] [review] patch Review of attachment 8546000 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fmradio/FMRadio.cpp @@ +456,5 @@ > > + float volume = 0.0; > + bool muted = true; > + mAudioChannelAgent->StartPlaying(&volume, &muted); > + CanPlayChanged(volume, muted); Can't find this function. :) @@ +467,3 @@ > { > + IFMRadioService::Singleton()->EnableAudio(!aMuted); > + // TODO: what about the volume? If we want to control the FM Volume, we should call the setStreamVolumeIndex in AudioManager for the FM_Device. But it would affect the design of our original volume control mechanism like the volume bar UI sync problem.

Hi Baku, When user want to play, could we let it's status to be pause, then let system app to resume it while receiving the BrowserElementAudioChannel:onactivestatechanged event?

For the compilation issue on B2g, I'm fixing them. I didn't test the patch for b2g... my fault. > When user want to play, could we let it's status to be pause, then let > system app to resume it while receiving the > BrowserElementAudioChannel:onactivestatechanged event? We can do this for b2g only. This code is shared with browser and android. What I can do is: by default, a new window is muted. How does it sound?

Fixed the b2g issues.

(In reply to Andrea Marchesini (:baku) from comment #48) > For the compilation issue on B2g, I'm fixing them. I didn't test the patch > for b2g... my fault. > > > When user want to play, could we let it's status to be pause, then let > > system app to resume it while receiving the > > BrowserElementAudioChannel:onactivestatechanged event? > > We can do this for b2g only. This code is shared with browser and android. > What I can do is: > by default, a new window is muted. How does it sound? I have tested your patch and set the mAudioMuted = ture @ nsGlobalWindow.cpp, When music try to play, the status would be "PAUSE" As I test before, I think system app can resume it by call this method + [Throws] + DOMRequest setMuted(boolean aMuted); in BrowserElementAudioChannel API. So the sound at the beginning of song won't be lose if we pause first and resume. I think this behavior can be in b2g only.

I do the check for the windowID in AudioChannelService::RegisterAudioChannelAgent() function, but I found the value is 1 whatever I try to play audio in music /browser/dailer, does the value is unique across processes?

(In reply to Randy Lin [:rlin] from comment #51) > I do the check for the windowID in > AudioChannelService::RegisterAudioChannelAgent() function, but I found the > value is 1 whatever I try to play audio in music /browser/dailer, does the > value is unique across processes? we have 1 AudioChannelService for each process. and the windowID is unique per process. So yes, we can have the same windowID in different process. But when we mute 1 iframeLoader, we use IPC if the app is removed. There is a mochitest for this. I used it to debug it with gdb attaching the debugger to the child and the parent processes.

(In reply to Andrea Marchesini (:baku) from comment #52) > (In reply to Randy Lin [:rlin] from comment #51) > > I do the check for the windowID in > > AudioChannelService::RegisterAudioChannelAgent() function, but I found the > > value is 1 whatever I try to play audio in music /browser/dailer, does the > > value is unique across processes? > > we have 1 AudioChannelService for each process. and the windowID is unique > per process. So yes, we can have the same windowID in different process. But > when we mute 1 iframeLoader, we use IPC if the app is removed. > > There is a mochitest for this. I used it to debug it with gdb attaching the > debugger to the child and the parent processes. Oh, AudioChannelService exists in every process and I didn't observe this. Thanks for this information.

The original play latency on music app: 01-15 12:08:17.218 I/HTMLMediaElement::Play( 1319): play........ 01-15 12:08:17.598 E/AudioTrack( 1319): ..start() status 0 <-- is android back-end start to play. 370ms 01-15 12:10:27.238 I/HTMLMediaElement::Play( 1341): play........ 01-15 12:10:27.578 E/AudioTrack::start()( 1341): ..start() status 0 340ms 01-15 12:09:02.988 I/HTMLMediaElement::Play( 1319): play........ 01-15 12:09:03.228 E/AudioTrack::start()( 1319): ..start() status 0 240ms 01-15 12:09:22.498 I/HTMLMediaElement::Play( 1319): play........ 01-15 12:09:22.748 E/AudioTrack::start()( 1319): ..start() status 0 250ms The longer is opening new app and try to play a 100sec mp3 file, short one is just re-play the same mp3 file by click FR button in music application.

Hi, Andrea, I have some questions about your new design, could you help me? I would shortly describe what I know, if there is any wrong parts, please correct me :) [New audio channel is created] 1. Register an AudioChannelAgent to the AudioChannelService 2. AudioChannelService fires a event to the ObserveService ("audiochannel-activity-active") 3. ObserveService would send this event across the IPC border, then send it to the BrowserElementAudioChannel 4. BrowserElementAudioChannel fires the "onActivityChange" event 5. System app should receive the "onActivityChange" event then do something.. [System app behavior] 1. By default, this audio should not be playback at the beginning 2. Decide whether the audio can be playback 3. Use BrowserElementAudioChannel to control its play state (setVol/Muted..) [Use system app to control audio channels] 1. Select the specific browser element, then get its allowAudioChannels 2. BrowserElementAudioChannel fires the functions setVol/Muted() via BrowserElementAPI 3. BrowserElementParent.js sends the request to across the IPC border 4. BrowserElementChildPreload.js gets this request 5. Notify AudioChannelService which window should be modified 6. AudioChannelService change its volume/muted 7. Notify the AudioChannelAgents belonging to this window 8. AudioChannelAgent would notify to MediaElements they should be changed its volume or playback state

Here still are a question :) If the operation flow of comment55 is correct, what is the usage of TabParent/Child in this architecture? Because I saw your add the event register & receive in these codes, I am not sure about its purpose.

I did a test to identify whether the new audio channel architecture has a latency issue. So I measured the time which is from "MediaElement starting playback" to "actually changing its playback state". The result didn't involving the decoding time, because I think that is no related with audio channel. Here are two test cases, the first one is to playback a new audio file, the second one is to playback paused audio file. I did these tests on the music app, and the duration of the audio file is 5:01. ============================================================= [Total time][Old architecture] [Case 1][Play a new audio file] [Average time = 1.043 ms] 1) 0.860 ms 01-15 21:15:06.779 2841 2841 I Gecko : DD | HTMLMediaElement::Play 01-15 21:15:07.639 2841 2841 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 2) 1.110 ms 01-15 21:18:06.049 2867 2867 I Gecko : DD | HTMLMediaElement::Play 01-15 21:18:07.159 2867 2867 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 3) 1.160 ms 01-15 21:18:42.069 3245 3245 I Gecko : DD | HTMLMediaElement::Play 01-15 21:18:43.229 3245 3245 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 [Case 2][Play a paused audio file] [Average time = 0.053 ms] 1) 0.040 ms 01-15 21:19:14.149 3245 3245 I Gecko : DD | HTMLMediaElement::Play 01-15 21:19:14.189 3245 3245 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 2) 0.070 ms 01-15 21:19:36.989 3245 3245 I Gecko : DD | HTMLMediaElement::Play 01-15 21:19:37.059 3245 3245 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 3) 0.050 ms 01-15 21:19:48.569 3245 3245 I Gecko : DD | HTMLMediaElement::Play 01-15 21:19:48.619 3245 3245 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 ============================================================= [Total time][New architecture] * In new architecture , |WindowVolumeChanged| means the |CanPlayChanged| [Case 1][Play a new audio file] [Average time = 1.153 ms] 1) 0.980 ms 01-15 20:42:37.395 13508 13508 I Gecko : DD | HTMLMediaElement::Play 01-15 20:42:38.315 13508 13508 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged 2) 1.210 ms 01-15 20:44:14.805 13530 13530 I Gecko : DD | HTMLMediaElement::Play 01-15 20:44:16.015 13530 13530 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged 3) 1.270 ms 01-15 20:44:42.605 13757 13757 I Gecko : DD | HTMLMediaElement::Play 01-15 20:44:43.875 13757 13757 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged [Case 2][Play a paused audio file] [Average time = 0.050 ms] 1) 0.050 ms 01-15 20:46:51.535 13757 13757 I Gecko : DD | HTMLMediaElement::Play 01-15 20:46:51.585 13757 13757 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged 2) 0.060 ms 01-15 20:47:33.185 13757 13757 I Gecko : DD | HTMLMediaElement::Play 01-15 20:47:33.245 13757 13757 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged 3) 0.040 ms 01-15 20:48:12.625 13757 13757 I Gecko : DD | HTMLMediaElement::Play 01-15 20:48:12.665 13757 13757 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged ================================================================================ My conclusion is that the new architecture doesn't has a latency issue. I would post another result later about the communication time between gecko and gaia.

This result is about the new audio architecture. It measures the time which is from "sending a request from AudioChannelService" to "AudioChannelService getting the response from system app". The test case is the same as comment57's. ================================================================================ [Communication time][New architecture] [Case 1][Play a new audio file] [Average time = 0.040 ms] 1) 0.010 ms 01-15 19:48:32.692 8893 8893 I Gecko : DD | ACS::NotifyChannelActive 01-15 19:48:32.692 8030 8030 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 19:48:32.702 8893 8893 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 19:48:32.702 8893 8893 I Gecko : DD | AudioChannelService::SetAudioChannelMuted 2) 0.080 ms 01-15 20:13:17.337 10648 10648 I Gecko : DD | ACS::NotifyChannelActive 01-15 20:13:17.337 9844 9844 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 20:13:17.407 10648 10648 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 20:13:17.417 10648 10648 I Gecko : DD | AudioChannelService::SetAudioChannelMuted 3) 0.030 ms 01-15 20:18:32.737 11722 11722 I Gecko : DD | ACS::NotifyChannelActive 01-15 20:18:32.737 9844 9844 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 20:18:32.767 11722 11722 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 20:18:32.767 11722 11722 I Gecko : DD | AudioChannelService::SetAudioChannelMuted [Case 2][Play a paused audio file] [Average time = 0.047 ms] 1) 0.040 ms 01-15 20:08:59.367 10514 10514 I Gecko : DD | ACS::NotifyChannelActive 01-15 20:08:59.367 9844 9844 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 20:08:59.407 10514 10514 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 20:08:59.407 10514 10514 I Gecko : DD | AudioChannelService::SetAudioChannelMuted 2) 0.050 ms 01-15 20:16:42.657 10648 10648 I Gecko : DD | ACS::NotifyChannelActive 01-15 20:16:42.657 9844 9844 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 20:16:42.707 10648 10648 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 20:16:42.707 10648 10648 I Gecko : DD | AudioChannelService::SetAudioChannelMuted 3) 0.050 ms 01-15 20:22:11.957 11722 11722 I Gecko : DD | ACS::NotifyChannelActive 01-15 20:22:11.957 9844 9844 I Gecko : DD | BrowserElementAudioChannel, send activestatechanged 01-15 20:22:12.077 11722 11722 I Gecko : DD | BEChild::_recvSetAudioChannelMuted 01-15 20:22:12.077 11722 11722 I Gecko : DD | AudioChannelService::SetAudioChannelMuted

> [New audio channel is created] > 1. Register an AudioChannelAgent to the AudioChannelService > 2. AudioChannelService fires a event to the ObserveService > ("audiochannel-activity-active") This happens if this is the first AudioChannelAgent for this window. If the window has something else active, we don't send the notification again. > 3. ObserveService would send this event across the IPC border, then send it > to the BrowserElementAudioChannel This happens if the AudioChannelService is not running on the parent process. In that case BrowserElementAudioChannel receives the notification directly. Otherwise, TabChild propagates the notification via IPC. > 4. BrowserElementAudioChannel fires the "onActivityChange" event > 5. System app should receive the "onActivityChange" event then do something.. Correct. > [System app behavior] > 1. By default, this audio should not be playback at the beginning This is not in the patch. But it's just a: #ifdef MOZ_B2G mMuted(true) #else mMuted(false) #endif > 2. Decide whether the audio can be playback > 3. Use BrowserElementAudioChannel to control its play state (setVol/Muted..) Correct. > [Use system app to control audio channels] > 1. Select the specific browser element, then get its allowAudioChannels > 2. BrowserElementAudioChannel fires the functions setVol/Muted() via > BrowserElementAPI If the app is running in a child process. Otherwise BrowserElementAudioChannel speaks with AudioChannelService directly, and we jump to point 6. > 3. BrowserElementParent.js sends the request to across the IPC border > 4. BrowserElementChildPreload.js gets this request > 5. Notify AudioChannelService which window should be modified > 6. AudioChannelService change its volume/muted > 7. Notify the AudioChannelAgents belonging to this window > 8. AudioChannelAgent would notify to MediaElements they should be changed > its volume or playback state Right.

(In reply to Alastor Wu [:alwu] from comment #56) > Here still are a question :) > If the operation flow of comment55 is correct, what is the usage of > TabParent/Child in this architecture? > Because I saw your add the event register & receive in these codes, I am not > sure about its purpose. AudioChannelService notifies observers with audio-activity-active. This doesn't cross the border yet. If AudioChannelService runs in a child process, TabChild receives this notification and sends it to TabParent. From there BrowserElementAudioChannel.

> My conclusion is that the new architecture doesn't has a latency issue. > I would post another result later about the communication time between gecko > and gaia. That is a good news. Can you test MediaElements and WebAudio in child processes and in parent process? Thanks!

Thanks! Your explain help me a lots :) There are some mistakes in the result of comment57&58, one is the wrong measure unit (it should be s, not ms), another one is using the AMR file to do testing (AMR file needs long time to decode, it's not proper to use it measuring the playback latency) So I will do the test again, and also test the WebAudio. I am not sure whether the MediaElements/WebAudio can run on parent process. I need to check it.

Hi, Andrea, I have some confused questions, could you help me? (1) > if (aWindow) { > nsCOMPtr<nsIDOMWindow> topWindow; > aWindow->GetScriptableTop(getter_AddRefs(topWindow)); > MOZ_ASSERT(topWindow); > > mWindow = do_QueryInterface(topWindow); > mWindow = mWindow->GetOuterWindow(); > > mWindowID = mWindow->WindowID(); > } If I don't get the SciptableTop & OuterWindow, just using the original window parameter (aWindow) which we send in, does that have any effect? I tried to remove them, and it can still work normally, so I have little confused about. (2) > if (!mFrameWindow) { > if (mTabParent == aSubject) { <-- fail here!! > ProcessStateChanged(aData); > } > return NS_OK; > } I tried to test the FMRadio in B2G, but it can't work. The reason is that the BrowserElement can't receive the notification from AudioChannelService. I can't receive the correct TabParent. Now I'm not familiar with this part, do you have any idea about? Very appreciate :)

> If I don't get the SciptableTop & OuterWindow, just using the original > window parameter (aWindow) which we send in, does that have any effect? > > I tried to remove them, and it can still work normally, so I have little > confused about. No. try to create a sub iframe with a media element. It's important to get the top-level window. > I can't receive the correct TabParent. Now I'm not familiar with this part, > do you have any idea about? No. I have to investigate it. I'll let you know!

Update the latency testing result. [Info] Test file format : mp3 File duration : 7:05 Device : flame ============================================================= [Total time][Old architecture] [Case 1][Play a new audio file] [Average time = 310 ms] 1) 320 ms 01-19 09:59:00.406 1301 1301 I Gecko : DD | HTMLMediaElement::Play 01-19 09:59:00.626 1301 1301 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 2) 300 ms 01-19 10:01:07.966 1617 1617 I Gecko : DD | HTMLMediaElement::Play 01-19 10:01:08.266 1617 1617 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 3) 310 ms 01-19 10:01:27.766 1715 1715 I Gecko : DD | HTMLMediaElement::Play 01-19 10:01:28.076 1715 1715 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 [Case 2][Play a paused audio file] [Average time = 37 ms] 1) 30 ms 01-19 09:59:40.086 1301 1301 I Gecko : DD | HTMLMediaElement::Play 01-19 09:59:40.116 1301 1301 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 2) 30 ms 01-19 09:59:54.016 1301 1301 I Gecko : DD | HTMLMediaElement::Play 01-19 09:59:54.046 1301 1301 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 3) 50 ms 01-19 10:00:04.266 1301 1301 I Gecko : DD | HTMLMediaElement::Play 01-19 10:00:04.316 1301 1301 I Gecko : DD | HTMLMediaElement::CanPlayChanged, canPlay = 0 ============================================================= [Total time][New architecture] * In new architecture , |WindowVolumeChanged| means the |CanPlayChanged| [Case 1][Play a new audio file] [Average time = 213 ms] 1) 190 ms 01-19 09:50:03.805 22005 22005 I Gecko : DD | HTMLMediaElement::Play 01-19 09:50:04.015 22005 22005 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 2) 220 ms 01-19 09:53:30.155 22033 22033 I Gecko : DD | HTMLMediaElement::Play 01-19 09:53:30.375 22033 22033 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 3) 230 ms 01-19 09:54:29.035 22676 22676 I Gecko : DD | HTMLMediaElement::Play 01-19 09:54:29.265 22676 22676 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 [Case 2][Play a paused audio file] [Average time = 30 ms] 1) 30 ms 01-19 09:51:46.205 22005 22005 I Gecko : DD | HTMLMediaElement::Play 01-19 09:51:46.235 22005 22005 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 2) 30 ms 01-19 09:52:26.605 22005 22005 I Gecko : DD | HTMLMediaElement::Play 01-19 09:52:26.635 22005 22005 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 3) 30 ms 01-19 09:52:55.835 22005 22005 I Gecko : DD | HTMLMediaElement::Play 01-19 09:52:55.865 22005 22005 I Gecko : DD | HTMLMediaElement::WindowVolumeChanged, aVolume = 1.000000 ================================================================================

Hi, Andrea, I could run the FM successful on B2G now, just using this patch. The fail reason is that we didn't create the BrowserElementAudioChannel for FM, because its manifest didn't have the permission - "audio-channel-content".

Great! So, a part the review process, this patch is working as we want, right?

Copy flags from bug 1069887 -- Keven

It seems that we works on the correct way :) We can discuss more details with Dominic and Evan on the Vidyo meeting. Here is the testing result on the different RAM setting of the Flame. ================================================================================ [RAM 512MB][Average time] [Case 1][Play a new audio file] New architecture = 293 ms Old architecture = 283 ms [Case 2][Play a paused audio file] New architecture = 50 ms Old architecture = 47 ms ================================================================================ [RAM 256MB][Average time] [Case 1][Play a new audio file] New architecture = 805 ms Old architecture = 740 ms [Case 2][Play a paused audio file] New architecture = 47 ms Old architecture = 60 ms

Comment on attachment 8546548 [details] [diff] [review] byebyelogic.patch Review of attachment 8546548 [details] [diff] [review]: ----------------------------------------------------------------- Apologies for the delay! Please provide interdiffs for all of the upcoming fixes. Thanks! ::: browser/installer/package-manifest.in @@ +888,5 @@ > @RESPATH@/components/DataStore.manifest > @RESPATH@/components/DataStoreImpl.js > @RESPATH@/components/dom_datastore.xpt > > +@RESPATH@/components/dom_audiochannel.xpt This already exists here, right? <https://dxr.mozilla.org/mozilla-central/source/b2g/installer/package-manifest.in#155> ::: dom/audiochannel/AudioChannelAgent.cpp @@ +93,5 @@ > + > + mWindow = do_QueryInterface(topWindow); > + mWindow = mWindow->GetOuterWindow(); > + > + mWindowID = mWindow->WindowID(); Why do you need to store the window ID here? Can't you just make WindowID() read it off of mWindow? ::: dom/audiochannel/AudioChannelService.cpp @@ +92,5 @@ > > + if (!gAudioChannelService) { > + // Create new instance, register, return > + nsRefPtr<AudioChannelService> service = new AudioChannelService(); > + MOZ_ASSERT(service); new is infallible, no need to assert. @@ +94,5 @@ > + // Create new instance, register, return > + nsRefPtr<AudioChannelService> service = new AudioChannelService(); > + MOZ_ASSERT(service); > + > + gAudioChannelService = service; Why not just assign to gAudioChannelService directly? @@ +99,3 @@ > } > > + nsRefPtr<AudioChannelService> service = gAudioChannelService.get(); Drop the .get() please. nsRefPtr knows how to copy itself. :-) @@ +141,1 @@ > obs->AddObserver(this, "xpcom-shutdown", false); So we never call RemoveObserver on these right? That causes the observer service to keep this alive until XPCOM is shut down. Shouldn't you RemoveObserver() all of these in the handler for xpcom-shutdown? @@ +162,5 @@ > > + uint64_t windowID = aAgent->WindowID(); > + AudioChannelWindow* winData = nullptr; > + > + if (!mWindows.Get(windowID, &winData)) { Any reason why not do the more readable thing? AudioChannelWindow* winData = mWindows.LookupOrAdd(windowID); @@ +168,5 @@ > + mWindows.Put(windowID, winData); > + } > + > + AudioChannelAgentData* data = new AudioChannelAgentData(aChannel); > + winData->mAgents.Put(aAgent, data); What guarantees that RegisterAudioChannelAgent is not called twice with the same agent? It seems like you'd just overwrite the previous one in that case. Also, doing a dynamic allocation just to store an integer is very wasteful. My typedef suggestion should help. :-) @@ +245,5 @@ > + > + *aVolume = 1.0; > + *aMuted = false; > + > + if (!aWindow) { Don't you need to check IsOuterWindow() here as well? @@ +254,4 @@ > > + nsCOMPtr<nsPIDOMWindow> window = aWindow; > + > + do { Please document what this loop does. @@ +263,5 @@ > + *aVolume *= window->GetAudioVolume(); > + *aMuted = *aMuted || window->GetAudioMuted(); > + > + nsCOMPtr<nsIDOMWindow> win; > + window->GetParent(getter_AddRefs(win)); It is OK to dishonor the mozbrowser boundaries here? (IOW, use GetParent and not GetScriptableParent?) @@ +271,4 @@ > > + window = do_QueryInterface(win); > + > + // If there is not parent, or we are the toplevel or the volume is Nit: no parent @@ +272,5 @@ > + window = do_QueryInterface(win); > + > + // If there is not parent, or we are the toplevel or the volume is > + // already 0.0, we don't continue. > + } while (window && window != aWindow); Did you forget to check *aVolume here? (Note that you should probably do an epsilon test.) Also, should you check *aMuted as well? I'm not sure if we care about exposing the correct volume number in the case where the channel is muted. @@ +291,5 @@ > > bool > AudioChannelService::TelephonyChannelIsActive() > { > + // TODO: no child process check. I expose you're going to handle this in the current bug. @@ +301,5 @@ > > bool > AudioChannelService::ProcessContentOrNormalChannelIsActive(uint64_t aChildID) > { > +/* TODO Ditto. @@ +334,5 @@ > > bool > AudioChannelService::AnyAudioChannelIsActive() > { > + // TODO: no child process check. This one too. @@ +411,5 @@ > } > #endif > } > > + else if (!strcmp(aTopic, "ipc:content-shutdown")) { You removed the AddObserver for "ipc:content-shutdown" but not this code. Why? @@ +460,5 @@ > AudioChannelService::RefreshAgentsVolume(nsPIDOMWindow* aWindow) > { > + AudioChannelWindow* winData = nullptr; > + if (!mWindows.Get(aWindow->WindowID(), &winData)) { > + return; Nit: please use the one argument version of Get() @@ +542,5 @@ > > + AudioChannelWindow* winData = nullptr; > + if (!mWindows.Get(aWindow->WindowID(), &winData)) { > + winData = new AudioChannelWindow(); > + mWindows.Put(aWindow->WindowID(), winData); You can use LookupOrAdd here as well. @@ +572,5 @@ > + aWindow->GetScriptableTop(getter_AddRefs(topWindow)); > + MOZ_ASSERT(topWindow); > + > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(topWindow); > + window = window->GetOuterWindow(); Please refactor this into a small helper and use it here and in other similar methods further below. @@ +715,5 @@ > + bool aHidden, > + uint64_t aChildID) > +{ > + if (XRE_GetProcessType() != GeckoProcessType_Default) { > + ContentChild *cc = ContentChild::GetSingleton(); Nit: ContentChild* :P @@ +725,5 @@ > } > > + // If this child is in the background and mDefChannelChildID is set to > + // others then it means other child in the foreground already set it's > + // own default channel already. Nit: s/ already//. ::: dom/audiochannel/AudioChannelService.h @@ +25,5 @@ > #ifdef MOZ_WIDGET_GONK > class SpeakerManagerService; > #endif > + > +#define LAST_AUDIOCHANNEL (uint32_t)AudioChannel::Publicnotification + 1 Call this: NUMBER_OF_AUDIO_CHANNELS. @@ +129,2 @@ > > protected: Why not private? @@ +141,3 @@ > {} > > AudioChannel mChannel; Wouldn't |typedef AudioChannel AudioChannelAgentData;| achieve the same purpose? @@ +152,5 @@ > + > + float mVolume; > + bool mMuted; > + > + uint32_t mItems; Wouldn't mNumberOfAgents be a better name? @@ +157,5 @@ > + }; > + > + struct AudioChannelWindow MOZ_FINAL { > + AudioChannelConfig mChannels[LAST_AUDIOCHANNEL]; > + nsClassHashtable<nsPtrHashKey<AudioChannelAgent>, AudioChannelAgentData> mAgents; What guarantees the agent objects stored in the key are not dead by the time you access them? Did you mean to use nsRefPtrHashKey? @@ +161,5 @@ > + nsClassHashtable<nsPtrHashKey<AudioChannelAgent>, AudioChannelAgentData> mAgents; > + }; > + > + AudioChannelWindow* > + GetOrCreateWindowData(nsPIDOMWindow* aWindow); It would be better to return an AudioChannelWindow& from this instead, in order to signal to the caller that the return value cannot be null. ::: dom/audiochannel/nsIAudioChannelService.idl @@ +1,5 @@ > +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* This Source Code Form is subject to the terms of the Mozilla Public > +* License, v. 2.0. If a copy of the MPL was not distributed with this > +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + Nit: trailing whitespace here and elsewhere in the file. ::: dom/browser-element/BrowserElementAudioChannel.cpp @@ +20,5 @@ > + > +void > +AssertIsInMainProcess() > +{ > + static const bool isMainProcess = I think you should make this a DebugOnly, otherwise you'll get a (fatal?) warning for the unused variable. @@ +36,5 @@ > + > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(BrowserElementAudioChannel) > + NS_INTERFACE_MAP_ENTRY(nsISupportsWeakReference) > + NS_INTERFACE_MAP_ENTRY(nsIObserver) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver) DETH already knows how to deal with nsISupports. @@ +166,5 @@ > + { > + nsRefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate(); > + MOZ_ASSERT(service); > + > + AutoJSAPI jsapi; Please ask Boris for review on the JSAPI usage. @@ +189,5 @@ > +protected: > + virtual void DoWork(AudioChannelService* aService, JSContext* aCx) MOZ_OVERRIDE > + { > + float volume = aService->GetAudioChannelVolume(mFrameWindow, mAudioChannel); > + JS::Rooted<JS::Value> value(aCx, JS::NumberValue(volume)); Why don't you use ToJSValue for all of these? ::: dom/browser-element/BrowserElementAudioChannel.h @@ +39,5 @@ > + nsIFrameLoader* aFrameLoader, > + nsIBrowserElementAPI* aAPI, > + AudioChannel aAudioChannel); > + > + void Initialize(ErrorResult& aRv); Based on how you call this method, I think it makes more sense to return an nsresult. ::: dom/browser-element/mochitest/browserElement_AudioChannel.js @@ +59,5 @@ > + .then(function() { > + return new Promise(function(r, rr) { > + ac.getVolume().onsuccess = function(e) { > + // the actual value is 0.800000011920929.. > + ok(e.target.result > 0.7 && e.target.result < 0.9, "The new volume should be 0.8: " + e.target.result); This is a huge range! Please do an epsilon test instead. ::: dom/browser-element/mochitest/file_audio.html @@ +5,5 @@ > +dump("Starting...\n"); > +var audio = document.getElementById('audio'); > +audio.play(); > +audio.onended = function() { > + dump("Ended\n"); Do you need the dumps? @@ +9,5 @@ > + dump("Ended\n"); > + setTimeout(function() { > + dump("Restarting...\n"); > + audio.play(); > + }, 1000); Why the 1 second pause? ::: dom/camera/DOMCameraControl.cpp @@ +704,5 @@ > mAudioChannelAgent->Init(mWindow, (int32_t)AudioChannel::Content, nullptr); > // Video recording doesn't output any sound, so it's not necessary to check canPlay. > + float volume = 0.0; > + bool muted = true; > + aRv = mAudioChannelAgent->StartPlaying(&volume, &muted); Shouldn't we let the callers who don't care about the out arguments to be able to pass nullptr? (Maybe not, because most callers seem to care...) ::: dom/fmradio/FMRadio.cpp @@ +467,3 @@ > { > + IFMRadioService::Singleton()->EnableAudio(!aMuted); > + // TODO: what about the volume? What about it? :-) ::: dom/html/HTMLMediaElement.cpp @@ +1751,5 @@ > } > > void HTMLMediaElement::SetVolumeInternal() > { > + float effectiveVolume = mMuted ? 0.0f : float(mVolume) * float(mAudioChannelVolume); Why not do the arithmetic in double precision and cast the result to a float? ::: dom/html/nsGenericHTMLFrameElement.cpp @@ +40,3 @@ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsGenericHTMLFrameElement, I wonder why this was _NO_UNLINK before? ::: dom/ipc/TabChild.cpp @@ +916,5 @@ > + for (uint32_t i = 0; table[i].tag; ++i) { > + topic.Assign("audiochannel-activity-"); > + topic.Append(table[i].tag); > + > + observerService->AddObserver(this, topic.get(), false); Where's the corresponding RemoveObserver calls? ::: dom/webidl/BrowserElementAudioChannel.webidl @@ +8,5 @@ > + CheckPermissions="browser"] > +interface BrowserElementAudioChannel : EventTarget { > + readonly attribute AudioChannel name; > + > + // This event is dispatch when this audiochannel is actually in used by the app. Nit: dispatched. Also, please document what kind of event this is. @@ +12,5 @@ > + // This event is dispatch when this audiochannel is actually in used by the app. > + attribute EventHandler onactivestatechanged; > + > + [Throws] > + DOMRequest getVolume(); I assume we want to stick with DOMRequest and not switch to Promise? ::: media/webrtc/trunk/webrtc/tools/e2e_quality/audio/perf @@ -1,1 @@ > -../../../../tools/perf \ No newline at end of file I already removed this annoying symlink a while ago. :-) ::: mobile/android/installer/package-manifest.in @@ +639,5 @@ > @BINPATH@/components/DataStore.manifest > @BINPATH@/components/DataStoreImpl.js > @BINPATH@/components/dom_datastore.xpt > + > +@BINPATH@/components/dom_audiochannel.xpt Do we want this on Android? If yes, why?

Comment on attachment 8546548 [details] [diff] [review] byebyelogic.patch Review of attachment 8546548 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/BrowserElementAudioChannel.webidl @@ +30,5 @@ > + > +partial interface BrowserElementPrivileged { > + [Constant, Cached, Throws] > + readonly attribute sequence<BrowserElementAudioChannel> allowedAudioChannels; > +}; I think this method also needs Pref="dom.mozBrowserFramesEnabled", CheckPermissions="browser"

> > +@RESPATH@/components/dom_audiochannel.xpt > > This already exists here, right? > <https://dxr.mozilla.org/mozilla-central/source/b2g/installer/package- > manifest.in#155> Right. But we need the audiochannel.xpt also for the browser. > @@ +99,3 @@ > > } > > > > + nsRefPtr<AudioChannelService> service = gAudioChannelService.get(); > > Drop the .get() please. nsRefPtr knows how to copy itself. :-) No: error: conversion from ‘mozilla::StaticRefPtr<mozilla::dom::AudioChannelService>’ to non-scalar type ‘nsRefPtr<mozilla::dom::AudioChannelService>’ requested > @@ +141,1 @@ > > obs->AddObserver(this, "xpcom-shutdown", false); > > So we never call RemoveObserver on these right? That causes the observer > service to keep this alive until XPCOM is shut down. Shouldn't you > RemoveObserver() all of these in the handler for xpcom-shutdown? This is done in Shutdown() and this happens when the browser is closed. > @@ +245,5 @@ > > + > > + *aVolume = 1.0; > > + *aMuted = false; > > + > > + if (!aWindow) { > > Don't you need to check IsOuterWindow() here as well? We have an assert: MOZ_ASSERT(!aWindow || aWindow->IsOuterWindow()); > > @@ +291,5 @@ > > > > bool > > AudioChannelService::TelephonyChannelIsActive() > > { > > + // TODO: no child process check. > > I expose you're going to handle this in the current bug. Yes. but a separate patch for the TODOs. > What guarantees the agent objects stored in the key are not dead by the time > you access them? Did you mean to use nsRefPtrHashKey? An Agent unregisters in the DTOR. > ::: dom/camera/DOMCameraControl.cpp > @@ +704,5 @@ > > mAudioChannelAgent->Init(mWindow, (int32_t)AudioChannel::Content, nullptr); > > // Video recording doesn't output any sound, so it's not necessary to check canPlay. > > + float volume = 0.0; > > + bool muted = true; > > + aRv = mAudioChannelAgent->StartPlaying(&volume, &muted); > > Shouldn't we let the callers who don't care about the out arguments to be > able to pass nullptr? (Maybe not, because most callers seem to care...) Indeed. This is the only case where we don't use the volume/muted. > ::: dom/fmradio/FMRadio.cpp > @@ +467,3 @@ > > { > > + IFMRadioService::Singleton()->EnableAudio(!aMuted); > > + // TODO: what about the volume? > > What about it? :-) Separate patch. > ::: dom/html/nsGenericHTMLFrameElement.cpp > @@ +40,3 @@ > > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsGenericHTMLFrameElement, > > I wonder why this was _NO_UNLINK before? a bug... :) I discussed this with smaug. > @@ +12,5 @@ > > + // This event is dispatch when this audiochannel is actually in used by the app. > > + attribute EventHandler onactivestatechanged; > > + > > + [Throws] > > + DOMRequest getVolume(); > > I assume we want to stick with DOMRequest and not switch to Promise? I guess we should switch to promise but not in this patch.

Here the full patch.

About the TODOs I'm going to propose a new patch. I'll not cover the FMRadio TODO because for that, we need something more complex and, btw, it's an existing bug: Read comment 46.

This last patch changes the mochitest in order to have an sub-iframe with a media element and it works perfectly. Let me know if you see any other corner-cases.

(In reply to Andrea Marchesini (:baku) from comment #73) > > > +@RESPATH@/components/dom_audiochannel.xpt > > > > This already exists here, right? > > <https://dxr.mozilla.org/mozilla-central/source/b2g/installer/package- > > manifest.in#155> > > Right. But we need the audiochannel.xpt also for the browser. Oh, sorry I was confused! > > @@ +99,3 @@ > > > } > > > > > > + nsRefPtr<AudioChannelService> service = gAudioChannelService.get(); > > > > Drop the .get() please. nsRefPtr knows how to copy itself. :-) > > No: error: conversion from > ‘mozilla::StaticRefPtr<mozilla::dom::AudioChannelService>’ to non-scalar > type ‘nsRefPtr<mozilla::dom::AudioChannelService>’ requested OK. > > @@ +141,1 @@ > > > obs->AddObserver(this, "xpcom-shutdown", false); > > > > So we never call RemoveObserver on these right? That causes the observer > > service to keep this alive until XPCOM is shut down. Shouldn't you > > RemoveObserver() all of these in the handler for xpcom-shutdown? > > This is done in Shutdown() and this happens when the browser is closed. You're right! > > @@ +245,5 @@ > > > + > > > + *aVolume = 1.0; > > > + *aMuted = false; > > > + > > > + if (!aWindow) { > > > > Don't you need to check IsOuterWindow() here as well? > > We have an assert: MOZ_ASSERT(!aWindow || aWindow->IsOuterWindow()); Yes, but the assertion only works on debug builds. Since you're doing the null check in non-debug builds, should you check the outer-ness as well? > > @@ +291,5 @@ > > > > > > bool > > > AudioChannelService::TelephonyChannelIsActive() > > > { > > > + // TODO: no child process check. > > > > I expose you're going to handle this in the current bug. > > Yes. but a separate patch for the TODOs. > > > What guarantees the agent objects stored in the key are not dead by the time > > you access them? Did you mean to use nsRefPtrHashKey? > > An Agent unregisters in the DTOR. OK. > > ::: dom/camera/DOMCameraControl.cpp > > @@ +704,5 @@ > > > mAudioChannelAgent->Init(mWindow, (int32_t)AudioChannel::Content, nullptr); > > > // Video recording doesn't output any sound, so it's not necessary to check canPlay. > > > + float volume = 0.0; > > > + bool muted = true; > > > + aRv = mAudioChannelAgent->StartPlaying(&volume, &muted); > > > > Shouldn't we let the callers who don't care about the out arguments to be > > able to pass nullptr? (Maybe not, because most callers seem to care...) > > Indeed. This is the only case where we don't use the volume/muted. > > > ::: dom/fmradio/FMRadio.cpp > > @@ +467,3 @@ > > > { > > > + IFMRadioService::Singleton()->EnableAudio(!aMuted); > > > + // TODO: what about the volume? > > > > What about it? :-) > > Separate patch. > > > ::: dom/html/nsGenericHTMLFrameElement.cpp > > @@ +40,3 @@ > > > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > > > > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsGenericHTMLFrameElement, > > > > I wonder why this was _NO_UNLINK before? > > a bug... :) I discussed this with smaug. > > > @@ +12,5 @@ > > > + // This event is dispatch when this audiochannel is actually in used by the app. > > > + attribute EventHandler onactivestatechanged; > > > + > > > + [Throws] > > > + DOMRequest getVolume(); > > > > I assume we want to stick with DOMRequest and not switch to Promise? > > I guess we should switch to promise but not in this patch. OK, that's fine.

Comment on attachment 8552404 [details] [diff] [review] interdiff Review of attachment 8552404 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +70,5 @@ > ? MOZ_UTF16("active") : MOZ_UTF16("inactive")); > } > > +void > +GetTopWindow(nsIDOMWindow* aWindow, nsPIDOMWindow** aPWindow) Can you please make this return an already_AddRefed<nsPIDOMWindow> instead? ::: mobile/android/installer/package-manifest.in @@ -648,5 @@ > @BINPATH@/components/DataStore.manifest > @BINPATH@/components/DataStoreImpl.js > @BINPATH@/components/dom_datastore.xpt > - > -@BINPATH@/components/dom_audiochannel.xpt Can you please explain how you decide where to enable these interfaces on?

Comment on attachment 8552406 [details] [diff] [review] patch 1 - BrowserElementAudioChannel I didn't review this.

Comment on attachment 8552473 [details] [diff] [review] patch 2 - telephony/content/normal channels are active Review of attachment 8552473 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +346,5 @@ > +{ > + bool* isActive = static_cast<bool*>(aPtr); > + *isActive = > + aWinData->mChannels[(uint32_t)AudioChannel::Content].mNumberOfAgents || > + aWinData->mChannels[(uint32_t)AudioChannel::Normal].mNumberOfAgents; Nit: > 0 both both conditions, please. ::: dom/audiochannel/AudioChannelService.h @@ +206,5 @@ > + const uint64_t& aChildID, > + nsAutoPtr<AudioChannelChildStatus>& aData, > + void *aPtr); > + > + nsClassHashtable<nsUint64HashKey, AudioChannelChildStatus> mChildren; Given that we remove the child if it doesn't have any playing channels, wouldn't it be better to call this variable mPlayingChildren?

Hi Kai-Zhen, Could you help to check whether the patch is able to land on 2.0M? Thanks!

There are some conflicts when try to apply patch 1-3 into v2.0m.

(In reply to Josh Cheng [:josh] from comment #83) > Hi Kai-Zhen, > Could you help to check whether the patch is able to land on 2.0M? Thanks! Josh, sorry to inform you but I don't think this bug helps the other branches except master because, it's a new gecko api and MUST operates with gaia logic, and system platform team is going to implement the gaia system part in v3, which means the gaia patch is also not ready yet. Please ping/chat with me directly if you have any question, thanks :)

Hi, Andrea, How does the AudioChannelService in the parent process communicate with its BrowserElementAudioChannel? Is the observer service still workable when this communication path is only within the parent process? Thanks a lots :)

Oh sorry Andrea, I think I have understood. The observe service could work at "across IPC border " or "within the same process", right?

Hi, Etienne, Sorry to bother you, I have some questions need your help :( Now we are doing a refactor of audio channel, the main changing of that is to change the audio competing decision from gecko to gaia. It means that the system app could control all audio channels directly. So the system app could access its child iframes, then control the audio channel via the BrowserElementAudioChannel (It's NEW!). Every iframe which had the MozBrower API will have this attribute, and here are more details if you want to know. (https://gist.github.com/evanxd/41d8e2d91c5201a42bfa) In the new architecture, every audio channel needs to hang itself on to a iframe, so that the system app could control it. The main question we met is about "Telephony" and "Ringtone", these audio channel don't belong any iframe. Could we hang these audio channel on to the iframe in CallScreenWindow? Thanks a lots :)

> Can you please explain how you decide where to enable these interfaces on? Well... anywhere we want to use this new allowedAudioChannels() method, we need to have nsIAudioChannelService. As far as I know we don't want to expose this to android yet.

(In reply to Alastor Wu [:alwu] from comment #86) > Hi, Andrea, > How does the AudioChannelService in the parent process communicate with its > BrowserElementAudioChannel? In case the app runs in the parent process we use observers and direct methods to/from AudioChannelService. Otherwise, for child processes, we use nsIBrowserElementAPI, and from there IPC. When we are in the child process, we get/set data from/to the local AudioChannelService. The notification happens in TabChild where we send messages to the TabParent when the local AudioChannelService dispatches notifications. From TabParent, we go to BrowserElementAudioChannel.

Patch 3 has been merged in patch 1.

I think the patches are ready to be used. Yesterday I had a quick talk with sicking and with ehsan about the "next steps" and both agree that we cannot land these patches to m-c until the system app is ready to replace the existing audiochannel policy. What I think we should do is: 1. file a bug for the FMRadio permission - the patch is not strictly related with this bug. And I guess we can land the patch immediately. 2. We should start implementing the new audiochannel policy in the system app with a custom build with these patches included. When the system app is ready we can land all in once. How does it sound?

It sounds great! I think that Dominic and Evan will is starting to implement the gaia part, we can wait for their news. Now I am studying the telephony part on B2G (See comment88). Thanks for the hard works :)

(In reply to Alastor Wu [:alwu] from comment #88) > Hi, Etienne, > Sorry to bother you, I have some questions need your help :( No problem at all :) > In the new architecture, every audio channel needs to hang itself on to a > iframe, so that the system app could control it. The main question we met is > about "Telephony" and "Ringtone", these audio channel don't belong any > iframe. Well the telephony channel is mainly used by the callscreen, and the callscreen already lives in a mozbrowser iframe [1]. Is the issue coming from [2]? And the ringtones I know about are coming from the system app iframe itself [3]. > > Could we hang these audio channel on to the iframe in CallScreenWindow? I'm not sure I understand what "hanging these audio channel" involves... Do we need to remove the `mozAudioChannelType` uses in the sytem app and instead pause all channels when the system app needs to make noise? [1] https://github.com/mozilla-b2g/gaia/blob/2535321f1bd55e68fd52291b193693a8995f8e62/apps/system/js/callscreen_window.js#L94-99 [2] https://github.com/mozilla-b2g/gaia/blob/2535321f1bd55e68fd52291b193693a8995f8e62/apps/system/js/notifications.js#L529 [3] https://github.com/mozilla-b2g/gaia/blob/2535321f1bd55e68fd52291b193693a8995f8e62/apps/system/js/dialer_agent.js#L51

Hi, Etienne, As I know, the ringer is created by the DialerAgent, so the owner of the DialerAgent is the system app, not the CallScreenWindow, right? If so, the ringtone audio channel can't be managed by system app, because the system app can't get its audio channel via MozBrowserAPI (this can only be used by the parent of the system app). The CallScreenWindow is a iframe which is the child of the system app. So If we let the ringer created by CallScreenWindow, we can use system app to access its children, then use MozBrowserAPI to get the children's audio channel. Is that right? ------------------- Could you help me to explain the code [1]? I have no idea about it. As I know, the telephony audio channel is created by TelephonyService, so it doesn't need an audio element. ------------------- > I'm not sure I understand what "hanging these audio channel" involves... > Do we need to remove the `mozAudioChannelType` uses in the sytem app and instead pause all channels > when the system app needs to make noise? Yes, I think we should make all audio channels belong to the children iframes of system app, instead of using the audio channel in system app directly. That is what I mean "hanging these audio channel on to inframe". Thanks for your help :) [1] https://github.com/mozilla-b2g/gaia/blob/2535321f1bd55e68fd52291b193693a8995f8e62/apps/system/js/notifications.js#L529

Hi, Baku, I observe that the ringtone have two AudioChannelAgent, does it make sense? We enable the telephony/ringtone by |SetPhoneState()| to create a AudioChannelAgent which is not related to any window, then register this agent to the AudioChannelService. But the dialer agent (in gaia/systemApp) had a ringtione object (HTMLAudioElement), it would created another AudioChannelAgent for ringtone when we call ringtone.play(). Could we remove the code here [1]? Don't create agents by AudioManager. Let gaia create the proper audio element (ringtone/telephony), then create the agent by audioElement.play(). I think it could also solve the problem that the system app couldn't manager audio channels of ringtones/telephony because of lacking correct windowID. How do you think? Thanks a lots :) ! [1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.cpp?from=audiomanager.cpp&case=true#639

(In reply to Alastor Wu [:alwu] from comment #96) > Hi, Etienne, > > As I know, the ringer is created by the DialerAgent, so the owner of the > DialerAgent is the system app, not the CallScreenWindow, right? yes > > If so, the ringtone audio channel can't be managed by system app, because > the system app can't get its audio channel via MozBrowserAPI (this can only > be used by the parent of the system app). that's what I thought > > The CallScreenWindow is a iframe which is the child of the system app. So If > we let the ringer created by CallScreenWindow, we can use system app to > access its children, then use MozBrowserAPI to get the children's audio > channel. > > Is that right? yep > > ------------------- > > Could you help me to explain the code [1]? > I have no idea about it. As I know, the telephony audio channel is created > by TelephonyService, so it doesn't need an audio element. > If you get an sms while on a call, we want to play the sms notification sound through the earpiece. But maybe we should just use the notification channel all the time and let gecko be smart about it :) > ------------------- > > > I'm not sure I understand what "hanging these audio channel" involves... > > Do we need to remove the `mozAudioChannelType` uses in the sytem app and instead pause all channels > > when the system app needs to make noise? > > Yes, I think we should make all audio channels belong to the children > iframes of system app, instead of using the audio channel in system app > directly. That is what I mean "hanging these audio channel on to inframe". Ok makes sense. For the telephony stuff (ringer, call etc...) it sounds reasonable to do so. But for notifications I'm not sure... The general notification sound should definitely not be managed by the callscreen and we probably don't want to create an iframe just for it... Can we come up with a design where the system app itself would still be able to play on the notification channel (and only there)?

Hi, Etienne, I think you are right! Today I discuss this with Alive and Dominic. We think that the notification is a special audio, if we change it onto other child iframe, that may result some big regression errors. Maybe we could discuss this issue after finishing the audio channel refactor? (if necessary) Finally, very appreciate your help :)

Already found the answer :)

I investigated how to enable the new API by pref. It turned out that it's complex and it require a lot of work for these reasons: 1. AudioChannelService interface is different in the new API. 2. IPC protocol is changed a lot. 3. nsIAudioChannelAgent has a different interface. Before starting working on it, I want to know if the backing-out the patches a valid option. This is the easiest solution.

Hi, Steven, How do you think about that? Is the backing-out patches a valid option? Thanks!

After discussing with alwu and evanxd, here is our plan. 1. Bug 110082 will implement the competing logics in system app. After bug 110082 is landed, Gaia can still work well with existing AudioChannel(that's to pass all the scenarios in the UX spec). 2. After 110082 is done, we will check whether the new AudioChannel and new system app can pass the spec. 3. (a) Ask Gaia developers who had ever added some work-arounds for audio to remove them. (b)For some conditions that are not in the spec, we will ask UX to add them into the spec when we encounter. (c) If there're some fatal bugs found and we need to back-out the patch, we can still ensure Gaia and Gecko works well. baku, how do you think?

(In reply to StevenLee[:slee] from comment #103) > After discussing with alwu and evanxd, here is our plan. > > 1. Bug 110082 will implement the competing logics in system app. After bug Sorry, it should be bug 1100822. > 110082 is landed, Gaia can still work well with existing AudioChannel(that's > to pass all the scenarios in the UX spec). > 2. After 110082 is done, we will check whether the new AudioChannel and new > system app can pass the spec. > 3. (a) Ask Gaia developers who had ever added some work-arounds for audio to > remove them. > (b)For some conditions that are not in the spec, we will ask UX to add > them into the spec when we encounter. > (c) If there're some fatal bugs found and we need to back-out the patch, > we can still ensure Gaia and Gecko works well. > > baku, how do you think?

> baku, how do you think? Sounds good to me. So we don't need to have the pref on/off for the old api. When do you think we can land these patches?

(In reply to Andrea Marchesini (:baku) from comment #105) > Sounds good to me. So we don't need to have the pref on/off for the old api. yes, bug 1100822 will take care of the API issues. It means that Gaia should able to detect whether gecko is using new or old APIs and behaves correctly. > When do you think we can land these patches? After bug 1100822 is landed, we can land these patches.

Sorry Ehsan, but you reviewed the rest of the patches... 1 more review please!

Hi, Baku, I tested your patches on the latest Gecko, the crash problem is solved. https://hg.mozilla.org/mozilla-central/rev/2f5c5ec1a24b But there happened another issue, I can't switch the website into background. I open a website using "searching" or "entering URL", there is nothing happened when I press the home key. Do you have any idea? Thanks :)

> Do you have any idea? I don't think this is related with the AudiochannelService :)

Hi, Baku, Is it possible that the patches result some the IPC errors in passing events? This issue happened after merging your patches, it didn't happen in the original codebase. But...I am not sure whether I lost something when merging your patches... Do you have this issue in the lastest Gecko version? Thanks :)

With the latest m-c and the latest gaia, I cannot reproduce this issue.

Comment on attachment 8563126 [details] [diff] [review] patch 3 - right management of audiochannel-activity events in child processes. Review of attachment 8563126 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabChild.cpp @@ +1004,5 @@ > + if (!window) { > + return NS_OK; > + } > + > + uint64_t windowID; Nit: please initialize to 0. @@ +1017,5 @@ > + return NS_OK; > + } > + > + nsAutoString activeStr(aData); > + bool active = activeStr.Equals(NS_LITERAL_STRING("active")); Nit: EqualsLiteral.

Comment on attachment 8583002 [details] [diff] [review] patch 4 - b2g muted by default Review of attachment 8583002 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, Thanks! ::: modules/libpref/init/all.js @@ +4553,5 @@ > // loaded without sandboxing even if this pref is changed. > pref("media.gmp.insecure.allow", false); > #endif > + > +pref("dom.audiochannel.mutedByDefault", false); I suggest to add a comment explaining the usage.

Hi, Baku, I found that we forgot to implement the volume changing of FM. Could you help me review it? Thanks a lots :)

Comment on attachment 8586030 [details] [diff] [review] patch 5 - Changing FM volume Review of attachment 8586030 [details] [diff] [review]: ----------------------------------------------------------------- The patch is good, but I guess we should find a different/better approach. ::: dom/fmradio/FMRadio.cpp @@ +473,1 @@ > // TODO: what about the volume? remove this comment. ::: dom/fmradio/FMRadioService.cpp @@ +49,5 @@ > +// standard. Then, we multiply the ratio to this standard to get the result we > +// want. If we don't keep the original volume, the volume would be decrease when > +// we send the he ratio which is smaller than 1. We also store the previous > +// input ratio to check whether user use the hw button to change the hw volume. > +float gPreviousVolumoRatio = 0.0; Volume @@ +1267,5 @@ > } > > +bool > +FMRadioService::ChangedVolumeFromHardwareButton(int aIndex, > + float aPreRatio, indentation @@ +1285,5 @@ > + audioManager->GetAudioChannelVolume((int32_t)AudioChannel::Content, &index); > + > + // If the hardware volume is changed by pressing hw button, we need to save > + // it to gPreviousVolumeIndex. > + if (ChangedVolumeFromHardwareButton(index, you don't need a method for this. Do: if (index != gPreviousVolumeRatio * gPreviousVOlumeIndex) { @@ +1292,5 @@ > + gPreviousVolumeIndex = index; > + } > + > + index = gPreviousVolumeIndex * aVolume; > + audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Content, index); but in this way you are changing the volume of any other audio of Content. Correct? It means that if we have radio AND a music player here you are changing the volume of both. It seems to me that we are introducing bad issues here... Can we figure out a different approach? For instance: 1. are we sure there is no way to change the FM audio stream before sending it to the output? 2. can we have a custom audiochannel volume for the radio?

(In reply to Andrea Marchesini (:baku) from comment #117) > ::: dom/fmradio/FMRadio.cpp > remove this comment. > > ::: dom/fmradio/FMRadioService.cpp > @@ +49,5 @@ > Volume > > @@ +1267,5 @@ > indentation Sorry for these mistakes. > @@ +1285,5 @@ > > + audioManager->GetAudioChannelVolume((int32_t)AudioChannel::Content, &index); > > + > > + // If the hardware volume is changed by pressing hw button, we need to save > > + // it to gPreviousVolumeIndex. > > + if (ChangedVolumeFromHardwareButton(index, > > you don't need a method for this. Do: > > if (index != gPreviousVolumeRatio * gPreviousVOlumeIndex) { OK. > @@ +1292,5 @@ > > + gPreviousVolumeIndex = index; > > + } > > + > > + index = gPreviousVolumeIndex * aVolume; > > + audioManager->SetAudioChannelVolume((int32_t)AudioChannel::Content, index); > > but in this way you are changing the volume of any other audio of Content. > Correct? > It means that if we have radio AND a music player here you are changing the > volume of both. > It seems to me that we are introducing bad issues here... > > Can we figure out a different approach? > For instance: > > 1. are we sure there is no way to change the FM audio stream before sending > it to the output? > 2. can we have a custom audiochannel volume for the radio? Because now I am not very familiar with the detail of the FM implementation in the B2G, I need to check whether we have another way to change its volume. I would update more information after checking! Thanks!

Hi, Baku, > 1. are we sure there is no way to change the FM audio stream before sending > it to the output? I think that is the only way we can change its volume. In media element, we can adjust the volume via the dom::AudioStream. However, the FM doesn't create any stream in Gecko, and it directly passes the data to the hw audio output. > 2. can we have a custom audiochannel volume for the radio? If we want to do that, we can modify the AudioManager::SetAudioChannelVolume(). When the FM is on, only change the FM stream, not to modify the music one. But, there exist a buggy code. Now we use the MUSIC type stream in the hw audio output to play the FM instead of FM type. That results the code here is totally useless. > @ AudioManager.cpp, AudioManager::SetAudioChannelVolume() > if (IsDeviceOn(AUDIO_DEVICE_OUT_FM)) { > status = SetStreamVolumeIndex(AUDIO_STREAM_FM, aIndex); > NS_ENSURE_SUCCESS(status, status); > } If we change the FM sound to the FM stream, we can keep them with different volume. But I don't know whether the their volume doesn't sync is the problem...? Thanks!!

Hi, Baku, > but in this way you are changing the volume of any other audio of Content. Correct? > It means that if we have radio AND a music player here you are changing the volume of both. > It seems to me that we are introducing bad issues here... What is your hoping situation? Separate the volume changing of music and FM? In the UX sound spec, the music & FM & video is the same thing, and they should share the same volume. The windowVolumeChange() of FM is only to be used at "volume fading" situation, and we will also not got the situation both music and FM is playable. So..I think maybe we can change their volume together...? What do you think? Thanks :)

I'm not an expert of /dev/radio0, so maybe what I'm saying is totally wrong, but, can we use V4L2_CID_AUDIO_VOLUME ? We should check if we have that control for our sRadioFD, but in case we can use it, right? I'm reading http://v4l-test.sourceforge.net/spec/x542.htm

Hi, Baku, Sorry, I have some questions... I think that should not involved too much the details of FM, because we just adjust the volume, don't change the functionality of FM, right? Why we need to use "V4L2_CID_AUDIO_VOLUME"? I think that is used on the driver instead of Gecko...? We just want to control the volume on the Gecko side, why we need to check the sRadioFD? What do you think :) ? Thanks a lots !

Hi Alastor, that was just an idea. But I think it can work. Basically, when we change the volume of a MediaElement, we don't change the global volume for that audiochannel, but we change the volume of that particular element. The same for the WebAudio. With your approach we change the volume for that channel. What about this scenario: 1. appA is using FMRadio - content audiochannel. It's in foreground and it's playing unmuted. 2. appB is using a MediaElement - content audioChannel and it's in background muted. Now the user switches from appA to appB. With your patch we cannot do a smoothly audio transition, scaling the volume of appA from 1 to 0 and in the meantime appB from 0 to 1. This because your patch, as far as I remember, touches the content AudioChannel volume and that modifies the volume of both apps. Is it correct? I guess that FMRadio must be able to change it's own volume if this feature is supported by the devices. BTW: I guess this should not block us. When the system app is ready we can land what we have. The FMRadio volume has been always somehow broken. We can fix this as follow up. What do you think?

It turned out that V4L2_CID_AUDIO_VOLUME is not supported by the driver in flame. So my proposal cannot actually applicable.

Hi, Michael, Sorry to bother you, I have some questions need your help! I found that the refCount of FM radio is "Music" stream in AudioFlinger instead of "FM" stream. Could we change it to the FM stream? We want to control their volume independently, so I hope to find a solution to change its audio stream. If this question is not related to you, do you know someone I can ask? Very appreciate :)

Hi, Baku, > Now the user switches from appA to appB. With your patch we cannot do a > smoothly audio transition, scaling the volume of appA from 1 to 0 and in the > meantime appB from 0 to 1. This because your patch, as far as I remember, > touches the content AudioChannel volume and that modifies the volume of both > apps. > Is it correct? Yes, you're right. We indeed need to find another way. > I guess that FMRadio must be able to change it's own volume if this feature > is supported by the devices. Okay. > BTW: I guess this should not block us. When the system app is ready we can > land what we have. > The FMRadio volume has been always somehow broken. We can fix this as follow > up. What do you think? Okay. Since this issue is not very serious, we can fix it after landing the refactor patches. Thanks!

> If this question is not related to you, do you know someone I can ask? > Very appreciate :) Funny, I didn't know Michael has a bugzilla account and I just wrote an email to him asking exactly the same thing.

Is the AudioChannelManager the last dependence we have? Do you think we can land the code?

Hi, Baku, Dominic & Evan would discuss whether there are other dependence bugs we need to wait for. I would update info ASAP after they have result.

Before landing this, we should solve the following gaia issues. (1) Manage the sound of the system app (ex. notification) (2) Move the ringtone player from the system app to the ringtone app. (3) Add ownAudioChannel() in Gaia (4) Change the keyboard and lockscreen to the system type (5) Remove some workarounds

alwu, if you need them, I have all these patches rebased on top of the current m-c.

It's great! Could you update these patches? Very appreciate :)

(In reply to Alastor Wu [:alwu] from comment #131) > Before landing this, we should solve the following gaia issues. > (1) Manage the sound of the system app (ex. notification) > (2) Move the ringtone player from the system app to the ringtone app. > (3) Add ownAudioChannel() in Gaia > (4) Change the keyboard and lockscreen to the system type > (5) Remove some workarounds Are there bugs for all these issues? If so, can you add them as blocker here?

Some bugs are still not filed yet. It is just our preliminary discussion result. If there is any updating, I would set the blocker.

alwu, do we have other dependences to fix?

rebased. The other patches are ok.

Comment on attachment 8612813 [details] [diff] [review] patch 5 - nsTObserverArray instead hashtables Review of attachment 8612813 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, Thanks! ::: dom/audiochannel/AudioChannelService.cpp @@ +351,1 @@ > mPlayingChildren.Enumerate(TelephonyChannelIsActiveInChildrenEnumerator, Why we still need the hash table? ::: dom/audiochannel/AudioChannelService.h @@ +14,5 @@ > #include "nsTArray.h" > > #include "AudioChannelAgent.h" > #include "nsAttrValue.h" > #include "nsClassHashtable.h" Same, do we still need the hash table? @@ +166,5 @@ > + , mChannel(aChannel) > + {} > + > + nsRefPtr<AudioChannelAgent> mAgent; > + AudioChannel mChannel; I think that adding too much new structs might be little confusing. We can add the const function |GetAudioChannelType()| in the AudioChannelAgent, and directly use it in |mAgents|.

Another glance? Thanks!

Comment on attachment 8613912 [details] [diff] [review] patch 5 - nsTObserverArray instead hashtables Review of attachment 8613912 [details] [diff] [review]: ----------------------------------------------------------------- Nice! Thanks :)

Comment on attachment 8613912 [details] [diff] [review] patch 5 - nsTObserverArray instead hashtables Can you check this patch too?

Alastor, I forgot to tell why that patch is needed :) I saw in some code this workflow: 1. MediaElement start playing 2. the agent calls RegisterAgent 3. this is the first agent for this window and we dispsatch media-playback 4. the callback stops the mediaElement 5. the agent unregisters itself 6. we send the media-playback 'inactive' but we are still into the registration of the agent and this notification is not received.

Hi, Baku, Sorry I don't understand very well :( You mean that the agent might be unregister by some callbacks during the RegisterAgent(), then the inactive notification would be sent before the active notification? Or I misunderstand your meaning..? Because I'm confused if we have sent the inactive notification, why the notification can't be received? Thanks!

Look about this code: var observer = { observe: function(subject, topic, data) { // active is received. dump("3"); } }; observerService.addObserver(observer, "media-playback", false); dump("1"); audio.play(); dump("2"); without this patch the order is: 132 because the notifications are sent synchronously. This introduces a wrong behavior because any notification code should pass through the event loop. With the patch, the order is: 123. If we don't have this patch, changing the order of the addObserver and audio.play() will not work: audio.play(); observerService.addObserver(observer, "media-playback", false); Or, think about this code: var observer = { observe: function(subject, topic, data) { // active is received. dump("3" + data + "\n"); audio.pause(); dump("4" + data + "\n"); } }; observerService.addObserver(observer, "media-playback", false); dump("1\n"); audio.play(); dump("2\n"); this could generate (if we don't pass through the event loop for some other reasons) something like: 1 3active 3inactive 4inactive 3active 2

Comment on attachment 8617420 [details] [diff] [review] patch 6 - media-playback has to be dispatched async Review of attachment 8617420 [details] [diff] [review]: ----------------------------------------------------------------- Thanks your explanation :) If all notification should be passed through the event loop, do we need to change the "default-volume-channel-changed" to another runnable?

Alastor, yes, probably that has to be dispatched in a separate runnable. But the reason of this NI is: do you think we can land this code now and fix the other 2 dependences when the code is already in m-c?

Hi, Baku, After discussion with Dominic/Evan, these dependencies could be landed in m-c soon. These bugs are needed, because they effect the audio channel controlling in the system app. Therefore, we still need to wait for a bit. Thanks!

(In reply to Andrea Marchesini (:baku) from comment #148) > Alastor, yes, probably that has to be dispatched in a separate runnable. But > the reason of this NI is: do you think we can land this code now and fix the > other 2 dependences when the code is already in m-c? Baku, Evan has finished the last two system patches and should be landing them soon after passed the try server, we will let you know asap, thanks!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/86b32f5442f2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e1ad68356a74 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/42dc5dbabdcd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/84dc903151c1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/127cde112795 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3562a09b2bf3

Backed out for a pile of Gaia test failures. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eb7e79a2c9e9&filter-searchStr=gaia

alwu, can you take care of these gaia test failures?

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa043f7e701f

I suspect the below failure was also from this bug: https://treeherder.mozilla.org/logviewer.html#?job_id=10938524&repo=mozilla-inbound

It's strange, the patch seems well. No idea why we got these failures.

Are you able to run the gaia tests locally? Here the list of test failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eb7e79a2c9e9&filter-searchStr=gaia

Ok, I found what the problem is. In the new code we don't dispatch the 'audio-channel-changed' and 'visible-audio-channel-changed' but we still have some code on gaia relaying on it. The broken test is: gaiatest/tests/functional/videoplayer/test_play_ogv_video.py but if you look for 'audio-channel-changed' and 'visible-audio-channel-changed' you will see some other files.

Hi, Baku, After discuss with some Gaia folks, I think these events can be discard. The system app knows all information about playing audio, so these codes should be implemented without these events. Therefore, "gaia_test::current_audio_channel" can be set in Gaia::AudioChannelManager. The codes about "visible-audio-channel-changed" events (in visibility_manager.js) should also be moved in Gaia::AudioChannelManager. But I still need to discuss some details with Evan to know how to modify these codes.

Hi Alastor and baku, The bug 1177254 is just fixed. You could continue to land the gecko patch. You might have three gaia ui test failures, but I'm doing disable the three tests in Bug 1179190. The patch of disabling the tests is just on r? process.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab88701ebe2 Maybe this tree is not up-to-date but I still see failures.

Taking a look of it.

For Gip tasks[1], I think we only need to wait for the tree up-to-date because I already disabled the tests in Bug 1179190. For Gij tasks[1], I saw many crash message from the gaia ui testing framework. Maybe we have crash issues here, and you guys could try to make sure the stability things of the patch for B2G desktop client. Some other tasks in Gij is not about crash, I'll take a look and file bugs. It's not easy to land the audio channel management module. But go go. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab88701ebe2

After discuss with Evan, the failures might be caused by not using up-to-date try environment. If the crash still happen, we'll continue to find the crash reason. Here is the new try-server result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb724bdccb9

Hi, Baku, I think we should make the audio and its UI consistently, its UI should be updated when we interrupt it. Maybe we can send the "volumechange" event in the HTMLMediaElement::WindowVolumeChanged()...? Thanks!

> Here is the new try-server result. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfb724bdccb9 We still have failures. Can you take a look? > Maybe we can send the "volumechange" event in the > HTMLMediaElement::WindowVolumeChanged()...? Better to send a notification to the window. How does it sound? But definitely it's a follow-up.

Sure, I'm still looking for the crash root cause.

Can we disable that test completely, land the code, and then re-enable the test in a follow-up?

Hi baku, Sorry, we cannot disable the tests then follow up. Somehow the patch causes the below security issues. ``` JavaScript Error: "SecurityError: The operation is insecure." {file: "app://system.gaiamobile.org/js/app_window.js" line: 859} ``` The statement of line 859 in `app_window.js` is `var audioChannels = this.browser.element.allowedAudioChannels;`. I don't know we have security issue there. But looks like we need to do some security things in gecko. How do you think? [1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window.js#L865

And that is one reason why we have these test failures[1]. 09:53:33 INFO - /home/worker/gaia/apps/verticalhome/test/marionette/bookmark_uninstall_test.js failed. Will retry. 09:54:09 INFO - .....[marionette-mocha] 1 failing 09:54:09 INFO - [marionette-mocha] 09:54:09 INFO - [marionette-mocha] 1) Vertical - Bookmark Uninstall "before each" hook: 09:54:09 INFO - NoSuchElement: NoSuchElement: Unable to locate element: .appWindow.active .menu-button 09:54:09 INFO - Remote Stack: 09:54:09 INFO - <none> 09:54:09 INFO - at Error.MarionetteError (/home/worker/gaia/node_modules/marionette-client/lib/marionette/error.js:94:13) 09:54:09 INFO - at Object.Client._handleCallback (/home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:485:19) 09:54:09 INFO - at /home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:519:23 09:54:09 INFO - at TcpSync.send (/home/worker/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:166:10) 09:54:09 INFO - at Object.send (/home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:463:36) 09:54:09 INFO - at Object.Client._sendCommand (/home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:512:21) 09:54:09 INFO - at Object._findElement (/home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:1337:19) 09:54:09 INFO - at Object.findElement (/home/worker/gaia/node_modules/marionette-client/lib/marionette/client.js:1390:32) 09:54:09 INFO - at Object.MarionetteHelper.waitForElement (/home/worker/gaia/node_modules/marionette-helper/index.js:139:19) 09:54:09 INFO - at Object.appChromeContextLink (/home/worker/gaia/apps/system/test/marionette/lib/system.js:156:31) 09:54:09 INFO - at Object.Bookmark.openAndSave (/home/worker/gaia/apps/system/test/marionette/lib/bookmark.js:75:16) 09:54:09 INFO - at Context.<anonymous> (/home/worker/gaia/apps/verticalhome/test/marionette/bookmark_uninstall_test.js:37:14) 09:54:09 INFO - at callFn (/home/worker/gaia/node_modules/mocha/lib/runnable.js:223:21) 09:54:09 INFO - at Hook.Runnable.run (/home/worker/gaia/node_modules/mocha/lib/runnable.js:216:7) 09:54:09 INFO - at next (/home/worker/gaia/node_modules/mocha/lib/runner.js:258:10) 09:54:09 INFO - at /home/worker/gaia/node_modules/mocha/lib/runner.js:270:7 09:54:09 INFO - at done (/home/worker/gaia/node_modules/mocha/lib/runnable.js:185:5) 09:54:09 INFO - at /home/worker/gaia/node_modules/mocha/lib/runnable.js:226:31 09:54:09 INFO - at /home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/lib/core.js:33:15 09:54:09 INFO - at flush (/home/worker/gaia/node_modules/marionette-js-runner/node_modules/promise/node_modules/asap/asap.js:27:13) 09:54:09 INFO - at process._tickCallback (node.js:442:13) [1]: https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/M0y9QWugQP61l0bDkrQRMA/2/public/logs/live_backing.log

Hi, Baku, The test case failures might be caused by that we return the "NS_ERROR_DOM_SECURITY_ERR" in nsBrowserElement::GetAllowedAudioChannel. Here is an example fail case, If we enter the search keyword in the rocket bar[1], the new iframe can't be open after we press the enter key. [1] The search bar fixed on the screen top in B2G.

If I don't return the NS_ERROR_DOM_SECURITY_ERR in nsBrowserElement::GetAllowedAudioChannel, the case mentioned in comment173 can be executed successfully. Here is the try-server result, we can verify whether the actual root cause is returning "NS_ERROR_DOM_SECURITY_ERR". https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b2bcb08c931

When we use the rocket bar, this procedure would generate two browser apps. The first one is the searching window [1], and second one is the result window [2]. The second window can't be regard as the app, so we return the NS_ERROR_DOM_SECURITY_ERR. --- The second window couldn't get the vaild |app| variable, but I don't know the reason. > In nsBrowserElement.cpp @ GetAllowedAudioChannel() > nsCOMPtr<mozIApplication> app; > aRv = appsService->GetAppByManifestURL(manifestURL, getter_AddRefs(app)); > if (NS_WARN_IF(aRv.Failed())) { > return; > } Baku, do you have any idea? Thanks! [1] https://drive.google.com/file/d/0B8z53fPg4O7NTGxQakVpZkFJejg/view?usp=sharing [2] https://drive.google.com/file/d/0B8z53fPg4O7NdVlkWDVZZE80Skk/view?usp=sharing

> > aRv = appsService->GetAppByManifestURL(manifestURL, getter_AddRefs(app)); Print what manifestURL is. If it's an empty string, then it seems we have a different problem.

Hi, Baku Here are two things. (1) About NS_ERROR_DOM_SECURITY_ERR After discuss with Alive, in present design, the browser do/should not have the manifestURL. Therefore, the system app would have two kinds of different iframes, one is "app" and another is only an "iframe". I think that we should remove these codes, > if (!noapp && !app) { > aRv.Throw(NS_ERROR_DOM_SECURITY_ERR); > return; > } Even if the iframe is not an app, we should return the "normal" audio channel. (2) dom.testing.browserElementAudioChannel.noapp Why we need this preference? Thanks :)