1336484 - Don't throttle timeouts in background tabs that are playing audio

Status: VERIFIED FIXED

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

Description

[Paul Adenot (:padenot)]

STR:
- Open a live stream on facebook on a tab (I don't seem to find a test URL I could use here).
- Switch away from this tab, and do something else

Expected:
- The audio is fine

Actual:
- Under-runs (gaps in audio) occur

Additional info:
- If the tab is focused on its own window, the audio is fine

Different people from different team (media and DOM) have different possible explanations.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, there is a theory this is due to our background throttling changes that landed in bug 1326614.  Since you are working on background timers can you investigate this?

Comment 2

[Paul Adenot (:padenot)]

NI to myself so I can find or setup a live stream.

Comment 3

[Paul Adenot (:padenot)]

NI to Jean-Yves that has an alternative theory.

Comment 4

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #1)
> Ehsan, there is a theory this is due to our background throttling changes
> that landed in bug 1326614.  Since you are working on background timers can
> you investigate this?

Is that the wrong bug?

Paul, can you please run mozregression to see what commit regressed this?  I don't know where I can find a Facebook live stream.  :-)

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Yea, I meant bug 1316871.

Comment 6

[Alice0775 White]

Steps to reproduce:
1. Open a any web page (e.g. http://rockridge.hatenablog.com/entry/20090129/1233179543 ) in a tab[A]
   And wait to stop tab-throbber spinning
2. Open ttps://www.facebook.com/livemap/?ref=bookmarks#@34.732584206123626,-95.899658203125,6z in a new tab[B]
   And click one of live stream video and then wait for the video playback to stabilize (for ~10 sec)
3. Switch to tab [A]
4. Observe audio issue

Actual Results:
Audio becomes to be intermittently interrupted (0.5sec@5-10sec)

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=88e0fb654a104419dd66774d422b20692639919e&tochange=44ab7f53ead5b57ef0d2e4de911567e4b5886d0a

Suspected: 6474932e4e65	Andreas Farre — Bug 1316871 - Throttle background setTimeouts. r=bkelly

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

[Tracking Requested - why for this release]:

Regression background tab audio in FF53.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Ehsan, see comment 6 for steps.  It does appear to be the background timers.  Can you take a look?  I don't understand why our audio context checks aren't preventing this.

Comment 9

[Jean-Yves Avenard [:jya]]

so it's the timers after all... nothing I can do.

Comment 10

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #8)
> Ehsan, see comment 6 for steps.  It does appear to be the background timers.
> Can you take a look?  I don't understand why our audio context checks aren't
> preventing this.

Can you reproduce this easily?  I can't.  I cannot reproduce basically ever in a new profile, but when my system is under load, both the audio and video stutter regardless of whether the tab is in the foreground or background.  I can relatively easily reproduce this in my normal browsing profile which has hundreds of tabs open, but that's not a useful environment for debugging what's happening.  That being said, in my main profile the audio is pretty choppy even when the tab is sitting in the foreground.  :(

The profile here <https://clptr.io/2kFayV5> demonstrates some of the audio choppiness in a local build (not all happening when the tab was in the bg.)  The pauses are all caused by the main thread for the tab not being responsive due to things like long GCs, long paints, etc.

(In reply to Ben Kelly [:bkelly] from comment #8)
> Ehsan, see comment 6 for steps.  It does appear to be the background timers.
> Can you take a look?  I don't understand why our audio context checks aren't
> preventing this.

This part is super simple: because Facebook doesn't use Web Audio.

Perhaps we should exempt all pages that are playing audio from background timeout throttling?

Comment 11

[(no longer active)]

I've contacted Facebook to ask about how the Facebook Live video works, whether they're aware of performance issues with it, and whether they can fix things on their side to not rely on the main thread being super responsive.

Comment 12

[Vladan Djeric (:vladan)]

Ehsan, that Cleopatra profile link in comment 10 is broken, it 404s on the profile fetch

Comment 13

[Vladan Djeric (:vladan)]

I was able to reproduce the audio stutter in background tabs consistently. Nothing fishy in my profile except setTimeouts in the background Live tab apparently being scheduled 1-2 seconds apart. This was on a mostly idle browser with only 2 tabs loaded.

> Perhaps we should exempt all pages that are playing audio from background timeout throttling?

This sounds good to me

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Vladan Djeric (:vladan) from comment #13)
> Perhaps we should exempt all pages that are playing audio from background
> timeout throttling?

Yes please! Doesn't the AudioChannelAgent know about playing HTMLMediaElements?

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Note, Chrome only exempts tabs from background throttling if they are playing audio above a certain volume level.  This is to avoid pages sticking silent audio elements in to circumvent these types of protections.

Should we do the same?

Comment 16

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #15)
> Note, Chrome only exempts tabs from background throttling if they are
> playing audio above a certain volume level.  This is to avoid pages sticking
> silent audio elements in to circumvent these types of protections.
> 
> Should we do the same?

We should use the same heuristics that we use to display the tab audio icon.  Since bug 1235612 we exclude silent audio when played through a media element, and if we need to in the future we can detect non-silent but non-audible audio.  I'm not worried about abuse scenarios since we have been shipping "workarounds" for setTimeout(0) throttling for years now (postMessage.)

I wrote a simple test page to test the behavior of other browsers: <https://people-mozilla.org/~eakhgari/timeout-delay.html>.  Pressing the test button causes us to schedule five setTimeout(0) timeouts with a delay of 1 second (to allow you to switch tabs when testing) and prints the delay times to the web console.  You can play a looping audio to test how that changes the behavior.  Both Chrome and Safari don't throttle background timeouts when the media element is playing.  If someone has Edge running locally I'd appreciate testing what Edge does as well.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

I do seem to get some delay in edge in the background on that test page:

> 1000
> timeout-delay.html (9,7)

But it seems to be only on the first timeout from the page.  The following timeouts are not delayed.

Comment 18

[(no longer active)]

Interesting...  I'm not sure why they would throttle only the first timeout.  But at any rate it seems clear what we should do here.

Comment 19

[Josh Matthews [:jdm]]

I don't see this problem when running a live stream from https://www.facebook.com/livemap/ in a background tab.

Comment 20

[Josh Matthews [:jdm]]

Disregard that, after enabling e10s I can reproduce the choppiness.

Comment 21

[(no longer active)]

Attachment: Don't throttle timeouts in background tabs that are playing audio

In websites such as Facebook Live, timeout chains are used to drive the
playback of a video or something similar in JavaScript.  Throttling the
minimum timeout values a tab playing a video from such websites in the
background could make the timeout based scheduling of video playback to
not work correctly, and cause audio buffer under-runs that are audible.

In order to address this, other major browsers don't throttle timeouts
in tabs that are playing audio.  This brings us to parity to other
browsers (even though we already do this for websites that use Web Audio
since we've had similar bug reports using the Web Audio API.)

The current audio agent setup that drives the tab audio notification
icons is currently tracking whether a Window is playing audio.  We use
this setup to decide whether to throttle timeouts when a window goes
into background.

We do this by storing nsDocShell::mPlayingAudio on the root docshell,
representing whether a tab is playing audio.  We use the existing
audio-playback notification to notify the root docshell of the window
about when audio playback starts and stops.  Then we modify the existing
heuristic about Web Audio in TimeoutManager to use the mPlayingAudio bit
on the root docshell to decide whether to throttle when the tab is in
the background.

Comment 22

[(no longer active)]

This patch makes Firefox behave similarly to Chrome and Safari in the test case in comment 16.  But since I can't really reproduce the Facebook Live problem myself reliably, I can't judge whether this fixes the issue.  I'd appreciate if others who can reproduce this more easily can help with testing.

I have done a try server push, and here are the builds for various desktop platforms:

* Windows: <https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-61496736f5d7c895baf9a90c8363a09593879e39/try-win32/firefox-54.0a1.en-US.win32.zip>
* Linux: <https://queue.taskcluster.net/v1/task/Y0rmvis0QXu0_qkPz4Zqug/runs/0/artifacts/public/build/target.tar.bz2>
* Mac: <https://queue.taskcluster.net/v1/task/LqxarFjKQGefR24NL9yKpA/runs/0/artifacts/public/build/target.dmg>

Thanks in advance!

Comment 23

[Vladan Djeric (:vladan)]

Confirmed, this build fixes it

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8835242 [details] [diff] [review]
Don't throttle timeouts in background tabs that are playing audio

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

What about if we introduce this:

NS_IMETHODIMP AudioChannelService::IsWindowActive(mozIDOMWindowProxy* aWindow, bool* aActive) {
  auto* window = nsPIDOMWindowOuter::From(aWindow)->GetScriptableTop();
  AudioChannelWindow* winData = GetOrCreateWindowData(aWindow);
  return !winData->mAudibleAgents.IsEmpty();
}

And we don't touch the docShell?

Comment 25

[(no longer active)]

(In reply to Vladan Djeric (:vladan) from comment #23)
> Confirmed, this build fixes it

Thanks for testing!

(In reply to Andrea Marchesini [:baku] from comment #24)
> And we don't touch the docShell?

Sounds great!

Comment 26

[(no longer active)]

Attachment: Don't throttle timeouts in background tabs that are playing audio

In websites such as Facebook Live, timeout chains are used to drive the
playback of a video or something similar in JavaScript.  Throttling the
minimum timeout values a tab playing a video from such websites in the
background could make the timeout based scheduling of video playback to
not work correctly, and cause audio buffer under-runs that are audible.

In order to address this, other major browsers don't throttle timeouts
in tabs that are playing audio.  This brings us to parity to other
browsers (even though we already do this for websites that use Web Audio
since we've had similar bug reports using the Web Audio API.)

The current audio agent setup that drives the tab audio notification
icons is currently tracking whether a Window is playing audio.  We use
this setup to decide whether to throttle timeouts when a window goes
into background.

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8835618 [details] [diff] [review]
Don't throttle timeouts in background tabs that are playing audio

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

Can you please introduce this:

/* static */ already_AddRefed<AudioChannelService>
AudioChannelService::Get()
{
  if (sXPCOMShuttingDown) {
    return nullptr;
  }

  RefPtr<AudioChannelService> service = gAudioChannelService.get();
  return service.forget();
}

::: dom/base/nsGlobalWindow.cpp
@@ +4157,2 @@
>  {
> +  RefPtr<AudioChannelService> acs = AudioChannelService::GetOrCreate();

RefPtr<AudioChannelService> acs = AudioChannelService::Get();
if (acs) {
  return acs->IsWindowActive(GetOuterWindow());
}

Comment 29

[(no longer active)]

Attachment: Don't throttle timeouts in background tabs that are playing audio

In websites such as Facebook Live, timeout chains are used to drive the
playback of a video or something similar in JavaScript.  Throttling the
minimum timeout values a tab playing a video from such websites in the
background could make the timeout based scheduling of video playback to
not work correctly, and cause audio buffer under-runs that are audible.

In order to address this, other major browsers don't throttle timeouts
in tabs that are playing audio.  This brings us to parity to other
browsers (even though we already do this for websites that use Web Audio
since we've had similar bug reports using the Web Audio API.)

The current audio agent setup that drives the tab audio notification
icons is currently tracking whether a Window is playing audio.  We use
this setup to decide whether to throttle timeouts when a window goes
into background.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77313ca8fd71
Don't throttle timeouts in background tabs that are playing audio; r=baku

Comment 31

[(no longer active)]

Comment on attachment 8836107 [details] [diff] [review]
Don't throttle timeouts in background tabs that are playing audio

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1316871.
[User impact if declined]: See comment 0 and also bug 1334825.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: It has been verified using a try build.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: It's moderately risky.  I'm not worried about web compat because this makes us behave the same way as other browsers, but even though the code is relatively straightforward, it's not a one-liner.  I'm applying for approval because I think bug 1334825 is a very nice optimization (one that every browser is doing except for us) so I'd rather ship it in 53 than 54.  But if this doesn't get approved, we can back out bug 1334825 from Aurora and ship it with the fix in 54.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/77313ca8fd71

Comment 33

[Gerry Chang [:gchang]]

Track 53+ as regression.

Comment 34

[Gerry Chang [:gchang]]

Hi Florin,
could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 35

[Andrei Vaida [:avaida]]

(In reply to Gerry Chang [:gchang] from comment #34)
> Hi Florin,
> could you help find someone to verify if this issue is fixed as expected on
> a latest Nightly build? Thanks!

Forwarding this to Brindusa.

Comment 36

[Simona Badau, Desktop QA]

Reproduced using the latest Nightly 54.0a1 (Build ID: 20170203030204) on Windows 10 x64.

Verified as fixed using the latest Nightly 54.0a1 (Build ID: 20170213030206) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.

The duplicate Bug 1334825 is not reproducible anymore (meaning that the audio playback on BBC does not completely stops shortly after Firefox is set into background) but, the sound becomes intermittently interrupted. Logged Bug 1339379 to track this issue.

Comment 37

[Gerry Chang [:gchang]]

Comment on attachment 8836107 [details] [diff] [review]
Don't throttle timeouts in background tabs that are playing audio

Fix an issue related to audio in the background tab and was verified. Aurora53+.

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6d0cc154d82

Comment 39

[Alexandru Simonca [:asimonca], Release Desktop QA]

Reproduced using Nightly 54.0a1 (Build ID: 20170203030204) on Windows 10 x64.

Verified as fixed using Beta 53.0b2 (Build ID: 20170313154936) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.