923247 - Nice feature to have is mute and volume per window

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: test.xpi

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.

Comment 3

[(no longer active)]

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

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch 2 - notifications

Comment 5

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

Comment 6

[(no longer active)]

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.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch 2 - notifications

Comment 8

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

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?

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

Comment 10

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

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

Comment 11

[Andrea Marchesini [:baku]]

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

Comment 12

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

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?

Comment 13

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

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.

Comment 14

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

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.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: patch 2 - notifications

Just moved to AudioChannelService.

Comment 17

[(no longer active)]

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?

Comment 18

[(no longer active)]

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.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch 2 - notifications

Comment 21

[Andrea Marchesini [:baku]]

Attachment: patch 0 - window in AudioChannelAgent

This patch has been already reviewed by mchen for bug 876631

Comment 22

[Paul Adenot (:padenot)]

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.

Comment 23

[Andrea Marchesini [:baku]]

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?

Comment 24

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

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

Comment 25

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

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.

Comment 26

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

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!

Comment 27

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

Comment 28

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

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.

Comment 29

[Andrea Marchesini [:baku]]

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

Comment 30

[(no longer active)]

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

Comment 31

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

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.

Comment 32

[Andrea Marchesini [:baku]]

Attachment: patch 1 - interdiff

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

Comment 33

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

here the full patch.

Comment 34

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

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.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: patch 1 - audioMuted/audioVolume

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

FMRadio implementation can be a follow up.

Comment 36

[Andrea Marchesini [:baku]]

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

Comment 37

[Ed Morley [:emorley]]

(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

Comment 38

[Andrea Marchesini [:baku]]

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

waiting for green on try before landing it again.

Comment 39

[Andrea Marchesini [:baku]]

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

Comment 40

[(no longer active)]

Yay!  This really deserves a blog post, Andrea!

Comment 41

[Ryan VanderMeulen [:RyanVM]]

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

Comment 42

[Emanuel Hoogeveen [:ehoogeveen]]

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

Comment 43

[Andrea Marchesini [:baku]]

Correct. No UI yet.

Comment 44

[(no longer active)]

Attachment: UX spec by shorlander

Comment 45

[(no longer active)]

Comment on attachment 8629601 [details]
UX spec by shorlander

Oops, wrong bug!