Andrea Marchesini [:baku] Assignee
	Description • 11 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 11 years ago

This horrible addon has been written just for testing this feature. The UI doesn't exist and the code is copy and paste from another addon, hacking its code.

	(no longer active)
	Comment 3 • 11 years ago

Comment on attachment 813236 [details] [diff] [review]
patch

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

This looks very good overall.  r=me if this works for media elements that are not in the DOM (and you should add a test for that too!)

::: dom/base/nsPIDOMWindow.h
@@ +12,5 @@
>  
>  #include "nsCOMPtr.h"
>  #include "nsAutoPtr.h"
>  #include "nsTArray.h"
> +#include "nsClassHashtable.h"

You shouldn't need this.

@@ +194,5 @@
> +  // Audio API
> +  void MediaElementActive(mozilla::dom::HTMLMediaElement* aElement);
> +  void MediaElementInactive(mozilla::dom::HTMLMediaElement* aElement);
> +
> +  bool GetAudioMuted()

Nit: const everywhere you can (I'm a const freak!)

@@ +760,5 @@
> +
> +  // List of the active Media Elements.
> +  nsTArray<nsRefPtr<mozilla::dom::HTMLMediaElement> > mActiveMediaElements;
> +
> +  bool mAudioMuted;

Please move this bool up for better packing.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1409,5 @@
> +     * with volume level 0.8 and this iframe has a parent window with
> +     * volume level 0.5, the real volume level of the MediaElement will be
> +     * 0.5 * 0.8 = 0.4.
> +     */
> +    attribute float audioVolume;

Please document why you have both audioMuted and audioVolume (for preserving the volume across mute/unmute I expect.)

	(no longer active)
	Comment 6 • 11 years ago

Comment on attachment 813515 [details] [diff] [review]
patch 2 - notifications

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

::: dom/base/nsGlobalWindow.cpp
@@ +3422,5 @@
> +    nsCOMPtr<nsIObserverService> observerService =
> +      services::GetObserverService();
> +    if (observerService) {
> +      observerService->NotifyObservers(ToSupports(this), "audio-active",
> +                                       nullptr);

Please call this "media-playback" and pass in "activate" or "deactivate" as the data.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 8 • 11 years ago

Comment on attachment 813516 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1712,5 @@
> +
> +  if (globalMuted) {
> +    SetMutedInternal(mMuted | MUTED_BY_WINDOW);
> +  } else {
> +    SetMutedInternal(mMuted & ~MUTED_BY_WINDOW);

Why do we need GetAudioGlobalMuted? Can't we just have the window return 0 for GetAudioGlobalVolume?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 10 • 11 years ago

Comment on attachment 816429 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

What about IPC? Shouldn't we honor window volumes set on cross-process ancestor windows?

::: content/html/content/src/HTMLMediaElement.cpp
@@ +1713,5 @@
> +  if (globalMuted) {
> +    SetMutedInternal(mMuted | MUTED_BY_WINDOW);
> +  } else {
> +    SetMutedInternal(mMuted & ~MUTED_BY_WINDOW);
> +  }

OK, why are we setting this flag? Nothing uses it right?

::: dom/base/nsGlobalWindow.cpp
@@ +3454,5 @@
> +      bool browserOrApp;
> +      if (!docShell ||
> +          NS_FAILED(docShell->GetIsBrowserOrApp(&browserOrApp)) ||
> +          browserOrApp) {
> +        continue;

Why are we doing this check?

@@ +3477,5 @@
> +
> +  if (IsInnerWindow()) {
> +    RefreshMediaElements();
> +  } else {
> +    mInnerWindow->SetAudioMuted(aMuted);

Why don't we just delegate to the inner window immediately before doing anything else?

@@ +3485,5 @@
> +nsresult
> +nsPIDOMWindow::SetAudioVolume(float aVolume)
> +{
> +  if (aVolume < 0.0 || aVolume > 1.0) {
> +    return NS_ERROR_DOM_INDEX_SIZE_ERR;

I think volumes > 1.0 should be allowed.

@@ +3497,5 @@
> +
> +  if (IsInnerWindow()) {
> +    RefreshMediaElements();
> +  } else {
> +    mInnerWindow->SetAudioVolume(aVolume);

As above. Why not just delegate to the inner window directly?

@@ +3518,5 @@
> +    return aVolume * mAudioVolume;
> +  }
> +
> +  float parentVolume = parent->GetAudioVolume();
> +  return parent->GetAudioGlobalVolumeInternal(parentVolume * aVolume);

I think it would be better to use a loop instead of making recursive calls. It is probably no more code and it's more efficient.

::: dom/base/nsPIDOMWindow.h
@@ +191,5 @@
>    }
>  
> +  // Audio API
> +  void MediaElementActive(mozilla::dom::HTMLMediaElement* aElement);
> +  void MediaElementInactive(mozilla::dom::HTMLMediaElement* aElement);

Add a "typedef mozilla::dom::HTMLMediaElement HTMLMediaElement;" at the top of this class so we don't have to use prefixes here and below.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1406,5 @@
> +   attribute boolean audioMuted;
> +
> +    /**
> +     * range: 0.0 to 1.0. The real volume level is calculated by any
> +     * parent window. So if the MediaElement is contained by a iframe

"The real volume level is affected by the volume of all ancestor windows."

	Andrea Marchesini [:baku] Assignee
	Comment 11 • 11 years ago

> What about IPC? Shouldn't we honor window volumes set on cross-process
> ancestor windows?

I would do this in a follow up.

> > +  if (globalMuted) {
> > +    SetMutedInternal(mMuted | MUTED_BY_WINDOW);
> > +  } else {
> > +    SetMutedInternal(mMuted & ~MUTED_BY_WINDOW);
> > +  }
> 
> OK, why are we setting this flag? Nothing uses it right?

Mute in HTMLMediaElement can be set for many reasons: user, audioChannel, and now by window.
If 1 of them is set, the mute is active. mMuted is a bit map and MUTED_BY_WINDOW is a new ID for this map.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 12 • 11 years ago

Yeah but why do we need to set the mute flag here? If we don't set it, everything will still work and the tab will be silent, right?

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 11 years ago

Roc, I'm still working on a issue with the innerWindow. I have a crash on try. So I'll ask you a full review once this patch will work completely.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 14 • 11 years ago

Comment on attachment 817849 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

If you don't mind I'd prefer to just do the full review when it's ready.

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 10 years ago

FMRadio is not implemented and WebAudio is probably to be improved.

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 10 years ago

Just moved to AudioChannelService.

	(no longer active)
	Comment 17 • 10 years ago

Comment on attachment 8385508 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

Looks good, but I'd like roc to also look at this.

::: content/html/content/src/HTMLAudioElement.cpp
@@ +247,5 @@
>  
>  NS_IMETHODIMP
> +HTMLAudioElement::VolumeChanged(float aVolume)
> +{
> +  NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE);

This is paranoia, right?  I meant there should be no way for content to call this code.  Maybe we should add a MOZ_ASSERT too?

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +382,5 @@
>  
> +NS_IMETHODIMP
> +AudioDestinationNode::VolumeChanged(float aVolume)
> +{
> +  SendDoubleParameterToStream(DestinationNodeEngine::VOLUME, aVolume);

Talked to Andrea about this on IRC, he'll make this better in another patch.

::: content/media/webaudio/AudioDestinationNode.h
@@ +62,5 @@
>    NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
>  
>    // nsIAudioChannelAgentCallback
>    NS_IMETHOD CanPlayChanged(int32_t aCanPlay);
> +  NS_IMETHOD VolumeChanged(float aVolume);

MOZ_OVERRIDE here and elsewhere please.

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +188,5 @@
> +void
> +AudioChannelAgent::WindowVolumeChanged()
> +{
> +  nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback();
> +  if (!callback || !mWindow) {

Please drop the mWindow check here.  The win check below takes care of this as well.

::: dom/audiochannel/AudioChannelService.cpp
@@ +792,5 @@
> +}
> +void
> +AudioChannelService::RefreshAgentsVolume()
> +{
> +  mAgents.EnumerateRead(RefreshAgentsVolumeEnumerator, nullptr);

This will break if some day a WindowVolumeChanged callback ends up modifying mAgents.

Perhaps we should consider copyiong these agents into an array on the stack and then iterate on it and call WindowVolumeChanged() on each item.

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +5,5 @@
>  #include "nsISupports.idl"
>  
>  interface nsIDOMWindow;
>  
> +[scriptable, uuid(86975108-cd78-4796-8fc8-6cd777cd6eba)]

Please rev the uuid.

@@ -5,5 @@
>  #include "nsISupports.idl"
>  
>  interface nsIDOMWindow;
>  
> -[function, scriptable, uuid(86975108-cd78-4796-8fc8-6cd777cd6eba)]

Is naIAudioChannelAgentCallback used as a function somewhere (maybe add-ons)?  If yes, this will be a breaking change.  (Not that _I_ care much about that!)

::: dom/base/test/test_audioWindowUtils.html
@@ +43,5 @@
> +
> +  utils.audioVolume = 0;
> +  is(utils.audioVolume, 0.0, "utils.audioVolume is ok");
> +  utils.audioVolume = 1.0;
> +  is(utils.audioVolume, 1.0, "utils.audioVolume is ok");

Can you please test nested frames?

	(no longer active)
	Comment 18 • 10 years ago

Comment on attachment 8385524 [details] [diff] [review]
patch 2 - notifications

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

::: dom/audiochannel/AudioChannelService.cpp
@@ +113,5 @@
>    mAgents.Put(aAgent, data);
>    RegisterType(aType, CONTENT_PROCESS_ID_MAIN, aWithVideo);
> +
> +  uint32_t count = CountWindow(aAgent->Window());
> +  if (count == 1) {

Please add a comment to explain the counting logic here and below at |count == 0|.

::: dom/base/test/test_audioNotification.html
@@ +15,5 @@
> +
> +var expectedNotification = null;
> +
> +var observer = SpecialPowers.wrapCallbackObject({
> +  QueryInterface : function (iid) {

Please use XPCOMUtils here.

@@ +34,5 @@
> +var observerService = SpecialPowers.Cc["@mozilla.org/observer-service;1"]
> +                                   .getService(SpecialPowers.Ci.nsIObserverService);
> +
> +var audio = new Audio();
> +audio.src = "/tests/content/html/content/test/wakelock.ogg";

It's not great to rely on files elsewhere in the tree like this.  Please copy the file into this directory and add it to the support files manifest section.

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 10 years ago

This patch has been already reviewed by mchen for bug 876631

	Paul Adenot (:padenot)
	Comment 22 • 10 years ago

I'm not sure what info you need from me, so I'll just cheer on the side :-)

Feel free to ni? me again if you have a question.

	Andrea Marchesini [:baku] Assignee
	Comment 23 • 10 years ago

Here the question for you:

With this patch we can change the volume of a window and any media element (webaudio as well) will change their internal volume without exposing it to content. In HTMLMediaElement this works, but in WebAudio, the patch does:

  SendDoubleParameterToStream(DestinationNodeEngine::VOLUME, aVolume);

And this can be detected if the destination node is attach to other nodes. What can we do to do not expose the new volume to content?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 10 years ago

Call MediaStream::SetAudioOutputVolume on the destination node's stream (probably via a method on DestinationNode).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 25 • 10 years ago

Comment on attachment 8385826 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

::: content/html/content/public/HTMLMediaElement.h
@@ +1067,5 @@
>    };
>  
>    uint32_t mMuted;
>  
> +  float mWindowVolume;

Instead of storing this here, wouldn't it be simpler to retrieve the window volume from the window in SetVolumeInternal?

::: dom/audiochannel/nsIAudioChannelAgent.idl
@@ +22,5 @@
> +
> +  /**
> +   * Notified when the window volume/mute is changed
> +   */
> +  void volumeChanged(in float volume);

Please call this windowVolumeChanged otherwise it's confusing (especially in HTMLMediaElement where there are lots of methods that change the volume).

I don't think you need to pass the new volume, either.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 26 • 10 years ago

Comment on attachment 8385827 [details] [diff] [review]
patch 2 - notifications

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

::: dom/audiochannel/AudioChannelService.h
@@ +192,5 @@
> +  CountWindowEnumerator(AudioChannelAgent* aAgent,
> +                        AudioChannelAgentData* aUnused,
> +                        void *aPtr);
> +
> +  uint32_t CountWindow(nsIDOMWindow* aWindow);

Document these!

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 28 • 10 years ago

Comment on attachment 8386604 [details] [diff] [review]
patch 1 - audioMuted/audioVolume

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

Delicious! Just that AudioDestinationNode bug to fix :-)

::: content/media/webaudio/AudioDestinationNode.cpp
@@ +388,5 @@
> +    nsresult rv = mAudioChannelAgent->GetWindowVolume(&volume);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    SendDoubleParameterToStream(DestinationNodeEngine::VOLUME,
> +                                volume);

See comment #24.

	Andrea Marchesini [:baku] Assignee
	Comment 29 • 10 years ago

This is probably what you meant, but it doesn't mute the media element when I set volume to 0. What am I doing wrong?

	(no longer active)
	Comment 30 • 10 years ago

(In reply to comment #29)
> Created attachment 8386687 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8386687&action=edit
> patch 1 - interdiff
> 
> This is probably what you meant, but it doesn't mute the media element when I
> set volume to 0. What am I doing wrong?

This seems to be the only place mVolume is used: <http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#844>  I'm not sure why adding the volume is the right thing to do as the comment suggests, but adding 0 will be a no-op.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 31 • 10 years ago

Comment on attachment 8386687 [details] [diff] [review]
patch 1 - interdiff

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

Adding volumes is the right thing there thanks to the comment above.

The basic idea here is that each MediaStream can have independent "audio outputs", each with its own key. Anyone who wants to play MediaStream output to speakers calls AddAudioOutput with their own key. Those outputs each have an independently controllable volume.

So the problem with the interdiff is that you're adding a new audio-output for the DestinationNode and controlling its volume, while leaving the original audio output untouched. So you're just making the node louder :-).

AudioContext::AudioContext does
  mDestination->Stream()->AddAudioOutput(&gWebAudioOutputKey);
So I suggest we add a WindowVolumeChanged method to AudioContext that gets the window volume and sets it on the audio output using the gWebAudioOutputKey.

	Andrea Marchesini [:baku] Assignee
	Comment 32 • 10 years ago

Thanks for your comment.This seems a bit a ping-pong between AudioContext and AudioDestinationNode. But at least it works :)

	Andrea Marchesini [:baku] Assignee
	Comment 33 • 10 years ago

here the full patch.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 34 • 10 years ago

Comment on attachment 8387019 [details] [diff] [review]
patch 1 - interdiff

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

We could avoid this ping-ponging by moving AudioChannel stuff from the destination node to the AudioContext, or by moving the AddAudioOutput stuff to the destination node.

	Andrea Marchesini [:baku] Assignee
	Comment 35 • 10 years ago

Last quick review before landing it? I moved AddAudioOutput to the constructor of the AudioDestinationNode class.

FMRadio implementation can be a follow up.

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d86a702e24c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98f7e8789561
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d19f076a7451

	Ed Morley [:emorley]
	Comment 37 • 10 years ago

(In reply to Andrea Marchesini (:baku) from comment #36)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d86a702e24c0
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/98f7e8789561
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d19f076a7451

Backed out for build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=35892477&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e567df520fd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a49b82e21ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/34af3402920b

	Andrea Marchesini [:baku] Assignee
	Comment 38 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=d7d8865d6f10

waiting for green on try before landing it again.

	Andrea Marchesini [:baku] Assignee
	Comment 39 • 10 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/798346050542
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71d145853be8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec6116366680

	(no longer active)
	Comment 40 • 10 years ago

Yay!  This really deserves a blog post, Andrea!

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/798346050542
https://hg.mozilla.org/mozilla-central/rev/71d145853be8
https://hg.mozilla.org/mozilla-central/rev/ec6116366680

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 42 • 10 years ago

After glancing through the patches, I take it that this adds new functionality to the backend, but there's no UI for it yet?

	Andrea Marchesini [:baku] Assignee
	Comment 43 • 10 years ago

Correct. No UI yet.

	(no longer active)
	Comment 45 • 9 years ago

Comment on attachment 8629601 [details]
UX spec by shorlander

Oops, wrong bug!