Closed Bug 1336484 Opened 3 years ago Closed 3 years ago

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

Categories

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

All
Unspecified
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
platform-rel --- +
firefox52 --- unaffected
firefox53 + verified
firefox54 --- verified

People

(Reporter: padenot, Assigned: ehsan)

References

Details

(Keywords: qawanted, regression, Whiteboard: [platform-rel-Facebook])

Attachments

(1 file, 2 obsolete files)

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.
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?
Blocks: 1326614
Flags: needinfo?(ehsan)
NI to myself so I can find or setup a live stream.
NI to Jean-Yves that has an alternative theory.
Flags: needinfo?(jyavenard)
Blocks: 1316871
No longer blocks: 1326614
(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.  :-)
Flags: needinfo?(padenot)
Flags: needinfo?(ehsan)
Flags: needinfo?(bkelly)
Yea, I meant bug 1316871.
Flags: needinfo?(bkelly)
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
[Tracking Requested - why for this release]:

Regression background tab audio in FF53.
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.
Flags: needinfo?(ehsan)
so it's the timers after all... nothing I can do.
Flags: needinfo?(jyavenard)
(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?
Flags: needinfo?(ehsan)
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.
Ehsan, that Cleopatra profile link in comment 10 is broken, it 404s on the profile fetch
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
(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?
platform-rel: --- → +
Whiteboard: [platform-rel-Facebook]
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?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
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.
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.
Assignee: nobody → ehsan
Summary: Issue with facebook live streaming: audio underrun when the tab is not focused → Don't throttle timeouts in background tabs that are playing audio
I don't see this problem when running a live stream from https://www.facebook.com/livemap/ in a background tab.
Disregard that, after enabling e10s I can reproduce the choppiness.
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.
Attachment #8835242 - Flags: review?(amarchesini)
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/eakhgari@mozilla.com-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!
Flags: needinfo?(padenot)
Confirmed, this build fixes it
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?
Attachment #8835242 - Flags: review?(amarchesini)
(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!
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.
Attachment #8835618 - Flags: review?(amarchesini)
Attachment #8835242 - Attachment is obsolete: true
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());
}
Attachment #8835618 - Flags: review?(amarchesini) → review+
Duplicate of this bug: 1334825
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.
Attachment #8835618 - Attachment is obsolete: true
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77313ca8fd71
Don't throttle timeouts in background tabs that are playing audio; r=baku
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.
Attachment #8836107 - Flags: approval-mozilla-aurora?
Keywords: steps-wantedqawanted
https://hg.mozilla.org/mozilla-central/rev/77313ca8fd71
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Track 53+ as regression.
Hi Florin,
could you help find someone to verify if this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
(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.
Flags: qe-verify+
Flags: needinfo?(florin.mezei)
Flags: needinfo?(brindusa.tot)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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+.
Attachment #8836107 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1339930
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.
Depends on: 1375119
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.