1003683 - Images displayed in multiple foreground documents (with independent refresh drivers) will refresh & invalidate too frequently

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

(Spinning this bug off to cover the issue described in bug 1002632 comment 21 - 22.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This patch throttles RequestRefresh() calls to no more than 2x the refresh driver's default refresh rate.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8415532 [details] [diff] [review]
fix v1

Seems reasonable.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8415532 [details] [diff] [review]
fix v1

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

::: image/src/Image.cpp
@@ +137,5 @@
> +  static bool isRecentThresholdInitialized = false;
> +
> +  // Our threshold for "recent" is 1/2 of the default refresh-driver interval.
> +  // This ensures that we allow for frame rates at least as fast as the
> +  // refresh driver's default rate.

Does anyone ever create a refresh driver that refreshes *faster* than the default interval?

@@ +140,5 @@
> +  // This ensures that we allow for frame rates at least as fast as the
> +  // refresh driver's default rate.
> +  if (!isRecentThresholdInitialized) {
> +    recentThreshold =
> +      TimeDuration::FromMilliseconds(nsRefreshDriver::DefaultInterval() / 2.0);

Can we just initialize recentThreshold with this value directly? That way we can get rid of isRecentThresholdInitialized, and the compiler will generate code that is pretty much equivalent to what you have here.

::: image/src/RasterImage.cpp
@@ +540,5 @@
>  
>  //******************************************************************************
>  // [notxpcom] void requestRefresh ([const] in TimeStamp aTime);
>  NS_IMETHODIMP_(void)
>  RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)

Should HadRecentRefresh just be using aTime?

There are at least two arguments I see for why it should:

(1) Reading the current time is expensive, and now we have to do it once for every image instead of once for every refresh driver tick. This seems to me to be potentially a big deal.

(2) Using the current time means that it becomes harder to reason about which of multiple refresh drivers will 'win'. (It depends on not only on the phase they have with respect to each other, but also can depend on jitter in how long it takes to run RequestRefresh for other images on each page.) This also seems to me like it might have the potential to increase perceived jitter in updates to animated images which run into this situation, though it's possible that it'd have the opposite effect if the phase relationship between the different refresh drivers isn't stable.

I definitely don't claim to be an expert on refresh drivers, but within the limitations of what I know these arguments seem persuasive to me.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #3)
> > +  static bool isRecentThresholdInitialized = false;
> > +
> > +  // Our threshold for "recent" is 1/2 of the default refresh-driver interval.
> > +  // This ensures that we allow for frame rates at least as fast as the
> > +  // refresh driver's default rate.
> 
> Does anyone ever create a refresh driver that refreshes *faster* than the
> default interval?

Sometimes yes (for one or two samples), but that's not quite the point here. ("yes" because a single standard refresh driver does sometime refreshes fast enough to be clamped by this 2x-default-rate cutoff, based on my testing, due to imperfect timeout estimates between samples, or something like that.)

The point of using DefaultInterval() / 2 is to make it harder for this clamping to accidentally impose something *worse* than 60 FPS.

As a simple example for how this could happen, let's say we clamp to DefaultInterval(), and we have a single client whose timer alternates between being a bit faster than DefaultInterval & being a bit slower than DefaultInterval, due to random variation. In that situation, we'd force every other frame to be skipped, and that would be bad.

> Can we just initialize recentThreshold with this value directly? That way we
> can get rid of isRecentThresholdInitialized, and the compiler will generate
> code that is pretty much equivalent to what you have here.

Not quite equivalent; IIUC, if we dropped the bool and called FromMilliseconds/DefaultInterval() direcly, the compiler would generate a static initializer, which would run at startup, and make startup a bit slower.  Taras went on a crusade against static initializers a few years back, and I've got a reflexive avoidance of them as a result. :) (I think we try to avoid them unless they're absolutely necessary.)

Maybe you're saying the compiler could be smart enough to figure out that this particular static function-call always returns the same thing (and hence can be expanded at compile time)? I'm not sure we can trust the compiler to be that smart, given that the function definition is in a different compilation unit. (though I'm not a compiler expert, and it's possible I'm being too pessimistic)

> >  RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> 
> Should HadRecentRefresh just be using aTime?

Oh, yeah -- good point. I think I initially intended to use |aTime|, but I thought one of the potential callers might not have it, or something. But all of the callers definitely have it at this point, so I should probably be using it.

I'll give that a whirl tomorrow. Thanks!

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> The point of using DefaultInterval() / 2 is to make it harder for this
> clamping to accidentally impose something *worse* than 60 FPS.

Sure. If someone makes a refresh driver that's significantly faster, though, we'd still be clamping them. If the default is 60 FPS I guess that's not a problem in practice since I imagine we're not trying to animate images much faster than 60 FPS very often. =)

> Not quite equivalent; IIUC, if we dropped the bool and called
> FromMilliseconds/DefaultInterval() direcly, the compiler would generate a
> static initializer, which would run at startup, and make startup a bit
> slower.  Taras went on a crusade against static initializers a few years
> back, and I've got a reflexive avoidance of them as a result. :) (I think we
> try to avoid them unless they're absolutely necessary.)

That is the behavior of a static initializer for a global. Those are indeed totally evil, and not just because they make startup slower, but because it's impossible to reason about the order in which the initializers run. However, a static initializer for a function-local static variable does exactly what your code does - it lazily initializes the value the first time the function is called. Those are totally safe to use.

> I'll give that a whirl tomorrow. Thanks!

Nice!

Comment 6

[Boris Zbarsky [:bzbarsky]]

At some point we might move the refresh driver to vsync, which will then depend on the monitor frequency...

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> At some point we might move the refresh driver to vsync, which will then
> depend on the monitor frequency...

Possibly it's worth adding a comment somewhere to remind anyone who makes that change to update this code?

I'm not 100% sure what the best place is to add such a comment, but presumably somewhere in the nsRefreshDriver code.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v2

(In reply to Seth Fowler [:seth] from comment #5)
> Sure. If someone makes a refresh driver that's significantly faster, though,
> we'd still be clamping them. If the default is 60 FPS I guess that's not a
> problem in practice since I imagine we're not trying to animate images much
> faster than 60 FPS very often. =)

Yeah, I think the rate here (up to 2*60=120 fps) should be more than fast enough for any current usages.

If we end up needing *significantly* faster-than-60fps refreshes, then at that point we'll probably want to reconsider making all callers of DefaultInterval() (including this one) do something smarter.

> However, a static initializer for a function-local static variable does
> exactly what your code does

Ah! Right, I'd forgotten about that distinction. In that case, initializing it directly is much simpler; thanks for catching that. :)

Here's an updated patch, with the recent threshold initialized directly & with aTime used instead of CurrentTime().

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> At some point we might move the refresh driver to vsync, which will then
> depend on the monitor frequency...

(I'm not sure what that would end up meaning for this code, but I could imagine vsync-based imgIContainer::RequestRefresh() calls coming in with a special flag, perhaps, and having custom behavior if we see that flag, if necessary. Not sure it would be necessary, though; if we fire all refresh drivers on vsync, then I'd expect this to just work. I think the only problem would be if vsync was faster than 120 FPS; if that ends up happening & causing any noticable problems (doubtful), we can add smarter logic to figure that out & handle that.)

(In reply to Seth Fowler [:seth] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > At some point we might move the refresh driver to vsync, which will then
> > depend on the monitor frequency...
> 
> Possibly it's worth adding a comment somewhere to remind anyone who makes
> that change to update this code?
> 
> I'm not 100% sure what the best place is to add such a comment but
> presumably somewhere in the nsRefreshDriver code.

Good news! There's already a comment at the top of nsRefreshDriver.cpp:

> 16  * [...]  In the future, additional timer types may
> 17  * implement things like blocking on vsync.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp?rev=c5aae1b3dc3f#16

:)

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8419035 [details] [diff] [review]
fix v2

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

Looks good!

Comment 11

[Daniel Holbert [:dholbert]]

https://tbpl.mozilla.org/?tree=Try&rev=0fe74fb66a09

Comment 12

[Daniel Holbert [:dholbert]]

(oops, that try run was with fix v1 instead of v2. Still, they're close enough to make me happy to land the final version, once the tree reopens.)

Comment 13

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa947e53a790

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/fa947e53a790