Nice feature to have is mute and volume per window

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
a month ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 30+)

Details

Attachments

(18 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Posted patch patch (obsolete) — Splinter Review
Attachment #813236 - Flags: review?(roc)
Attachment #813236 - Flags: review?(ehsan)
(Assignee)

Comment 2

6 years ago
Posted file test.xpi (obsolete) —
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

6 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.)
Attachment #813236 - Flags: review?(ehsan) → review+
(Assignee)

Comment 4

6 years ago
Posted patch patch 2 - notifications (obsolete) — Splinter Review
Attachment #813515 - Flags: review?(roc)
Attachment #813515 - Flags: review?(ehsan)
(Assignee)

Comment 5

6 years ago
Attachment #813236 - Attachment is obsolete: true
Attachment #813236 - Flags: review?(roc)
Attachment #813516 - Flags: review?(roc)

Comment 6

6 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.
Attachment #813515 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

6 years ago
Posted patch patch 2 - notifications (obsolete) — Splinter Review
Attachment #813515 - Attachment is obsolete: true
Attachment #813515 - Flags: review?(roc)
Attachment #813540 - Flags: review?(roc)
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?
Attachment #813516 - Flags: review?(roc) → review-
(Assignee)

Comment 9

6 years ago
Attachment #813516 - Attachment is obsolete: true
Attachment #816429 - Flags: review?(roc)
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."
Attachment #816429 - Flags: review?(roc) → review-
(Assignee)

Comment 11

6 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.
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?
(Assignee)

Comment 13

6 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.
Attachment #816429 - Attachment is obsolete: true
Attachment #817849 - Flags: feedback?(roc)
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.
Attachment #817849 - Flags: feedback?(roc)
Blocks: 961202
(Assignee)

Comment 15

5 years ago
FMRadio is not implemented and WebAudio is probably to be improved.
Attachment #817849 - Attachment is obsolete: true
Attachment #8385508 - Flags: review?(ehsan)
(Assignee)

Comment 16

5 years ago
Posted patch patch 2 - notifications (obsolete) — Splinter Review
Just moved to AudioChannelService.
Attachment #813540 - Attachment is obsolete: true
Attachment #8385524 - Flags: review?(ehsan)

Comment 17

5 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?
Attachment #8385508 - Flags: review?(roc)
Attachment #8385508 - Flags: review?(ehsan)
Attachment #8385508 - Flags: review+

Comment 18

5 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.
Attachment #8385524 - Flags: review?(ehsan) → review+
(Assignee)

Updated

5 years ago
Flags: needinfo?(paul)
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 19

5 years ago
Attachment #8385508 - Attachment is obsolete: true
Attachment #8385508 - Flags: review?(roc)
Attachment #8385826 - Flags: review?(roc)
(Assignee)

Comment 20

5 years ago
Posted patch patch 2 - notifications (obsolete) — Splinter Review
Attachment #8385524 - Attachment is obsolete: true
Attachment #8385827 - Flags: review?(roc)
(Assignee)

Comment 21

5 years ago
This patch has been already reviewed by mchen for bug 876631
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.
Flags: needinfo?(paul)
(Assignee)

Comment 23

5 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?
Flags: needinfo?(paul)
Call MediaStream::SetAudioOutputVolume on the destination node's stream (probably via a method on DestinationNode).
Flags: needinfo?(paul)
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.
Attachment #8385826 - Flags: review?(roc) → review-
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!
Attachment #8385827 - Flags: review?(roc) → review+
(Assignee)

Comment 27

5 years ago
Attachment #8385826 - Attachment is obsolete: true
Attachment #8386604 - Flags: review?(roc)
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.
Attachment #8386604 - Flags: review?(roc) → review-
(Assignee)

Comment 29

5 years ago
Posted patch patch 1 - interdiff (obsolete) — Splinter Review
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?
Attachment #8386687 - Flags: feedback?(roc)

Comment 30

5 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.
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.
Attachment #8386687 - Flags: feedback?(roc) → feedback-
(Assignee)

Comment 32

5 years ago
Posted patch patch 1 - interdiff (obsolete) — Splinter Review
Thanks for your comment.This seems a bit a ping-pong between AudioContext and AudioDestinationNode. But at least it works :)
Attachment #8386687 - Attachment is obsolete: true
Attachment #8387019 - Flags: review?(roc)
(Assignee)

Comment 33

5 years ago
here the full patch.
Attachment #8386604 - Attachment is obsolete: true
Attachment #8387020 - Flags: review?(roc)
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.
Attachment #8387019 - Flags: review?(roc) → review+
(Assignee)

Comment 35

5 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.
Attachment #8387019 - Attachment is obsolete: true
Attachment #8387020 - Attachment is obsolete: true
Attachment #8388363 - Flags: review?(roc)
(Assignee)

Comment 38

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

waiting for green on try before landing it again.

Comment 40

5 years ago
Yay!  This really deserves a blog post, Andrea!
https://hg.mozilla.org/mozilla-central/rev/798346050542
https://hg.mozilla.org/mozilla-central/rev/71d145853be8
https://hg.mozilla.org/mozilla-central/rev/ec6116366680
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
After glancing through the patches, I take it that this adds new functionality to the backend, but there's no UI for it yet?
(Assignee)

Comment 43

5 years ago
Correct. No UI yet.

Updated

5 years ago
Depends on: 983984

Updated

4 years ago
Blocks: 1141119

Updated

4 years ago
Depends on: 1167690

Updated

4 years ago
Depends on: 1180421

Updated

4 years ago
Depends on: 1180423

Comment 44

4 years ago
Posted image UX spec by shorlander (obsolete) —
Attachment #813239 - Attachment is obsolete: true
Attachment #8385827 - Attachment is obsolete: true
Attachment #8385830 - Attachment is obsolete: true
Attachment #8388363 - Attachment is obsolete: true

Comment 45

4 years ago
Comment on attachment 8629601 [details]
UX spec by shorlander

Oops, wrong bug!
Attachment #8629601 - Attachment is obsolete: true

Updated

4 years ago
No longer depends on: 1167690, 1180421, 1180423

Updated

4 years ago
Depends on: 1190082

Updated

4 years ago
Depends on: 1191814
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.