1113086 - Implement AudioChannel API into BrowserElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: WIP

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.

Comment 2

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

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?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: patch 1 - remove the existing logic from AudioChannelService

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

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - IDL

This second patch exposes some IDL methods.

Comment 5

[Andrea Marchesini [:baku]]

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.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch 1 - remove the existing logic from AudioChannelService

Changed many things.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 2 - IDL

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 3 - Audio in BrowserElement API

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 1 - remove the existing logic from AudioChannelService

wrong patch.

Comment 10

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

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 11

[Kan-Ru Chen [:kanru] (UTC+9)]

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?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch 3 - Audio in BrowserElement API

Here the comment applied plus the 'notification' event.

About childID, any iframe can run 1 single app, and that has 1 single childID. Correct?

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 4 - tests

Here some tests.

Comment 14

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

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/

Comment 15

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

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.

Comment 16

[Marco Chen [:mchen]]

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.

Comment 17

[(no longer active)]

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

Comment 18

[Andrea Marchesini [:baku]]

> 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.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 5 - b2g

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

Comment 20

[Andrea Marchesini [:baku]]

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

Comment 21

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

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?

Comment 22

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

(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?

Comment 23

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

(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 24

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

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)?

Comment 25

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

(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...

Comment 26

[Andrea Marchesini [:baku]]

(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.

Comment 27

[Andrea Marchesini [:baku]]

> 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?

Comment 28

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

(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.

Comment 29

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

(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 30

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

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.

Comment 31

[Andrea Marchesini [:baku]]

(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.

Comment 32

[Andrea Marchesini [:baku]]

> 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.

Comment 33

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

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?

Comment 34

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

(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.

Comment 35

[Andrea Marchesini [:baku]]

> * 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.

Comment 36

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

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 37

[(no longer active)]

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 38

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

Comment on attachment 8540238 [details] [diff] [review]
patch 2 - IDL

As c37, clear the review for this one.

Comment 39

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

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....

Comment 40

[Andrea Marchesini [:baku]]

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.

Comment 41

[Andrea Marchesini [:baku]]

Attachment: patch

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.

Comment 42

[Andrea Marchesini [:baku]]

s/page/patch/

Comment 43

[(no longer active)]

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!

Comment 44

[Andrea Marchesini [:baku]]

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 45

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

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 46

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

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.

Comment 47

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

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?

Comment 48

[Andrea Marchesini [:baku]]

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?

Comment 49

[Andrea Marchesini [:baku]]

Attachment: byebyelogic.patch

Fixed the b2g issues.

Comment 50

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

(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.

Comment 51

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

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?

Comment 52

[Andrea Marchesini [:baku]]

(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.

Comment 53

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

(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.

Comment 54

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

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.

Comment 55

[Alastor Wu [:alwu]]

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

Comment 56

[Alastor Wu [:alwu]]

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.

Comment 57

[Alastor Wu [:alwu]]

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.

Comment 58

[Alastor Wu [:alwu]]

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

Comment 59

[Andrea Marchesini [:baku]]

> [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.

Comment 60

[Andrea Marchesini [:baku]]

(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.

Comment 61

[Andrea Marchesini [:baku]]

> 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!

Comment 62

[Alastor Wu [:alwu]]

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.

Comment 63

[Alastor Wu [:alwu]]

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 :)

Comment 64

[Andrea Marchesini [:baku]]

> 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!

Comment 65

[Alastor Wu [:alwu]]

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

================================================================================

Comment 66

[Alastor Wu [:alwu]]

Attachment: Add permission to FM app

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".

Comment 67

[Andrea Marchesini [:baku]]

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

Comment 69

[Keven Kuo [:kkuo]]

Copy flags from bug 1069887

--
Keven

Comment 70

[Alastor Wu [:alwu]]

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 71

[(no longer active)]

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 72

[Kan-Ru Chen [:kanru] (UTC+9)]

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"

Comment 73

[Andrea Marchesini [:baku]]

> > +@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.

Comment 74

[Andrea Marchesini [:baku]]

Attachment: interdiff

Comment 75

[Andrea Marchesini [:baku]]

Attachment: patch 1 - BrowserElementAudioChannel

Here the full patch.

Comment 76

[Andrea Marchesini [:baku]]

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.

Comment 77

[Andrea Marchesini [:baku]]

Attachment: patch 2 - telephony/content/normal channels are active

Comment 78

[Andrea Marchesini [:baku]]

Attachment: patch 3 - sub-iframe mochitest

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.

Comment 79

[(no longer active)]

(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 80

[(no longer active)]

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 81

[(no longer active)]

Comment on attachment 8552406 [details] [diff] [review]
patch 1 - BrowserElementAudioChannel

I didn't review this.

Comment 82

[(no longer active)]

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?

Comment 83

[Josh Cheng [:josh]]

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

Comment 84

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 85

[Dominic Kuo [:dkuo]]

(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 :)

Comment 86

[Alastor Wu [:alwu]]

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 :)

Comment 87

[Alastor Wu [:alwu]]

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

Comment 88

[Alastor Wu [:alwu]]

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 :)

Comment 89

[Andrea Marchesini [:baku]]

> 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.

Comment 90

[Andrea Marchesini [:baku]]

(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.

Comment 91

[Andrea Marchesini [:baku]]

Attachment: patch 1 - BrowserElementAudioChannel

Patch 3 has been merged in patch 1.

Comment 92

[Andrea Marchesini [:baku]]

Attachment: patch 2 - telephony/content/normal channels are active

Comment 93

[Andrea Marchesini [:baku]]

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?

Comment 94

[Alastor Wu [:alwu]]

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 :)

Comment 95

[Etienne Segonzac (:etienne)]

(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

Comment 96

[Alastor Wu [:alwu]]

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

Comment 97

[Alastor Wu [:alwu]]

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

Comment 98

[Etienne Segonzac (:etienne)]

(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)?

Comment 99

[Alastor Wu [:alwu]]

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 :)

Comment 100

[Alastor Wu [:alwu]]

Already found the answer :)

Comment 101

[Andrea Marchesini [:baku]]

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.

Comment 102

[Alastor Wu [:alwu]]

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

Comment 103

[StevenLee[:slee]]

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?

Comment 104

[StevenLee[:slee]]

(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?

Comment 105

[Andrea Marchesini [:baku]]

> 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?

Comment 106

[StevenLee[:slee]]

(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.

Comment 107

[Andrea Marchesini [:baku]]

Attachment: patch 3 - right management of audiochannel-activity events in child processes.

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

Comment 108

[Alastor Wu [:alwu]]

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 :)

Comment 109

[Andrea Marchesini [:baku]]

> Do you have any idea?

I don't think this is related with the AudiochannelService :)

Comment 110

[Alastor Wu [:alwu]]

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 :)

Comment 111

[Andrea Marchesini [:baku]]

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

Comment 112

[(no longer active)]

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 113

[Andrea Marchesini [:baku]]

Attachment: patch 3 - right management of audiochannel-activity events in child processes.

Comment 114

[Andrea Marchesini [:baku]]

Attachment: patch 4 - b2g muted by default

Comment 115

[Alastor Wu [:alwu]]

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.

Comment 116

[Alastor Wu [:alwu]]

Attachment: patch 5 - Changing FM volume

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

Comment 117

[Andrea Marchesini [:baku]]

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?

Comment 118

[Alastor Wu [:alwu]]

(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!

Comment 119

[Alastor Wu [:alwu]]

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!!

Comment 120

[Alastor Wu [:alwu]]

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 :)

Comment 121

[Andrea Marchesini [:baku]]

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

Comment 122

[Alastor Wu [:alwu]]

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 !

Comment 123

[Andrea Marchesini [:baku]]

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?

Comment 124

[Andrea Marchesini [:baku]]

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

Comment 125

[Alastor Wu [:alwu]]

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 :)

Comment 126

[Alastor Wu [:alwu]]

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!

Comment 127

[Andrea Marchesini [:baku]]

> 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.

Comment 128

[Andrea Marchesini [:baku]]

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

Comment 129

[Alastor Wu [:alwu]]

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.

Comment 131

[Alastor Wu [:alwu]]

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

Comment 132

[Andrea Marchesini [:baku]]

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

Comment 133

[Alastor Wu [:alwu]]

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

Comment 134

[Andrew Overholt [:overholt]]

(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?

Comment 135

[Alastor Wu [:alwu]]

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

Comment 136

[Andrea Marchesini [:baku]]

alwu, do we have other dependences to fix?

Comment 137

[Andrea Marchesini [:baku]]

Attachment: patch 1 - BrowserElementAudioChannel

rebased. The other patches are ok.

Comment 138

[Andrea Marchesini [:baku]]

Attachment: patch 5 - nsTObserverArray instead hashtables

Comment 139

[Alastor Wu [:alwu]]

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|.

Comment 140

[Andrea Marchesini [:baku]]

Attachment: patch 5 - nsTObserverArray instead hashtables

Another glance? Thanks!

Comment 141

[Alastor Wu [:alwu]]

Comment on attachment 8613912 [details] [diff] [review]
patch 5 - nsTObserverArray instead hashtables

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

Nice! Thanks :)

Comment 142

[Andrea Marchesini [:baku]]

Comment on attachment 8613912 [details] [diff] [review]
patch 5 - nsTObserverArray instead hashtables

Can you check this patch too?

Comment 143

[Andrea Marchesini [:baku]]

Attachment: patch 6 - media-playback has to be dispatched async

Comment 144

[Andrea Marchesini [:baku]]

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.

Comment 145

[Alastor Wu [:alwu]]

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!

Comment 146

[Andrea Marchesini [:baku]]

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 147

[Alastor Wu [:alwu]]

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?

Comment 148

[Andrea Marchesini [:baku]]

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?

Comment 149

[Alastor Wu [:alwu]]

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!

Comment 150

[Dominic Kuo [:dkuo]]

(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!

Comment 151

[Andrea Marchesini [:baku]]

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

Comment 152

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7e79a2c9e9

Comment 153

[Ryan VanderMeulen [:RyanVM]]

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

Comment 154

[Andrea Marchesini [:baku]]

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

Comment 155

[Pulsebot]

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

Comment 156

[Ryan VanderMeulen [:RyanVM]]

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

Comment 157

[Alastor Wu [:alwu]]

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

Comment 158

[Andrea Marchesini [:baku]]

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

Comment 159

[Andrea Marchesini [:baku]]

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.

Comment 160

[Alastor Wu [:alwu]]

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.

Comment 161

[Evan Tseng [:evanxd]]

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.

Comment 162

[Andrea Marchesini [:baku]]

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

Maybe this tree is not up-to-date but I still see failures.

Comment 163

[Evan Tseng [:evanxd]]

Taking a look of it.

Comment 164

[Evan Tseng [:evanxd]]

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

Comment 165

[Alastor Wu [:alwu]]

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

Comment 166

[Alastor Wu [:alwu]]

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!

Comment 167

[Andrea Marchesini [:baku]]

> 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.

Comment 168

[Alastor Wu [:alwu]]

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

Comment 170

[Andrea Marchesini [:baku]]

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

Comment 171

[Evan Tseng [:evanxd]]

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

Comment 172

[Evan Tseng [:evanxd]]

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

Comment 173

[Alastor Wu [:alwu]]

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.

Comment 174

[Alastor Wu [:alwu]]

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

Comment 175

[Alastor Wu [:alwu]]

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

Comment 176

[Andrea Marchesini [:baku]]

> > 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.

Comment 177

[Alastor Wu [:alwu]]

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 :)

Comment 178

[Andrea Marchesini [:baku]]

Attachment: patch 7 - last fix

Seems green on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7080605d6a4

Comment 179

[Evan Tseng [:evanxd]]

For debugging the test failures in [1], we're trying to run the tests in device locally.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7080605d6a4

Comment 180

[Alastor Wu [:alwu]]

Attachment: Log : gecko log during the local test , Gij 9

It's the log when I run the Gij test 9 on my local environment (with patch).
However, I didn't find any errors that would be possibly generated by our patches.

Comment 181

[Alastor Wu [:alwu]]

Attachment: Log : gaia log during the local test , Gij 9

Here is the gaia log.

Comment 182

[Alastor Wu [:alwu]]

In previous test[1], we would get two test case failures, which are in the test  “browser_meta_application_name_test.js ” and “trusted_window_test.js”.

Although the push result[2] from baku's latest patches looks good, we can't not sure that these failures are actually solved or might happen again (example, hitting the specific timing issue). Can we temporarily disable these tests, and re-enable them in follow-up?

For some reasons, I hope we can land it first, 
1) Because these patches involves lots of changing, and it would need to be rebased everytime when I update my m-c. However, sometime I can't rebase it successfully (build fail) or even I can't sure that the try-server errors (push from my local build) is resulted by my error rebasing or the patches itself.
2) The try result seems good.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7080605d6a4
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b63a09e96c1
---

Hi, Kevin,
Because you are the author of these tests, I think we should let you know if we can disable them temporarily.

Comment 183

[Kevin Grandon :kgrandon]

I would not recommend disabling tests.

browser_meta_application_name_test.js - This looks intermittent, as it has a bug starred. I recommend submitting to try again to see if it goes away.

trusted_window_test.js - Did you try running this locally? This does not look intermittent, so a failure here might be introducing some real breakage.

Comment 184

[Alastor Wu [:alwu]]

Attachment: Log : Gij 14

Here is the log for Gij14. 
It might be the last failure we need to fix.
I think it is similar with the previous problem, because the error message is "this._manifest is null".

Comment 185

[Andrea Marchesini [:baku]]

> I think it is similar with the previous problem, because the error message
> is "this._manifest is null".


Can you tell me how to reproduce this issue locally? Thanks

Comment 186

[Alastor Wu [:alwu]]

Hi, Baku, 
I didn't find the reproduce steps.
I just ran the marionette test on my device and then captured the log.

Comment 187

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc4dec6cd81
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/caf57ae8cbce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ec088f0892f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/380859ae955b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b9525c60a737
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb34138bb9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/675ea719b91c

Comment 188

[Pulsebot]

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

Comment 189

[Xidorn Quan [:xidorn] UTC+11]

Backed out for build bustage in https://hg.mozilla.org/integration/mozilla-inbound/rev/ee99ebac7761

https://treeherder.mozilla.org/logviewer.html#?job_id=11583165&repo=mozilla-inbound

Comment 190

[(no longer active)]

The fix was really trivial, so I'm relanding with the build issue fixed.

Comment 191

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcfbdb934c37
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f09bee844e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/768c8cd889e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/69427ebb50f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9cc17209f0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6c9832002f
https://hg.mozilla.org/integration/mozilla-inbound/rev/200c185e4d71
https://hg.mozilla.org/integration/mozilla-inbound/rev/91846db1056c

Comment 193

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=727b321502a6 since i think the changes broke the cpp tests on windows very frequently with timeouts like https://treeherder.mozilla.org/logviewer.html#?job_id=11595725&repo=mozilla-inbound

Comment 194

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/929bb7be96a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c310d868f0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0ea17d5074
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ab57a89b1f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7b81fffa9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/753b9523de08
https://hg.mozilla.org/integration/mozilla-inbound/rev/245563f697d1

Comment 195

[Andrea Marchesini [:baku]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4120dbd77a5d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/790dadba73e3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c42b01aa2632
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a636a82ae5f1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7c999bab3c45
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4d77b5fa4e8a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/699598e5f420
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc4991e7fc83

Comment 196

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/929b875fbab1

Comment 197

[(no longer active)]

(In reply to Pulsebot from comment #196)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/929b875fbab1

I removed TestAudioChannelService from cppunittest.ini.  I think you forgot to do that here, which caused the test to somehow run (presumbaly from an older build) when I landed my patch for bug 1180535 last night!

Comment 198

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4120dbd77a5d
https://hg.mozilla.org/mozilla-central/rev/790dadba73e3
https://hg.mozilla.org/mozilla-central/rev/c42b01aa2632
https://hg.mozilla.org/mozilla-central/rev/a636a82ae5f1
https://hg.mozilla.org/mozilla-central/rev/7c999bab3c45
https://hg.mozilla.org/mozilla-central/rev/4d77b5fa4e8a
https://hg.mozilla.org/mozilla-central/rev/699598e5f420
https://hg.mozilla.org/mozilla-central/rev/cc4991e7fc83
https://hg.mozilla.org/mozilla-central/rev/929b875fbab1