New issue
Advanced search Search tips

Issue 914608 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner:
Closed: Dec 17
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-12-19
OS: Windows , Mac
Pri: 1
Type: Bug



Sign in to add a comment

AudioParamTimeline leaks events causing page to become progressively slower

Reported by brandon....@oculus.com, Dec 13

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36

Steps to reproduce the problem:
1. Run attached html
2. See the dev console output
3. Note the time between frames is increasing rapidly

What is the expected behavior?
Time between frames should remain relatively stable.

What went wrong?
I noticed my VR game was slowing down, and tracked it to events leaking in AudioParamTimeline.cpp.  I'm not familiar enough with this code to propose a fix.

This likely impacts most 3d games/vr experiences that use Three.js to do positional audio. See: https://github.com/mrdoob/three.js/issues/15344

Did this work before? No 

Does this work in other browsers? N/A

Chrome version: 70.0.3538.77  Channel: stable
OS Version: 10.0
Flash Version: 

I've only tested as far back as m66, but it repros since then at least.
 
panner_bug.html
1.5 KB View Download
I've investigated a bit this issue with a debugger, a bit more info. The issue is that events_ and new_events_ arrays in third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc are growing and never get cleared in this case. The code that clears those arrays is never called for this timeline: neither AudioParamTimeline::ClampNewEventsToCurrentTime nor AudioParamTimeline::ValuesForFrameRangeImpl nor AudioParamTimeline::HandleAllEventsInThePast are called. And these method are the only ones which can clear the aforementioned arrays, AFAICS. Thus, the method AudioParamTimeline::InsertEvent just continuing to add new events forever, slowing down more and more and consuming more and more memory.
Labels: Needs-Triage-M70
Cc: phanindra.mandapaka@chromium.org
Labels: Triaged-ET Needs-Bisect Needs-Feedback
Status: Untriaged (was: Unconfirmed)
As per comment #0, Able to reproduce the issue on reported chrome 70.0.3538.77 and not seen on latest beta 72.0.3626.17 and latest chrome 73.0.3639.0 Using Windows 10. and issue not seen on M-60.
Hence, Marking it as Untriage. Will provide the reverse bisect information and other OS behavior soon.

@Reporter: can you please check and confirm the issue on latest chrome beta 72.0.3626.17, you can download latest chrome builds here:" https://www.chromium.org/getting-involved/dev-channel ". Let us know whether issue still persists.

Thanks..! 
The latest chrome beta (windows 10/64bit) shows version 71.0.3578.98 for me.

I can confirm it doesn't no repro on this build.

Comment 5 Deleted

Sorry I didn't mean the double negative in comment 5.  It does not repro on the latest chrome beta.
If it doesn't repro in M71+, is there a way to point us out the commit that fixes it (via chromium-gerrit, or by just providing commit's SHA)?
The closest one I found is this:
https://chromium-review.googlesource.com/c/chromium/src/+/1249583

This CL does not address the concern in #1. I have not investigated that yet.
Let me know if you have more questions.
I doubt it, it doesn't seem to have anything with how events_ are allocated / processed (and I know for sure that the slow down was caused by growing events_ array inside the AudioParamTimeline::InsertEvent method.

In fact, I've backported the whole new audio_param_timeline.cc from master into our version of the Browser (which is M66-based) and the leak / slowdown is still there.

I've also went through the list of changes in WebAudio folder and I was unable to find the dedicated fix for this particular issue. This makes me think that the issue might be fixed as a part of other modifications, possibly inadvertently, i.e. it still may be a potential problem even though our repro doesn't demonstrate it anymore. Do you have any ideas why InsertEvent could be not adding more events in the case when source is stopped? Or, could it still add the events but for some reasons not iterating through the whole array, i.e. still leaking memory?
That is the only change in the range that is relevant to AudioParam's performance.

> I've also went through the list of changes in WebAudio folder and I was unable to find the dedicated fix for this particular issue.

That might be the case. You can try bisecting if you are really curious about the fix.

The AudioParam code base has been managed by the other API owner, so it'll take some time for me in order to give you the better answer.

I think the best course of action here is to close this one and open a new issue for a potential memory leak. If you can attach a simple repro code that affects M73 (canary), it would be great.
Labels: -Needs-Feedback -Needs-Bisect ReleaseBlock-Stable RegressedIn-70 Target-71 M-71 FoundIn-71 hasbisect OS-Mac
Owner: mlamouri@chromium.org
Status: Assigned (was: Untriaged)
Able to reproduce the issue on reported chrome 70.0.3538.77 however issue seems to be fixed in latest chrome beta 72.0.3626.17 and canary 73.0.3638.0 using Mac 10.14.0, Windows 10 with steps mentioned in comment# 0, hence providing reverse bisect info

Reverse Bisect Info:
================
Last Bad build:  72.0.3626.0
First Good build: 72.0.3626.10

As the issue break is seen in 72.0.3626 branch build, hence providing below change-log from omahaproxy
CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/72.0.3626.0..72.0.3626.10?pretty=fuller&n=10000
Reviewed-on: https://chromium-review.googlesource.com/c/1355333

Mounir Lamouri: Please confirm the issue and help in re-assigning if it is not related to your change.Adding 'ReleaseBlock-Stable' label as this is a recent regression. Please feel free to remove if this is not applicable.

Note: Issue not observed on Ubuntu 17.10

Thanks..!
Labels: -Pri-2 -ReleaseBlock-Stable Pri-1
++Adding..

Abel to reproduce the issue on chrome stable M-71(71.0.3578.98) using Windows 10 and Mac 10.14.0. Updating the bisect info with nearest range and changing the priority=1. As the issue regressed in M-70,Hence removing ReleaseBlock-Stable M-71.
Reverse Bisect Info:
================
Last Bad build:   72.0.3626.9
First Good build: 72.0.3626.10

Thanks..!
Okay, this might be a simple mistake.

Please take a careful look at:
https://developers.google.com/web/updates/2018/11/web-audio-autoplay

The problem is the timeline is not being processed because the context is not running. The repro case does not follow the autoplay policy change (M71), so the context starts in the "suspended" state.

The repro code is constantly scheduling new events on the suspended context. Thus events will just keep on piling. We can't reject scheduling, or remove events from the list without processing them, so the accumulating events is an expected behavior.

There are certain points removing events from the list, so #1 is not true.
https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc?sq=package:chromium&g=0&l=1067
https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc?sq=package:chromium&g=0&l=1907
Labels: -RegressedIn-70 -Needs-Triage-M70
Owner: hongchan@chromium.org
phanindra@

Thanks for the bisect. It was really useful. Per #13, this is not a regression.
I'll own this issue until reporters agree with my response.

My plan is to close this issue "WontFix" after the agreement.
Labels: Needs-Feedback
NextAction: 2018-12-19
artyom17@

Could you take a look at The resolution at #14?
Well... Still weird that this repro is piling events in M66, doesn't do it in M70 and then again doing that in M72. Also, Firefox doesn't seem to do it. But it is up to you how to handle this issue, if you are certain that "it is by design" then it is fine. 
> piling events in M66, doesn't do it in M70 and then again doing that in M72

Is it doing that again in M72? That's weird. What's the exact version number?
So the events are piling up even when the context is running to process the graph?
I am sorry, I just got the impression from previous posts that the issue exists in M72 as well. Sorry for confusing, disregard my #16 message.
Status: WontFix (was: Assigned)
Don't be. I really appreciate you're raising this issue and helping the triage. As I mentioned in the related GitHub comment [1], the problem does not exist in the latest version of Chrome.

I am closing this issue as WontFix, but please feel free to open a new issue if you find another performance issue.

[1]: https://github.com/mrdoob/three.js/issues/15422#issuecomment-448013616
The NextAction date has arrived: 2018-12-19

Sign in to add a comment