Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use requestAnimationFrame() instead of setTimeout/setInterval() #24

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ibc
Copy link
Contributor

@ibc ibc commented Sep 8, 2016

requestAnimationFrame() is much more efficient than setTimeout() or setInterval(), and the result is smoother and more accurate (just check the provided demo and the consumed CPU).

@latentflip
Copy link
Contributor

Interesting, haven't looked at the demo yet but it's not immediately obvious why cpu usage would be lower with this? Won't using raf mean we go from calculating ffts every 100ms (by default) to every frame (16ms) which should be more work no?

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

requestAnimationFrame() invokes the given callback 60 times per second (maximum) but that depends on how "good" the browser feels. If it cannot provide all those slots it just reduces the interval. The callback is basically called when it's good to call it.

The CPU usage is similar (just check the demo with and without this commit), but take into account that before this PR the interval is 100ms which produces a visual effect much worse than with this commit applied.

@saghul
Copy link

saghul commented Sep 8, 2016

But there is no visual effect. This thing runs the FFT at the given interval, detecting speech, so what's the benefit of doing it as fast as possible?

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

But there is no visual effect.

If you want to detect voice volume is because you want to display a VU meter. The smoother the visual effect is the better it looks.

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

Firstly, setTimeout doesn’t take into account what else is happening in the browser. The page could be hidden behind a tab, hogging your CPU when it doesn’t need to, or the animation itself could have been scrolled off the page making the update call again unnecessary. Chrome does throttle setInterval and setTimeout to 1fps in hidden tabs, but this isn’t to be relied upon for all browsers.

Secondly, setTimeout only updates the screen when it wants to, not when the computer is able to. That means your poor browser has to juggle redrawing the animation whilst redrawing the whole screen, and if your animation frame rate is not in synchronised with the redrawing of your screen, it could take up more processing power. That means higher CPU usage and your computer’s fan kicking in, or draining the battery on your mobile device.

@saghul
Copy link

saghul commented Sep 8, 2016

If you want to detect voice volume is because you want to display a VU meter. The smoother the visual effect is the better it looks.

I don't. This library provides 2 things: audio levels and speaking / not speaking events. You don't need raf for the latter, AFAIS.

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

This library provides 2 things: audio levels and speaking / not speaking events. You don't need raf for the latter, AFAIS.

Right. Now please show me how better is a 100ms interval than requestAnimationFrame for that second thing.

@saghul
Copy link

saghul commented Sep 8, 2016

I didn't say it's better, I said it's not necessary :-) Doing the detection too often would probably lead to flip-flopping too many times, I guess.

@latentflip
Copy link
Contributor

:) nobody said anything about anyone being crazy. I agree, in general for animations rAF is definitely the way to go, and obviously hark can be used to drive animations.

My caution is simply that I've always had concern in the back of my mind about how performant hark is or isn't, as well as how much CPU/battery/etc, running constant FFTs on audio streams consumes; and I haven't ever got my head fully around how good/bad things are in that regard.

Sure rAF allows the browser to scale back and do less frames when it's busy with other things more easily, but equally, when things aren't otherwise busy we're now doing ~60 sets of calculations per second instead of ~10. So I just want to be sure we have a good understanding of the impact of that, and whether rAF should be the default/optional/otherwise.

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

Doing the detection too often would probably lead to flip-flopping too many times, I guess.

Then the interval should not be a user-configurable parameter.

Anyhow, you can do stuff within the given callback such as returning if no enough time has passed. Still requestAnimationFrame is better for that than timers.

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

@latentflip I understand your concerns. I've the same ones :)

However, when things go bad and the computer is consuming lot of CPU, a 100ms periodic timer may be much worse (in terms of CPU usage and performance) than a requestAnimationFrame.

@latentflip
Copy link
Contributor

What if we do both, use rAF to trigger every frame, but then still keep the interval option, and only run the actual update if the interval has passed (a la http://stackoverflow.com/a/19772220).

@saghul
Copy link

saghul commented Sep 8, 2016

My last 2 cents :-) raf seems to be all about animations. If you want butter-smooth animations you need to do things many times per second, that's a given. But that's not the case here. If you CPU is overloaded then timers may fall behind, but that doesn't matter. We may miss some speaking events, but that doesn't matter either. Or maybe it does, depends on your application. Before this patch that value was configurable, now it's not, so people who use it would lose the ability to do so.

Telling users "yeah whatever, handle it yourself in the callback" just doesn't sound right.

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

@latentflip I do agree:

let fps = 30;  // User-configurable
let interval = 1000 / fps;
let before = Date.now();

function check()
{
    timer = requestAnimationFrame(check);

    /* emit volume levels here */

    let now = Date.now();

    if (now - before >= interval)
    {
        before = now;

        /* emit speaking/non-speaking events here */
    }
}

check();

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

@saghul said:

Before this patch that value (interval) was configurable

Such an interval is of very little use when your computer is consuming lot of CPU. Chrome does magic by reducing timer intervals in this scenario, but other browsers may just queue tasks and more tasks. requestAnimationFrame is the only solution that guarantees that the callback will just be called when appropriate and "delayed" callbacks won't be queued.

@latentflip
Copy link
Contributor

I think I'd rather:

a) leave interval as user configurable (rather than fps), and leave it as 100ms
b) do both the calculation and emitting both volume and speaking/non-speaking conditionally on that interval (in your code sample we'd still be running the calculations at 60fps)

e.g.:

let interval = 100; // User-configurable
let before = Date.now();

function check()
{
    timer = requestAnimationFrame(check);

    let now = Date.now();

    if (now - before >= interval)
    {
        before = now;
        /* calculate volume levels */
        /* emit volume levels here */
        /* emit speaking/non-speaking events here */
    }
}

check();

@ibc
Copy link
Contributor Author

ibc commented Sep 8, 2016

@latentflip that would also work, of course. It's just that the VU meter feature looks better when interval === 16 :)

IMHO the speaking/non-speaking detection should not rely on a user provided interval value (this library is supposed to know which interval is the perfect one).

@xdumaine
Copy link
Contributor

xdumaine commented Sep 8, 2016

For what it's worth:

Hark doesn't do animation, so I'm in the camp of optimizing for performance gathering, but if you're animating something based on events, that's a tangential concern. Often times (as was the case when hark was first written), the animation was used to transmit your volume level to another party (e.g., over a data channel) at which point they would animate it. Again, this separates the concern of gathering the volume level and animating it.

Another reason the delay was set to 100ms and not 16 (for 60fp) is that this was bombarding the capture, transmission, and animation in cases where there are multiple participants.

I'd suggest that if your concern is butter smooth animations, then you (collectively, across all volume meters to be animated), use rAF outside of hark, with the highest rate you want. Then tune hark to perform as well as it can with regards only to capture of the value, which is its job, and leave the animation up to the animation layer.

@ibc
Copy link
Contributor Author

ibc commented Sep 9, 2016

Another reason the delay was set to 100ms and not 16 (for 60fp) is that this was bombarding the capture, transmission, and animation in cases where there are multiple participants.

This is exactly the reason for using requestAnimationFrameas all those callbacks will be called within a specific loop slot in which the screen will be redrawn.

Anyhow, I insist: arguing that a 100ms interval is better for CPU usage than raf is not true. All the browsers guarantee that raf will invoke the given callback when appropriate (each 16ms if the CPU is fine, and at lower interval if the CPU is busy). That is not true for setTimeout or setInterval (regardless Chrome does magic). A browser may just queue all the callbacks forever and ever, producing a result much worse than raf in case of congestion.

@latentflip
Copy link
Contributor

Okay, to summarise, I think there are multiple concerns being conflated here:

  1. Does the current use of setInterval potentially cause issues of queueing up multiple expensive speaking checks if the CPU is busy.
    1. I believe the answer to this is yes, if CPU busyness lasts longer than the interval (default 100ms) then this may happen
    2. Switching to some kind of rAF based solution should reduce this issue
  2. Does switching straight from setInterval to rAF, as the current PR does, cause other potential issues? I believe the answer is yes for these reasons:
    1. Checking every 16ms instead of 100ms is straight up more work CPU being done over time
    2. Emitting volume/is speaking events every 16ms may cause cascading more effort to be done, as well as more toggling between speaking/is speaking events which may have knock on effects
    3. I believe that looping on animation frames, but only actually running volume checks every N ms
      as proposed in Use requestAnimationFrame() instead of setTimeout/setInterval() #24 (comment) deals with these issues
  3. If we are using rAF, do we want to optionally allow users to drop the check/emit interval all the way down to 16ms, so that people can run smoother animations off the back of volume changes?
    1. I am happy to support this use-case, as long as it's opt-in and doesn't affect existing users
    2. If we do this, we may need to add some kind of hysteresis to the speaking event emissions so that they don't toggle too wildly.

My feeling is that the code I proposed in #24 (comment), with some kind of a tweak to deal with the issue in 3.ii. should cover all of these issues/use-cases - allowing @ibc to do what he's trying to do, and the only change for existing users will be that potential CPU issues described in 1. will be improved.

@ibc
Copy link
Contributor Author

ibc commented Sep 9, 2016

Agreed.

BTW: I would also open the door to separate intervals for both VU-meter and speaking detection features:

let timer;
let vuInterval = 100;  // default value (configurable)
let speakingInterval = 100;  // fixed value
let vuBefore = Date.now();
let speakingBefore = Date.now();

function check()
{
    timer = requestAnimationFrame(check);

    let now = Date.now();

    if (now - vuBefore >= vuInterval)
    {
        vuBefore = now;

        /* emit VU-meter event here */
    }

    if (now - speakingBefore >= speakingInterval)
    {
        speakingBefore = now;

        /* emit speaking/non-speaking events here */
    }
}

check();

@ibc
Copy link
Contributor Author

ibc commented Mar 9, 2017

Hi guys, any update to this? :)

@xdumaine
Copy link
Contributor

Maybe using rAF could just be a config option, so backward behavior is maintained.

@jadbox
Copy link

jadbox commented Jun 30, 2018

@ibc do you mind rebasing from master?

@ibc
Copy link
Contributor Author

ibc commented Jun 30, 2018

Hi @jadbox, which is the preferred behavior then? Something as I proposed in #24 (comment)?

@jadbox
Copy link

jadbox commented Jul 19, 2018

@ibc it seems like the right approach to me, but I'm still just starting to learn the code base. To give context, I'm building a large mobile application that uses Hark to drive high fidelity animations, and it'll be ideal to only process events during a frame.
Ultimately though, maybe it would be useful to have Hark configurable so that it can be drive by either requestAnimationFrame or by a timer, depending on a config option like { useFrameTiming: true }.

@piranna
Copy link

piranna commented Apr 13, 2021

Any update on this? Although having no updates in three years is a bad symptomp... :-/

@notsuretho
Copy link

I was reading an article on MDN and it said:

As we covered earlier, you don't specify a time interval for requestAnimationFrame(). It just runs it as quickly and smoothly as possible in the current conditions. The browser also doesn't waste time running it if the animation is offscreen for some reason, etc.

This might imply that RAF doesn't get called if the animation is offscreen. Meaning while timeouts and invertals might be throttled, RAF callback might not be called at all.

@piranna
Copy link

piranna commented Jul 30, 2021

This might imply that RAF doesn't get called if the animation is offscreen.

That is true. It's browser dependent, but I think it was Chrome or Safari than when browser tab was not active, it throttle requestAnimationFrame() down to 1 execution each 15 seconds (1/15fps), instead of the usual 60fps most monitors have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants