Bug 38233 – Slow rendering w/multiple animated resizes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Stephen White]

WebKit rendering is slow (5fps) when multiple images have animated resizes, such as http://ie.microsoft.com/testdrive/Performance/01FlyingImages/Default.html.  (Firefox renders this demo at 15-60fps).

[Stephen White]

Some history:  back in http://trac.webkit.org/changeset/34210, Dave Hyatt introduced a speedup for GUIMark.  The heuristic is (roughly) "did we get another resize request for this image within the last 50ms?  If so, render in low quality, set a timer, and render in high quality when the timer expires".
This is a good idea, but won't work in this case for a couple of reasons:

1)  The heuristic depends on render time.  If the scene is complex enough (ie., takes more than 50ms), the condition will never be met, and we will always render at high quality.  (In the case of Chrome, our high quality render is slow enough that this happens even for GUIMark, and we never fall into the low quality path).

2)  There are multiple images in the scene, each with their own timer.  When one image's timer expires (telling it to render at high quality), it may be overlapping another image which has already rendered at high quality, and the redraw will be considered "another (quick) resize request" and the images will toggle back-and-forth between high and low quality (this only happens if #1 is fixed).

3)  There is a "sticky bit" that keeps the render at the previous quality if it renders at the same size.  In this case, it causes a slowdown since one image may not be resizing on a particular frame, while others are, so the non-resizing images render at high quality while the others render at low, until that image rotates far enough to also need a resize.

[Stephen White]

Created attachment 54717 [details]
Patch

[Stephen White]

Consider this patch a request for comments on a work-in-progress, since this change fixes things for Chromium, but isn't optimal for Safari yet (see * below).

This addresses the issues above as follows:

1)  This is fixed by recording the render time of the previous frame, and subtracting that off the difference between the last-requested resize and the current time.  This is intended to factor out render time from the heuristic used to determine whether to draw low quality or not.

2)  There is now a single timer for all outstanding resizes, owned by RenderImageScaleObserver (which I turned into a singleton -- not sure what WebKit policy is on those; let me know if I should implement this another way).

3)  There is no "sticky bit" anymore.  Each redraw (whether timer-driven or not) computes the heuristic independently, and uses that to determine whether to draw high or low quality.

Here are the benchmarks:

flying images

chrome/win before:  5-6fps
chrome/win after:     60fps
chrome/mac before:  5-6fps
chrome/mac after:   45fps
safari before:  5-6 FPS
safari after:   5-6 fps, then 63 fps once it gets "unstuck" *

GUIMark:

chrome/win before:  11fps
chrome/win after:   29fps
chrome/mac before:  55fps
chrome/mac after:   55fps
safari before:   55fps
safari after:    55fps

The changes in platform/graphics/skia/ are plumbing to implement setImageInterpolationQuality() in Chromium.

The changes in page/ and rendering/ implement the heuristic above.

[Stephen White]

Created attachment 55027 [details]
Patch

[Stephen White]

(In reply to comment #4)

It turns out the reason this fix works on Chrome but sticks on Safari is that there are 3 separate damage rects:  one for the moving images, one for the FPS and one for the text at the bottom that get invalidated every frame.  Chrome coalesces invalidation regions into one big rect, so it records a single (long) paint time for the whole frame.  Safari does not, so there are alternating slow and fast repaints, and the fast repaints store a short paint duration into the FrameView, which messes up the "how fast is this resizing" heuristic.

In this patch, I reverted the changes to FrameView to record render time, and simply increased the threshold to 500ms (as it is for resized background images in RenderBoxModelObject).  This works for this test case (and GUIMark), but has the same scalability problems as the original code:  it's always possible to come up with a more complex scene whose render time  I'd like to come up with a heuristic that works, but for now I'm just using the same approach as the existing code.

[Simon Fraser (smfr)]

(In reply to comment #3)

> The changes in platform/graphics/skia/ are plumbing to implement
> setImageInterpolationQuality() in Chromium.

Please do these in a separate patch. You can/should do these before the rest of the changes.

[Stephen White]

OK, I've broken out the Chromium-specific stuff as https://bugs.webkit.org/show_bug.cgi?id=38686.

[Stephen White]

Created attachment 57367 [details]
Patch

[Stephen White]

(In reply to comment #8)
> Created an attachment (id=57367) [details]
> Patch

Same as patch #2, but with all the Chromium-related plumbing removed.

[Simon Fraser (smfr)]

Comment on attachment 57367 [details]
Patch

> Index: WebCore/ChangeLog
> ===================================================================
> +
> +        This improves Safari performance for the following IE9 platform demos on my C2D MacPro (10.5):

This doesn't help me understand what your patch is changing. You should give a high-level summary of
1) the underlying problem
2) how this patch addresses it.

This will help me review the rest of the patch.

[Stephen White]

(In reply to comment #10)
> (From update of attachment 57367 [details])
> > Index: WebCore/ChangeLog
> > ===================================================================
> > +
> > +        This improves Safari performance for the following IE9 platform demos on my C2D MacPro (10.5):
> 
> This doesn't help me understand what your patch is changing. You should give a high-level summary of
> 1) the underlying problem
> 2) how this patch addresses it.
> 
> This will help me review the rest of the patch.

Description and Comment #1 describe the overall problem, and Comments #3 and #5 describe how they are addressed.  I'll try to summarize everything here:

The heuristic introduced in trac.webkit.org/changeset/34210 is a good idea, and works for a single animated resize, but when multiple images are resizing simultaneously, it doesn't work.

1)  The heuristic depends on render time.  If the scene is complex enough (ie., takes more than 50ms), the condition will never be met, and we will always render at high quality.

Solution:  bump the threshold to 500ms.  This is not ideal (my original approach was to try to measure time between repaints, exclusive of draw time, but that didn't work in Safari).  However, it is the same threshold as that used for background image resizing.

2)  There are multiple images in the scene, each with their own timer.

Solution:  Unify all the timers into a single timer, turn RenderImageScaleObserver into a singleton, and have it maintain the list of pending images to be redrawn.

3)  There is a "sticky bit" that keeps the render at the previous quality if it renders at the same size.  If an image which is "sticking" at high quality overlaps an image attempting to draw at low quality, it slows down the redraw.

Solution:  There is no "sticky bit" anymore.  Each redraw (whether timer-driven or not) computes the heuristic independently, and uses that to determine whether to draw high or low quality.

[Oliver Hunt]

Comment on attachment 57367 [details]
Patch

There are two problems i have with this patch 
* This optimisation also exists for scaled background-images, but i think has (effectively) a duplicate of the code.
* I think the cut off logic is bad -- We shouldn't be looking at how long between frames, we should be looking at how long it takes to do the paint.  I think a "workable" approach maybe to trigger with something along the lines of:
   1. if shouldUseNormalQuality || (scale == FloatSize(1.0, 1.0))
      a. setScalingMode()
      b. paint normally
      c. return
   2. startTime = currentTime()
   3. paint normally
   4. endTime = currentTime();
   5. shouldUseNormalQuality = (endTime-startTime) > magicValue

[Stephen White]

(In reply to comment #12)
> (From update of attachment 57367 [details])
> There are two problems i have with this patch 
> * This optimisation also exists for scaled background-images, but i think has (effectively) a duplicate of the code.

Hmm.  So you'd like me to refactor the two using the algorithm below?  I can give that a try.

> * I think the cut off logic is bad -- We shouldn't be looking at how long between frames, we should be looking at how long it takes to do the paint.  I think a "workable" approach maybe to trigger with something along the lines of:
>    1. if shouldUseNormalQuality || (scale == FloatSize(1.0, 1.0))
>       a. setScalingMode()
>       b. paint normally
>       c. return
>    2. startTime = currentTime()
>    3. paint normally
>    4. endTime = currentTime();
>    5. shouldUseNormalQuality = (endTime-startTime) > magicValue

Interesting.  Three potential problems with this approach:

1)  Images which are resized more than once, will remain at low quality after the second paint.  E.g., an image which animates for a while, and then stops animating, while remain at low quality.  See the flickr demo, for example.

2)  A scene consisting of many many small images resizing may not trigger the "fast" rendering, even though its total render time is much more than a scene consisting of one larger image.  It may be tricky to get the threshold correct for something like the "flying images" demo, for example, in a way that doesn't basically just cause fast (low quality) rendering always.

3)  Consider an image resizing from very small to very large and back again.  Since the time to resize a single image depends on the number of pixels covered, an image may not trigger the fast case until it's drawn large enough.  Once it is, though, as in #1, it won't go back, even if it stops.

I'm willing to give it a try, but I thought I'd point this out before I start.

[Stephen White]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 57367 [details] [details])
> > There are two problems i have with this patch 
> > * This optimisation also exists for scaled background-images, but i think has (effectively) a duplicate of the code.
> 
> Hmm.  So you'd like me to refactor the two using the algorithm below?  I can give that a try.
> 
> > * I think the cut off logic is bad -- We shouldn't be looking at how long between frames, we should be looking at how long it takes to do the paint.  I think a "workable" approach maybe to trigger with something along the lines of:
> >    1. if shouldUseNormalQuality || (scale == FloatSize(1.0, 1.0))
> >       a. setScalingMode()
> >       b. paint normally
> >       c. return
> >    2. startTime = currentTime()
> >    3. paint normally
> >    4. endTime = currentTime();
> >    5. shouldUseNormalQuality = (endTime-startTime) > magicValue
> 
> Interesting.  Three potential problems with this approach:
> 
> 1)  Images which are resized more than once, will remain at low quality after the second paint.  E.g., an image which animates for a while, and then stops animating, while remain at low quality.  See the flickr demo, for example.
> 
> 2)  A scene consisting of many many small images resizing may not trigger the "fast" rendering, even though its total render time is much more than a scene consisting of one larger image.  It may be tricky to get the threshold correct for something like the "flying images" demo, for example, in a way that doesn't basically just cause fast (low quality) rendering always.
> 
> 3)  Consider an image resizing from very small to very large and back again.  Since the time to resize a single image depends on the number of pixels covered, an image may not trigger the fast case until it's drawn large enough.  Once it is, though, as in #1, it won't go back, even if it stops.

After thinking about/discussing it some more, if the low quality resize becomes sticky, this would be a showstopper for us.  So I can't really go ahead with that proposal as-is.

I will try refactoring this code with the background image resize.

[Oliver Hunt]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 57367 [details] [details] [details])
> > > There are two problems i have with this patch 
> > > * This optimisation also exists for scaled background-images, but i think has (effectively) a duplicate of the code.
> > 
> > Hmm.  So you'd like me to refactor the two using the algorithm below?  I can give that a try.
> > 
> > > * I think the cut off logic is bad -- We shouldn't be looking at how long between frames, we should be looking at how long it takes to do the paint.  I think a "workable" approach maybe to trigger with something along the lines of:
> > >    1. if shouldUseNormalQuality || (scale == FloatSize(1.0, 1.0))
> > >       a. setScalingMode()
> > >       b. paint normally
> > >       c. return
> > >    2. startTime = currentTime()
> > >    3. paint normally
> > >    4. endTime = currentTime();
> > >    5. shouldUseNormalQuality = (endTime-startTime) > magicValue
> > 
> > Interesting.  Three potential problems with this approach:
> > 
> > 1)  Images which are resized more than once, will remain at low quality after the second paint.  E.g., an image which animates for a while, and then stops animating, while remain at low quality.  See the flickr demo, for example.
> > 
> > 2)  A scene consisting of many many small images resizing may not trigger the "fast" rendering, even though its total render time is much more than a scene consisting of one larger image.  It may be tricky to get the threshold correct for something like the "flying images" demo, for example, in a way that doesn't basically just cause fast (low quality) rendering always.
> > 
> > 3)  Consider an image resizing from very small to very large and back again.  Since the time to resize a single image depends on the number of pixels covered, an image may not trigger the fast case until it's drawn large enough.  Once it is, though, as in #1, it won't go back, even if it stops.
> 
> After thinking about/discussing it some more, if the low quality resize becomes sticky, this would be a showstopper for us.  So I can't really go ahead with that proposal as-is.
> 
> I will try refactoring this code with the background image resize.

I'm not sure what the skia issue is -- for webkit with the CG backend the problem we have is by default we use trilinear filtering (vs. bilinear for everyone else) which makes scaling much more expensive than in other browsers.  Anyhoo, so we should be dropping into bilinear filter rather than trilinear so it's not an "ugly" vs. "non-ugly" issue but rather "good" vs. "excessive"

[Stephen White]

(In reply to comment #15)

> I'm not sure what the skia issue is -- for webkit with the CG backend the problem we have is by default we use trilinear filtering (vs. bilinear for everyone else) which makes scaling much more expensive than in other browsers.  Anyhoo, so we should be dropping into bilinear filter rather than trilinear so it's not an "ugly" vs. "non-ugly" issue but rather "good" vs. "excessive"

In the Skia backend, the default is Lanczos3, and the "low quality" is bilinear (or at least, it will be once 38686 lands -- before that, there were some heuristics in the backend to switch between the two, but 38686 hooks up the setInterpolationQuality() call, so that it behaves much more like the CG backend, and will use Lanczos3 less).  The bilinear is fast and heavily optimized, the Lanczos3 is beautiful but slow, and memory bandwidth heavy, so I doubt it will come close to bilinear even if optimized.

Note that the numbers above are from Safari, not Chrome, and show a 5x-10x perf delta.  Chrome's performance delta is larger, but not hugely so.  So this is an issue that Chrome and Safari both share, so I was hoping to fix them both.

[Stephen White]

Created attachment 57547 [details]
Patch

[Stephen White]

Comment on attachment 57547 [details]
Patch

This patch refactors the implementations in RenderBoxModelObject and RenderImage.

[Dave Hyatt]

We just need to make sure iChat chat window resizing with a scaled photo (the default) in it doesn't get slow.  Olliej held off on changing RenderImage to 500ms because of this.  We need to test it to make sure we don't break it with a 500ms threshold.  I don't think we will, but we should just make sure.

run-webkit-app would work great for launching iChat and checking.

[Stephen White]

(In reply to comment #19)
> We just need to make sure iChat chat window resizing with a scaled photo (the default) in it doesn't get slow.  Olliej held off on changing RenderImage to 500ms because of this.  We need to test it to make sure we don't break it with a 500ms threshold.  I don't think we will, but we should just make sure.
> 
> run-webkit-app would work great for launching iChat and checking.

OK, it seems to be ok.   I tried it with iChat, and it seems to do low quality while dragging, then high quality on mouseup.  Seems to be the same as without this change.

I also tried a large IMG with WIDTH=100%, and it used the new algorithm:  rendered at low quality until no mouse events for 500ms, then rendered at high quality.

[Stephen White]

Created attachment 58284 [details]
Patch

[Stephen White]

(In reply to comment #21)
> Created an attachment (id=58284) [details]
> Patch

Minor cleanup from previous patch:  removed some redundant class scope operators.

[David Levin]

Comment on attachment 58284 [details]
Patch

These are my 1st pass comments, but there are enough of them that I decided to enter them before doing my second pass comments. (My 1st pass tends to be simpler to spot items, more stylistic, etc. My second pass is where I finish getting a big picture and try to verify correctness. I'll try to start a second pass soon.)

---------------------------------
http://wkrietveld.appspot.com/38233/diff/1/2
File WebCore/ChangeLog (right):

http://wkrietveld.appspot.com/38233/diff/1/2#newcode6
WebCore/ChangeLog:6: https://bugs.webkit.org/show_bug.cgi?id=38233
I would add a return here.

http://wkrietveld.appspot.com/38233/diff/1/3
File WebCore/rendering/RenderBoxModelObject.cpp (left):

http://wkrietveld.appspot.com/38233/diff/1/3#oldcode134
WebCore/rendering/RenderBoxModelObject.cpp:134: bool contextIsScaled = !currentTransform.isIdentityOrTranslationOrFlipped();
Why are we now using "size" instead of the transform as was done here before? 

I see this matched what the other instance of this code was doing, but did we loose something from this instance by doing this change?

http://wkrietveld.appspot.com/38233/diff/1/3
File WebCore/rendering/RenderBoxModelObject.cpp (right):

http://wkrietveld.appspot.com/38233/diff/1/3#newcode6
WebCore/rendering/RenderBoxModelObject.cpp:6: * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
You should add a copyright line here and other files where you made changes of more than a few lines.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode51
WebCore/rendering/RenderBoxModelObject.cpp:51: RenderBoxModelObject::ScaleObserver::ScaleObserver() : m_objects(0), m_timer(this, &ScaleObserver::highQualityRepaintTimerFired) {}
The initializers are typically put on separate lines (as RenderBoxModelScaleData did before) and then the {} would be on separate lines as well.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode62
WebCore/rendering/RenderBoxModelObject.cpp:62: if (m_objects) {
Please consider the fail fast approach.

if (!m_objects)
    return;

etc.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode63
WebCore/rendering/RenderBoxModelObject.cpp:63: m_objects->take(object);
Just use "remove" instead of "take" since you don't need the object returned.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode66
WebCore/rendering/RenderBoxModelObject.cpp:66: m_objects = 0;
m_timer.stop(); ?

Maybe that isn't worth it.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode73
WebCore/rendering/RenderBoxModelObject.cpp:73: if (m_objects) {
fail fast?

http://wkrietveld.appspot.com/38233/diff/1/3#newcode81
WebCore/rendering/RenderBoxModelObject.cpp:81: m_timer.stop();
There is no need to call stop, since you are setting the time on the next line. There is only one firing time that this timer may hold (see WebCore/platform/Timer.cpp).

http://wkrietveld.appspot.com/38233/diff/1/3#newcode102
WebCore/rendering/RenderBoxModelObject.cpp:102: // There is no scale in effect.  If we had a scale in effect before, we can just remove this object from the list.
One space after periods in WebKit comments.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
The { should go away since the statement is now one line.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
What is "if (time)" suppose to be checking for? that m_objects exists? If so, why not just check m_objects?

"m_objects->get(object)" will return "static T emptyValue() { return std::numeric_limits<T>::infinity(); }" if the object doesn't exist in the hashmap.

Before when the value was a pointer, the default value would be 0, so this code is different from before.

http://wkrietveld.appspot.com/38233/diff/1/3#newcode118
WebCore/rendering/RenderBoxModelObject.cpp:118: if (!time) {
What is !time checking for? (see comment above).

http://wkrietveld.appspot.com/38233/diff/1/3#newcode127
WebCore/rendering/RenderBoxModelObject.cpp:127: // If it has been at least <threshold> seconds since the last time a
Is "<threshold>" == cLowQualityTimeThreshold ?

http://wkrietveld.appspot.com/38233/diff/1/3#newcode128
WebCore/rendering/RenderBoxModelObject.cpp:128: // resize was requested, draw at hi quality and don't set the timer.
s/hi/high/

http://wkrietveld.appspot.com/38233/diff/1/3#newcode132
WebCore/rendering/RenderBoxModelObject.cpp:132: // Draw at low quality first, and set a timer for HQ.
HQ: Please avoid abreviations.

Also no comma before and (just like you did above for a similar comment).

http://wkrietveld.appspot.com/38233/diff/1/4
File WebCore/rendering/RenderBoxModelObject.h (right):

http://wkrietveld.appspot.com/38233/diff/1/4#newcode108
WebCore/rendering/RenderBoxModelObject.h:108: typedef HashMap<RenderBoxModelObject*, double> ScaleMap;
It seems like this file should include wtf/HashMap.

ScaleMap doesn't really give you scales or map scales to something. It maps RenderBoxModelObject to the time that object last painted. Can you come up with a better name?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode110
WebCore/rendering/RenderBoxModelObject.h:110: class ScaleObserver : public Noncopyable {
Nice use of Noncopyable!

http://wkrietveld.appspot.com/38233/diff/1/4#newcode112
WebCore/rendering/RenderBoxModelObject.h:112: ScaleObserver();
Why is the constructor public?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode113
WebCore/rendering/RenderBoxModelObject.h:113: static ScaleObserver* getInstance();
Misc idea: What about "instance()" as the name?

I wonder getInstance() is even exposed.  It looks like every time this is called, one does this:

ScaleObserver::getInstance()>method...

So why not just hide getInstance and make a few static methods that do this for you?

In fact, you could just expose the the static methods on  RenderBoxModelObject (not sure if method names will need to change) and then just hide the whole class inside of RenderBoxModelObject.cpp.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode115
WebCore/rendering/RenderBoxModelObject.h:115: void objectDestroyed(RenderBoxModelObject* object);
"object" shouldn't be here.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode116
WebCore/rendering/RenderBoxModelObject.h:116: void highQualityRepaintTimerFired(Timer<ScaleObserver>*);
highQualityRepaintTimerFire should be private.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
Please add a blank line here before private:

http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
restartTimer should be private.

http://wkrietveld.appspot.com/38233/diff/1/4#newcode119
WebCore/rendering/RenderBoxModelObject.h:119: ScaleMap* m_objects;
Every time I see m_objects, I don't know what it is.

It seems to be a mapping of RenderBoxModelObject's that are painted at low quality to the last time at which they were painted. Is that correct? Can this name be more descriptive (but not as long as my sentence :) )?

http://wkrietveld.appspot.com/38233/diff/1/4#newcode121
WebCore/rendering/RenderBoxModelObject.h:121: static ScaleObserver* gInstance;
Recent style guideline addition "Static data members should be prefixed by "s_". "

[Stephen White]

(In reply to comment #23)

(I'm going to send these comments, although I won't be uploading a new version just yet -- having some issues).

> http://wkrietveld.appspot.com/38233/diff/1/2#newcode6
> WebCore/ChangeLog:6: https://bugs.webkit.org/show_bug.cgi?id=38233
> I would add a return here.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3
> File WebCore/rendering/RenderBoxModelObject.cpp (left):
> 
> http://wkrietveld.appspot.com/38233/diff/1/3#oldcode134
> WebCore/rendering/RenderBoxModelObject.cpp:134: bool contextIsScaled = !currentTransform.isIdentityOrTranslationOrFlipped();
> Why are we now using "size" instead of the transform as was done here before? 
> I see this matched what the other instance of this code was doing, but did we loose something from this instance by doing this change?

We're still doing the check for size, we're just not doing the check for identity transform.  I pulled this out when I refactored with RenderImage (which had no such check), but to be honest, I'm not sure if it's 100% correct.

Oliver, could you take a look?  Should I try to preserve this check?

> http://wkrietveld.appspot.com/38233/diff/1/3
> File WebCore/rendering/RenderBoxModelObject.cpp (right):
> 
> http://wkrietveld.appspot.com/38233/diff/1/3#newcode6
> WebCore/rendering/RenderBoxModelObject.cpp:6: * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
> You should add a copyright line here and other files where you made changes of more than a few lines.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode51
> WebCore/rendering/RenderBoxModelObject.cpp:51: RenderBoxModelObject::ScaleObserver::ScaleObserver() : m_objects(0), m_timer(this, &ScaleObserver::highQualityRepaintTimerFired) {}
> The initializers are typically put on separate lines (as RenderBoxModelScaleData did before) and then the {} would be on separate lines as well.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode62
> WebCore/rendering/RenderBoxModelObject.cpp:62: if (m_objects) {
> Please consider the fail fast approach.
> 
> if (!m_objects)
>     return;
> 
> etc.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode63
> WebCore/rendering/RenderBoxModelObject.cpp:63: m_objects->take(object);
> Just use "remove" instead of "take" since you don't need the object returned.

Done.

> 
> http://wkrietveld.appspot.com/38233/diff/1/3#newcode66
> WebCore/rendering/RenderBoxModelObject.cpp:66: m_objects = 0;
> m_timer.stop(); ?
> 
> Maybe that isn't worth it.

Done.  (Why not.)

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode73
> WebCore/rendering/RenderBoxModelObject.cpp:73: if (m_objects) {
> fail fast?

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode81
> WebCore/rendering/RenderBoxModelObject.cpp:81: m_timer.stop();
> There is no need to call stop, since you are setting the time on the next line. There is only one firing time that this timer may hold (see WebCore/platform/Timer.cpp).

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode102
> WebCore/rendering/RenderBoxModelObject.cpp:102: // There is no scale in effect.  If we had a scale in effect before, we can just remove this object from the list.
> One space after periods in WebKit comments.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
> WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
> The { should go away since the statement is now one line.
> 
> http://wkrietveld.appspot.com/38233/diff/1/3#newcode103
> WebCore/rendering/RenderBoxModelObject.cpp:103: if (time) {
> What is "if (time)" suppose to be checking for? that m_objects exists? If so, why not just check m_objects?
> 
> "m_objects->get(object)" will return "static T emptyValue() { return std::numeric_limits<T>::infinity(); }" if the object doesn't exist in the hashmap.

You're right.  Changed to compare against HashTraits<double>::emptyValue() instead.

> Before when the value was a pointer, the default value would be 0, so this code is different from before.
> 
> http://wkrietveld.appspot.com/38233/diff/1/3#newcode118
> WebCore/rendering/RenderBoxModelObject.cpp:118: if (!time) {
> What is !time checking for? (see comment above).

Fixed.  In fact, we don't want this check anymore, since we're always going to draw at low quality first (since time never == 0, we were never falling into this case, so it should have been removed anyway).

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode127
> WebCore/rendering/RenderBoxModelObject.cpp:127: // If it has been at least <threshold> seconds since the last time a
> Is "<threshold>" == cLowQualityTimeThreshold ?

Fixed.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode128
> WebCore/rendering/RenderBoxModelObject.cpp:128: // resize was requested, draw at hi quality and don't set the timer.
> s/hi/high/

Fixed.

> http://wkrietveld.appspot.com/38233/diff/1/3#newcode132
> WebCore/rendering/RenderBoxModelObject.cpp:132: // Draw at low quality first, and set a timer for HQ.
> HQ: Please avoid abreviations.

Done.

> Also no comma before and (just like you did above for a similar comment).

Done.

> http://wkrietveld.appspot.com/38233/diff/1/4
> File WebCore/rendering/RenderBoxModelObject.h (right):
> 
> http://wkrietveld.appspot.com/38233/diff/1/4#newcode108
> WebCore/rendering/RenderBoxModelObject.h:108: typedef HashMap<RenderBoxModelObject*, double> ScaleMap;
> It seems like this file should include wtf/HashMap.

Done.

> ScaleMap doesn't really give you scales or map scales to something. It maps RenderBoxModelObject to the time that object last painted. Can you come up with a better name?

Changed to LastPaintTimeMap, as discussed.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode110
> WebCore/rendering/RenderBoxModelObject.h:110: class ScaleObserver : public Noncopyable {
> Nice use of Noncopyable!
> 
> http://wkrietveld.appspot.com/38233/diff/1/4#newcode112
> WebCore/rendering/RenderBoxModelObject.h:112: ScaleObserver();
> Why is the constructor public?

Fixed.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode113
> WebCore/rendering/RenderBoxModelObject.h:113: static ScaleObserver* getInstance();
> Misc idea: What about "instance()" as the name?

I was following the Singleton design pattern, where it's usually called getInstance().  I'll change it if you like.

> I wonder getInstance() is even exposed.  It looks like every time this is called, one does this:
> 
> ScaleObserver::getInstance()>method...
> 
> So why not just hide getInstance and make a few static methods that do this for you?
> 
> In fact, you could just expose the the static methods on  RenderBoxModelObject (not sure if method names will need to change) and then just hide the whole class inside of RenderBoxModelObject.cpp.

Good idea.  Done.

(Note that since RenderImage derives from RenderBoxModelObject, the call to objectDestroyed() in its destructor was redundant, and was removed.)

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode115
> WebCore/rendering/RenderBoxModelObject.h:115: void objectDestroyed(RenderBoxModelObject* object);
> "object" shouldn't be here.

Fixed.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode116
> WebCore/rendering/RenderBoxModelObject.h:116: void highQualityRepaintTimerFired(Timer<ScaleObserver>*);
> highQualityRepaintTimerFire should be private.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
> WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
> Please add a blank line here before private:

Done.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode117
> WebCore/rendering/RenderBoxModelObject.h:117: void restartTimer();
> restartTimer should be private.

Done.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode119
> WebCore/rendering/RenderBoxModelObject.h:119: ScaleMap* m_objects;
> Every time I see m_objects, I don't know what it is.
> It seems to be a mapping of RenderBoxModelObject's that are painted at low quality to the last time at which they were painted. Is that correct? Can this name be more descriptive (but not as long as my sentence :) )?

Changed to m_lastPaintTimeMap.

> http://wkrietveld.appspot.com/38233/diff/1/4#newcode121
> WebCore/rendering/RenderBoxModelObject.h:121: static ScaleObserver* gInstance;
> Recent style guideline addition "Static data members should be prefixed by "s_". "

Done.

[Stephen White]

Created attachment 58687 [details]
Patch

[Darin Adler]

Comment on attachment 58687 [details]
Patch

This is great! I think we'll want to use this for other things as well, and maybe it eventually won't be entirely tied to RenderBoxModelObject.

> +typedef HashMap<RenderBoxModelObject*, double> LastPaintTimeMap;
> +
> +class RenderBoxModelScaleObserver : public Noncopyable {
>  public:
> +    static RenderBoxModelScaleObserver* getInstance();
> +    bool shouldPaintAtLowQuality(GraphicsContext*, RenderBoxModelObject*, Image*, const IntSize&);
> +    void objectDestroyed(RenderBoxModelObject*);
>  
>  private:
> +    RenderBoxModelScaleObserver();
> +    void highQualityRepaintTimerFired(Timer<RenderBoxModelScaleObserver>*);
> +    void restartTimer();
> +    LastPaintTimeMap* m_lastPaintTimeMap;
> +    Timer<RenderBoxModelScaleObserver> m_timer;
> +    static RenderBoxModelScaleObserver* s_instance;
>  };

I normally find classes easier to read if they are paragraphed so that functions are split from the data. This one seems a little too cramped.

I think m_lastPaintTimeMap should be an OwnPtr even if we never do delete this object. Also, why use a pointer to the map. If we only ever have one of these objects it seems fine to just have the map in the object. No need for all that pointer dereferencing.

In WebKit code we do not use "get" in the names of functions like getInstance. Normally we'd call the function "sharedInstance" or something like that. Even "instance" would probably be OK.

I think the name RenderBoxModelScaleObserver is confusing, since is not a "scale observer". It does not observe scale. We should name the class based on what the object is. I think it’s an object that controls painting quality based on painting activity, so its name should reflect that. I also don't think its name needs to have a RenderBoxModel prefix. Maybe ImageQualityController would be a good name?

> +RenderBoxModelScaleObserver* RenderBoxModelScaleObserver::getInstance()
> +{
> +    if (!s_instance)
> +        s_instance = new RenderBoxModelScaleObserver;
> +    return s_instance;
> +}

A static data member is not the best idiom for this sort of thing. A better idiom is just a global variable scoped to the getter function.

    static ImageQualityController* imageQualityController()
    {
        static ImageQualityController* controller = new ImageQualityController;
        return controller;
    }

Since nothing outside the function accesses the global variable, there's no need for it to be a class static data member. I also think we can just let the function be a free function at the call site so it can be:
 
    imageQualityController()->objectDestroyed(this)

Rather than:

    RenderBoxModelScaleObserver::getInstance()->objectDestroyed(this)

I see no reason the policy that we use a single shared instance of this needs to be built into the class.

> +    if (!m_lastPaintTimeMap->size()) {

This should use isEmpty instead.

> -    if (!image || !image->isBitmapImage())
> +    if (!image || !image->isBitmapImage() || context->paintingDisabled())
>          return false;

Why this change? Did you run into a test case that showed this is needed? Seems OK to optimize like this but perhaps a little strange. We could also check if the image has a 0x0 size or is entirely clipped.

> +        // There is no scale in effect. If we had a scale in effect before, we can just remove this object from the list.
> +        if (time != HashTraits<double>::emptyValue())
> +            m_lastPaintTimeMap->remove(object);

To find out if something is in the map I think it's best to use find rather than get. With find you can compare with end to detect if the item was in the map. Using HashTraits::emptyValue explicitly like this is more awkward.

> +    bool shouldPaintAtLowQuality(GraphicsContext*, Image* image, const IntSize& size);

The "image" and "size" argument names here should be omitted.

This function should be protected, unless there's some reason for it to be public.

review- because I think you should do at least some of what I suggest

[Stephen White]

Created attachment 58711 [details]
Patch

[Stephen White]

(In reply to comment #26)
> (From update of attachment 58687 [details])
> This is great! I think we'll want to use this for other things as well, and maybe it eventually won't be entirely tied to RenderBoxModelObject.
> 
> > +typedef HashMap<RenderBoxModelObject*, double> LastPaintTimeMap;
> > +
> > +class RenderBoxModelScaleObserver : public Noncopyable {
> >  public:
> > +    static RenderBoxModelScaleObserver* getInstance();
> > +    bool shouldPaintAtLowQuality(GraphicsContext*, RenderBoxModelObject*, Image*, const IntSize&);
> > +    void objectDestroyed(RenderBoxModelObject*);
> >  
> >  private:
> > +    RenderBoxModelScaleObserver();
> > +    void highQualityRepaintTimerFired(Timer<RenderBoxModelScaleObserver>*);
> > +    void restartTimer();
> > +    LastPaintTimeMap* m_lastPaintTimeMap;
> > +    Timer<RenderBoxModelScaleObserver> m_timer;
> > +    static RenderBoxModelScaleObserver* s_instance;
> >  };
> 
> I normally find classes easier to read if they are paragraphed so that functions are split from the data. This one seems a little too cramped.

Fixed.

> I think m_lastPaintTimeMap should be an OwnPtr even if we never do delete this object. Also, why use a pointer to the map. If we only ever have one of these objects it seems fine to just have the map in the object. No need for all that pointer dereferencing.

Done.  BTW, in RenderImageScaleObserver (from whence some of this code came), it was originally a pointer, I'm guessing to minimize memory use when the map was not in use.

> In WebKit code we do not use "get" in the names of functions like getInstance. Normally we'd call the function "sharedInstance" or something like that. Even "instance" would probably be OK.

Function removed (now using imageQualityController() file-level static, as you suggested below).

> I think the name RenderBoxModelScaleObserver is confusing, since is not a "scale observer". It does not observe scale. We should name the class based on what the object is. I think it’s an object that controls painting quality based on painting activity, so its name should reflect that. I also don't think its name needs to have a RenderBoxModel prefix. Maybe ImageQualityController would be a good name?

Done.

> > +RenderBoxModelScaleObserver* RenderBoxModelScaleObserver::getInstance()
> > +{
> > +    if (!s_instance)
> > +        s_instance = new RenderBoxModelScaleObserver;
> > +    return s_instance;
> > +}
> 
> A static data member is not the best idiom for this sort of thing. A better idiom is just a global variable scoped to the getter function.
> 
>     static ImageQualityController* imageQualityController()
>     {
>         static ImageQualityController* controller = new ImageQualityController;
>         return controller;
>     }
> Since nothing outside the function accesses the global variable, there's no need for it to be a class static data member. I also think we can just let the function be a free function at the call site so it can be:
> 
>     imageQualityController()->objectDestroyed(this)
> 
> Rather than:
> 
>     RenderBoxModelScaleObserver::getInstance()->objectDestroyed(this)
> 
> I see no reason the policy that we use a single shared instance of this needs to be built into the class.

Good idea; done.

> > +    if (!m_lastPaintTimeMap->size()) {
> 
> This should use isEmpty instead.

Done.

> > -    if (!image || !image->isBitmapImage())
> > +    if (!image || !image->isBitmapImage() || context->paintingDisabled())
> >          return false;
> 
> Why this change? Did you run into a test case that showed this is needed? Seems OK to optimize like this but perhaps a little strange. We could also check if the image has a 0x0 size or is entirely clipped.

Once I introduced the context for the RenderImage case (in the last patch), the call to context->getCTM() would assert, since the platform context was NULL (I'm assuming because it was called during initialization).  Putting in this check avoids the assert (and makes sense -- there's no reason to start the timer until we've actually painted something).

> > +        // There is no scale in effect. If we had a scale in effect before, we can just remove this object from the list.
> > +        if (time != HashTraits<double>::emptyValue())
> > +            m_lastPaintTimeMap->remove(object);
> 
> To find out if something is in the map I think it's best to use find rather than get. With find you can compare with end to detect if the item was in the map. Using HashTraits::emptyValue explicitly like this is more awkward.

Fixed to use find() and an iterator.  Not super-familiar with the wtf classes, so please check that I didn't mess something up.

> > +    bool shouldPaintAtLowQuality(GraphicsContext*, Image* image, const IntSize& size);
> 
> The "image" and "size" argument names here should be omitted.

Done.

> This function should be protected, unless there's some reason for it to be public.

Done.

> review- because I think you should do at least some of what I suggest

[Eric Seidel (no email)]

Attachment 58711 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3308146

[Early Warning System Bot]

Attachment 58711 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3278140

[Oliver Hunt]

Comment on attachment 58711 [details]
Patch

I'm not sure how this fixes the multiple scaling+animating images case.

For example if a page has n images, and it animates those n images with some scaling applied it seems that eventually n * <time to draw each image> would be greater than cLowQualityTimeThreshold and so stop the lower quality rendering just when it should be using it.

Am I misunderstanding the code?

--Oliver

[Stephen White]

(In reply to comment #31)
> (From update of attachment 58711 [details])
> I'm not sure how this fixes the multiple scaling+animating images case.
> 
> For example if a page has n images, and it animates those n images with some scaling applied it seems that eventually n * <time to draw each image> would be greater than cLowQualityTimeThreshold and so stop the lower quality rendering just when it should be using it.
> 
> Am I misunderstanding the code?
> 
> --Oliver

Yes, if your entire page takes more than 500ms to render, it'll never kick in.  This is the same problem that the original algorithm had in Chrome (and I'm assuming why 500ms was used for the background image version, instead of 50ms for RenderImage).

In my first patch, I tried to factor out rendering time by measuring it, but unfortunately this is quite tricky to do.  It worked in Chrome, but not in Safari, due to differing ways that damage rects are coalesced in the two browsers.

The current patch just extends the existing algorithm to cover multiple resizes, with the caveat that the total rendering time still has to be less than 500ms.

[Oliver Hunt]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 58711 [details] [details])
> > I'm not sure how this fixes the multiple scaling+animating images case.
> > 
> > For example if a page has n images, and it animates those n images with some scaling applied it seems that eventually n * <time to draw each image> would be greater than cLowQualityTimeThreshold and so stop the lower quality rendering just when it should be using it.
> > 
> > Am I misunderstanding the code?
> > 
> > --Oliver
> 
> Yes, if your entire page takes more than 500ms to render, it'll never kick in.  This is the same problem that the original algorithm had in Chrome (and I'm assuming why 500ms was used for the background image version, instead of 50ms for RenderImage).
> 
> In my first patch, I tried to factor out rendering time by measuring it, but unfortunately this is quite tricky to do.  It worked in Chrome, but not in Safari, due to differing ways that damage rects are coalesced in the two browsers.
> 
> The current patch just extends the existing algorithm to cover multiple resizes, with the caveat that the total rendering time still has to be less than 500ms.

Something i was considering was doing something along the lines of measuring how much time had passed since the last paint call, and if it was lower than a certain threshold use that as a flag to indicate that it may be worth recording information to trigger lower quality scaling?

[Stephen White]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (From update of attachment 58711 [details] [details] [details])
> > > I'm not sure how this fixes the multiple scaling+animating images case.
> > > 
> > > For example if a page has n images, and it animates those n images with some scaling applied it seems that eventually n * <time to draw each image> would be greater than cLowQualityTimeThreshold and so stop the lower quality rendering just when it should be using it.
> > > 
> > > Am I misunderstanding the code?
> > > 
> > > --Oliver
> > 
> > Yes, if your entire page takes more than 500ms to render, it'll never kick in.  This is the same problem that the original algorithm had in Chrome (and I'm assuming why 500ms was used for the background image version, instead of 50ms for RenderImage).
> > 
> > In my first patch, I tried to factor out rendering time by measuring it, but unfortunately this is quite tricky to do.  It worked in Chrome, but not in Safari, due to differing ways that damage rects are coalesced in the two browsers.
> > 
> > The current patch just extends the existing algorithm to cover multiple resizes, with the caveat that the total rendering time still has to be less than 500ms.
> 
> Something i was considering was doing something along the lines of measuring how much time had passed since the last paint call, and if it was lower than a certain threshold use that as a flag to indicate that it may be worth recording information to trigger lower quality scaling?

That's kind of what I did in the first patch.  The problem is, Safari was doing three separate paint calls (three damage rects) for each draw.  So even on a single draw, where no animation was happening, there would be two quick "zero time between paint" events.  So it's hard to distinguish between that and a timer-based redraw loop.

[Stephen White]

Created attachment 58718 [details]
Patch

[Stephen White]

r-'ing myself.

During additional testing, I found an issue where the system could end up toggling endlessly between high and low quality.  I originally thought I had fixed this problem by introducing a factor of 2.0 between the time threshold and the timer, but this was just masking the problem.

The real fix here is not to do a high quality draw if the timer is still outstanding.  This fixes the flickering problem, and lets me lower the timer:threshold factor to 1.05 (so that it just covers the threshold).  Patch upcoming.

[David Levin]

(In reply to comment #36)
> r-'ing myself.

Thanks I was just looking at this.


> During additional testing, I found an issue where the system could end up toggling endlessly between high and low quality.  I originally thought I had fixed this problem by introducing a factor of 2.0 between the time threshold and the timer, but this was just masking the problem.
> 
> The real fix here is not to do a high quality draw if the timer is still outstanding.  This fixes the flickering problem, and lets me lower the timer:threshold factor to 1.05 (so that it just covers the threshold).  Patch upcoming.

I validated that your previous patch didn't regress (nor improve) GUIMark and GUIMark 2.

When I looked at the checkins for the code that you are changing, I found this comment:

   https://bugs.webkit.org/show_bug.cgi?id=33808#c21 (and then comment 24)

which is interesting. Note that this is what resulted in the transform check that you are removing.

In short, please *make sure to test the new tab page* in Chromium to verify that the problem isn't reintroduced.

[Stephen White]

Created attachment 58996 [details]
Patch

[Stephen White]

Created attachment 59020 [details]
Patch

[Stephen White]

(In reply to comment #39)
> Created an attachment (id=59020) [details]
> Patch

No code changes; added expected pixel test failures to test_expectations.txt.  Updated ChangeLogs.

[Stephen White]

I responded to David in private; posting here for posterity.

(In reply to comment #37)
> When I looked at the checkins for the code that you are changing, I found this comment:
> 
>    https://bugs.webkit.org/show_bug.cgi?id=33808#c21 (and then comment 24)
> 
> which is interesting. Note that this is what resulted in the transform check that you are removing.

Note that I put the transform check back (and added it also for the RenderImage case).

> In short, please *make sure to test the new tab page* in Chromium to verify that the problem isn't reintroduced.

Checked.  The NTP seems fine, and a previous version of this change successfully passed the trybots.

[Stephen White]

Committed r61341: <http://trac.webkit.org/changeset/61341>

[Mark Rowe (bdash)]

This change caused bug 41036.