1147819 - Any media element should be stopped by the AudioChannelService when the window is destroyed

Status: RESOLVED FIXED

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

Description

[Geoff Lankow (:darktrojan)]

I don't really know much else apart from what's in the bug title.

STR:
Set media.useAudioChannelService to true
Start a YouTube video
Switch tab

Comment 1

[Geoff Lankow (:darktrojan)]

I've found out something else: no messageManager messages make it to message listeners while the tab is not selected.

Also I'm pretty sure this isn't just YouTube, it's any HTML5 media.

Comment 2

[Geoff Lankow (:darktrojan)]

(In reply to Geoff Lankow (:darktrojan) from comment #1)
> I've found out something else: no messageManager messages make it to message
> listeners while the tab is not selected.

I think I was mistaken about this.


@baku: since you're killing this function off in bug 1113086, can I disable it (make visible always true) for desktop only in the meantime? That would make desktop behave as expected with the service enabled.

http://hg.mozilla.org/mozilla-central/file/883e17fc475f/dom/audiochannel/AudioChannelAgent.cpp#l144

Comment 3

[Andrea Marchesini [:baku]]

Just set media.defaultAudioChannel to  'content'. This is what I do on desktop when I enable AudioChannelService.

Why do you wantto enable AudioChannelService in Desktop? Some particular addon?

Comment 4

[Geoff Lankow (:darktrojan)]

Okay, I didn't know about that pref, and it fixes the original problem. But now I have another: the audio won't stop playing after I navigate away from the page it was on.

This is what I want it for: https://addons.mozilla.org/en-US/firefox/addon/noise-control/

Comment 5

[Andrea Marchesini [:baku]]

Attachment: audio.patch

The behavior this patch fixes is that, if the AudioChannelService is enabled and a media element is playing when the page is navigated away, that media element has to be muted.

Comment 6

[(no longer active)]

Comment on attachment 8589071 [details] [diff] [review]
audio.patch

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

This needs a test!

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +102,5 @@
> +    if (!pWindow->IsInnerWindow()) {
> +      pWindow = pWindow->GetCurrentInnerWindow();
> +    }
> +
> +    mWindow = pWindow;

Nit: pWindow.forget() so that we don't AddRef/Release unnecessarily.

::: dom/audiochannel/AudioChannelService.cpp
@@ +39,5 @@
>  
> +// When a inner-window is destroyed we have to mute all the related
> +// AudioChannelAgents. In order to do this we have to notify them after purging
> +// AudioChannelService::mAgents.
> +struct WindowDestroyedEnumeratorData

MOZ_STACK_CLASS.

@@ +41,5 @@
> +// AudioChannelAgents. In order to do this we have to notify them after purging
> +// AudioChannelService::mAgents.
> +struct WindowDestroyedEnumeratorData
> +{
> +  WindowDestroyedEnumeratorData(uint64_t aInnerID)

explicit

Comment 7

[Andrea Marchesini [:baku]]

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

Comment 8

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2be1c521ab5f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2be1c521ab5f

Comment 10

[Geoff Lankow (:darktrojan)]

It's getting a bit late, but do you think this could be pushed forward to 39?

Comment 11

[Andrea Marchesini [:baku]]

The patch should apply easily and in case, I can rebase it. And I don't think it will introduce regressions. ehsan?

Comment 12

[(no longer active)]

As far as I can tell, this shouldn't affect users unless they have manually changed the media.useAudioChannelService pref, so it should be safe to backport, I think.

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8589071 [details] [diff] [review]
audio.patch

Approval Request Comment
[Feature/regressing bug #]: AudioChannelPolicy
[User impact if declined]: nothing. This is useful for users who enabled AudioChannelService in desktop manually. There are 2 addons who will benefice from this patch (one is mine).
[Describe test coverage new/current, TreeHerder]: green on try but no specific tests.
[Risks and why]: All this code is disabled in beta completely.
[String/UUID change made/needed]: none

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8589071 [details] [diff] [review]
audio.patch

I'd like us to let this ride with 40. It doesn't seem urgent or high user impact. If that assessment is wrong let me know or re-nominate this for uplift. Thanks!