Bug 42390 – Multi-layer backgrounds repaint (in low quality) every 0.5 seconds

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Andreas Kling]

ImageQualityController (in RenderBoxModelObject.cpp) is broken for multi-layer backgrounds.
Since it only remembers the RenderBoxModelObject* and what IntSize the tile was, an element with multiple layers will get caught by the "animated resize" heuristic and get stuck in a loop where it's repainted in low quality every 0.5 seconds.

This can be observed with LayoutTests/fast/backgrounds/size/contain-and-cover.html

[Andreas Kling]

Created attachment 61694 [details]
Proposed patch

Skip the shouldPaintAtLowQuality() check for layers with siblings.

[WebKit Review Bot]

Attachment 61694 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 61696 [details]
Proposed patch

[Simon Fraser (smfr)]

Why aren't the authors of ImageQualityController copied on this bug?

[Andreas Kling]

(In reply to comment #4)
> Why aren't the authors of ImageQualityController copied on this bug?

Sorry about that, I was leaving in a hurry last night. Thank you for adding :-)

[Stephen White]

Looks good to me (though I'm not a reviewer).  I checked this against the  benchmarks my the original patch, and there were no regressions.

[Mark Rowe (bdash)]

<rdar://problem/8202530>

[Simon Fraser (smfr)]

Does this affect some live sites? Can you post a testcase?

[Andreas Kling]

(In reply to comment #8)
> Does this affect some live sites? Can you post a testcase?

I have not seen this on any live sites, but it's not unlikely. I stumbled upon this problem while working on bug 41201.

Like I said, you can observe this with the in-tree layout test LayoutTests/fast/backgrounds/size/contain-and-cover.html. If you resize that page, only the first <div> will be repainted in high quality after 500ms. The rest are repainted in low quality. This repeats every 500ms.

[Darin Adler]

Comment on attachment 61696 [details]
Proposed patch

This seems more like a workaround than a fix. Just turns off the logic for layers that have a next pointer, with no comment or indication in the code why, and no regression test. We can do better.

> -            bool useLowQualityScaling = shouldPaintAtLowQuality(context, image, tileSize);
> +            bool useLowQualityScaling;
> +            if (bgLayer->next())
> +                useLowQualityScaling = false;
> +            else
> +                useLowQualityScaling = shouldPaintAtLowQuality(context, image, tileSize);

I think the logic would be easier to read like this:

    bool useLowQualityScaling = !bgLayer->next() && shouldPaintAtLowQuality(context, image, tileSize);

But still, we need an explanation for why we should paint multiple layer backgrounds always in high quality.

[Andreas Kling]

(In reply to comment #10)
> (From update of attachment 61696 [details])
> This seems more like a workaround than a fix. Just turns off the logic for layers that have a next pointer, with no comment or indication in the code why, and no regression test. We can do better.

You are completely right.

@Stephen: You hacked a bunch on ImageQualityController - could you have a closer look at this?

[Simon Fraser (smfr)]

Stephen: ping!

[Stephen White]

(In reply to comment #12)

Sorry; was on vacation.  Taking a look now.

[Stephen White]

I can't repro this on r67642 using Safari, nor Chrome or Chrome's test_shell.  Andreas, is there some other way I can reproduce this?

The code still looks ok to me, but I don't know much about multi-image backgrounds, to be honest.  I did see some similar problems in earlier versions of ImageQualityController, when each image had its own timer (when two images were overlapping, one image's expiring timer would trigger another image's timer to start, resulting in toggling qualities), but when the timers were unified, those problems went away.

[Andreas Kling]

(In reply to comment #14)
> I can't repro this on r67642 using Safari, nor Chrome or Chrome's test_shell.  Andreas, is there some other way I can reproduce this?

Just repro'd on ToT Qt. Are you sure that you can't reproduce? Perhaps it's just more apparent on Qt since the difference between low and high quality scaling is very noticeable with QPainter.

[Stephen White]

(In reply to comment #15)
> (In reply to comment #14)
> > I can't repro this on r67642 using Safari, nor Chrome or Chrome's test_shell.  Andreas, is there some other way I can reproduce this?
> 
> Just repro'd on ToT Qt. Are you sure that you can't reproduce? Perhaps it's just more apparent on Qt since the difference between low and high quality scaling is very noticeable with QPainter.

Hmm, I still can't see the difference visually, but setting some breakpoints reveals that the timer is definitely firing repeatedly.  (Chrome has some further criteria for drawing high quality in platform/graphics/skia which may be obscuring the result, although it's usually pretty easy to see in Safari).

It definitely looks like there are some ping-pong effects going on here.  I don't know how much time I'll have to look into this just now, so if you want to go ahead and commit your patch in the interim (with the fixes Darin suggested), I'm ok with that.  Perhaps leave a comment suggesting that this is only an interim fix until we track down why it's ping-ponging in this case.

[Simon Fraser (smfr)]

I don't think the patch makes any sense, so I don't think we should commit it.

[Balazs Kelemen]

I have created a somewhat simpler test case that triggers the bug: http://gist.github.com/632183. The interesting thing is that I could not trigger it with only one box that has two images but just when I added another. Maybe the root of the problem is that we have only one global timer for all box objects.

[Balazs Kelemen]

D'oh, I pushed it with one div, so you should add the other to trigger the bug.

[Balazs Kelemen]

Created attachment 71295 [details]
Patch

[Balazs Kelemen]

Unfortunately I could not create a test case for this. I have tried it on
Mac and Windows but it seems like only Qt has the visual difference.
Inside the debugger I have seen that the repainting has happened in every 0.5 sec
on those platforms as well.

[Simon Fraser (smfr)]

Won't this patch still cause low-quality painting if the same image is used several times in a CSS background?

[Simon Fraser (smfr)]

Created attachment 71320 [details]
Testcase

[Simon Fraser (smfr)]

Comment on attachment 71295 [details]
Patch

Patch does not fix the attached testcase.

[Simon Fraser (smfr)]

To get this right I think you really need to know what it is that's drawing the image. You don't care if the same image is being drawn for different reasons at different sizes, you care about whether the image is being resized for this one particular use.

[Stephen White]

The problem seems to be that, since background-image allows more than one URL per element, neither the Image nor the RenderBoxModelObject (element) alone are enough to uniquely identify the object being drawn.

I've put together a patch that replaces the RenderBoxModelObject* key with an {Object, Image} pair, which seems to fix the problem.  Let me know what you guys think.

[Stephen White]

Created attachment 73768 [details]
Patch

[Simon Fraser (smfr)]

Doesn't that still have issues if the same image is used multiple times in background-image?

[WebKit Review Bot]

Attachment 73768 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5727029

[Eric Seidel (no email)]

Attachment 73768 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5724030

[Eric Seidel (no email)]

Attachment 73768 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5757028

[Stephen White]

(In reply to comment #28)
> Doesn't that still have issues if the same image is used multiple times in background-image?

You're right.  That won't work for your test case above.

I'm trying another version which includes the bgLayer pointer as part of the key.    This should basically represent the "role" of the image in the background-image.  Ideally, it should just be an index (int), but I couldn't find an easy way to do that, so I've made it a void*, to discourage ImageQualityController from dereferencing it (it should only be used for hashing).

[Stephen White]

Created attachment 73776 [details]
Patch

[Stephen White]

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

For your consideration.  ;)

(see above)

[Stephen White]

Landed as r72282.  Closing bug (manually, since webkit-patch got confused).

[Pavel Feldman]

Needed to roll devtools' part out. Were you landing this manually?

        M       WebKit/chromium/ChangeLog
        M       WebKit/chromium/src/js/devTools.css
Committed r72511

[Stephen White]

(In reply to comment #36)
> Needed to roll devtools' part out. Were you landing this manually?

Sorry, don't know how that snuck in there.  (Yes, I landed this manually.)