Adding occlusion detection to reduce overdraw in RenderBox background rendering

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Novosad]

Adding occlusion detection to reduce overdraw in RenderBox background rendering

[Justin Novosad]

Created attachment 174758 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174758 [details]
Patch

Attachment 174758 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14861375

New failing tests:
fast/dom/shadow/shadow-dom-event-dispatching.html

[Brent Fulgham]

Comment on attachment 174758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174758&action=review

Looks likea great change, but I was curious about the static cast switch rather than a simple virtual method. They all appear to be children of a common superclass.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:200
> +{

In the ChangeLog could you indicate why this switch statement is preferable to a virtual method on CSSImageGeneratorValue? Seems like a lot of casting and specialized knowledge about which subclasses need this behavior. Was this found to be a bottleneck?

[Justin Novosad]

(In reply to comment #3)
> (From update of attachment 174758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174758&action=review
> 
> Looks likea great change, but I was curious about the static cast switch rather than a simple virtual method. They all appear to be children of a common superclass.

Actually, I was curious about that too when I wrote it.  I just followed the pattern that was already there.  I plead guilty to cargo cult programming.

I assume there is some sort of requirement for CSS*Value classes to be POD?  I should go satisfy my own curiosity, and come back...

FWIW, before landing this I will perform some experiments just to make sure we are getting full use case coverage (all the hasAlpha methods returning true and false).  I will add additional tests to the patch if needed.

[Justin Novosad]

(In reply to comment #3)
> 
> In the ChangeLog could you indicate why this switch statement is preferable to a virtual method on CSSImageGeneratorValue? Seems like a lot of casting and specialized knowledge about which subclasses need this behavior. Was this found to be a bottleneck?

The reason is already documented in CSSValue.h:
// NOTE: This class is non-virtual for memory and performance reasons.
// Don't go making it virtual again unless you know exactly what you're doing!

[Justin Novosad]

Created attachment 175503 [details]
Patch

[Justin Novosad]

Latest patch fixes issues I found while performing more in-depth testing.  I found that hasAlpha was returning true for opaque bitmap images that had not yet been pulled into cache.  To fix this, I had to add a RanderObject* argument to hasAlpha so that if the style contains a BitmapImage, the bitmap can be resolved for the current renderer.

The new patch also adds a layout test that covers a series of use cases that are optimization candidates.  The current patch successfully optimizes 6 of the 10 cases.  A regular test run does not verify that occlusion culling kicked in. To determine whether an optimization is successful, the test has to be run manually with a debugger attached.

[Justin Novosad]

Created attachment 175520 [details]
Patch

[Justin Novosad]

New patch adds test expectations for missing baselines

[Build Bot]

Comment on attachment 175520 [details]
Patch

Attachment 175520 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14961191

New failing tests:
fast/backgrounds/background-opaque-images-over-color.html

[Justin Novosad]

Created attachment 175684 [details]
Patch

[Justin Novosad]

New patch adds text baseline and fixes bug where "space" repeat mode was falsely considered opaque (bug caught by existing test).

[Stephen White]

Comment on attachment 175520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175520&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:478
> +bool CachedImage::hasAlpha(const RenderObject* renderer)

Maybe we should rename this to currentFrameHasAlpha() as well, just to be clear to callers.

[Stephen White]

Comment on attachment 175520 [details]
Patch

Fix the above on landing, and I'm good with it.  r=me

[Stephen White]

Comment on attachment 175684 [details]
Patch

r+'ing the right patch this time.

[Justin Novosad]

Created attachment 175695 [details]
Patch for landing

[Stephen White]

Comment on attachment 175695 [details]
Patch for landing

According to Noel Gordon, the change to BitmapImage will break deferred image decoding.  See http://trac.webkit.org/changeset/125154

I'm going to cq- this patch for now until we can sort this out.

[Justin Novosad]

Created attachment 175805 [details]
Patch

[Justin Novosad]

Triggering the decode for the purpose of occlusion culling while rendering the RenderBox is perfectly fine because the decoded image will be required imminently anyways in order to draw it. New patch reverts change to BimapImage, and moves the decode triggering into CachedImage.  Since this is a new method, it won't affect previously existing code paths that check for the presence of alpha.

[Stephen White]

Comment on attachment 175805 [details]
Patch

Great.  This is the solution I was thinking of as well.  r=me

[Build Bot]

Comment on attachment 175805 [details]
Patch

Attachment 175805 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14966761

New failing tests:
fast/backgrounds/background-opaque-images-over-color.html

[Justin Novosad]

Created attachment 175821 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 175821 [details]
Patch for landing

Clearing flags on attachment: 175821

Committed r135629: <http://trac.webkit.org/changeset/135629>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

Comment on attachment 175821 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=175821&action=review

I wish I'd had a chance to review this before it was landed.

> Source/WebCore/loader/cache/CachedImage.cpp:482
> +    if (image->isBitmapImage())
> +        image->nativeImageForCurrentFrame(); // force decode

Seem unfortunate to force a decode here. Would that decode have always happened anyway, or would it be better to just say that it has alpha?

> Source/WebCore/loader/cache/CachedImage.h:55
> +    bool currentFrameHasAlpha(const RenderObject*); // Side effect: ensures decoded image is in cache, therefore should only be called when about to draw the image.

Why is this not const?

> Source/WebCore/rendering/RenderBox.cpp:1007
> +    // RenderBoxModelOBject::paintFillLayerExtended. A more efficient solution might be to move

Typo: OBject

> Source/WebCore/rendering/RenderBox.cpp:1012
> +    if (fillLayer->next() && (!fillLayer->hasOpaqueImage(this) || !fillLayer->image()->canRender(this, style()->effectiveZoom()) || !fillLayer->hasRepeatXY()))
> +        paintFillLayers(paintInfo, c, fillLayer->next(), rect, bleedAvoidance, op, backgroundObject);
>      paintFillLayer(paintInfo, c, fillLayer, rect, bleedAvoidance, op, backgroundObject);

I don't see how this correctly handles background-clip (which means the image might not fill the entire border box).

> Source/WebCore/rendering/RenderBoxModelObject.cpp:913
> +    if (!bgLayer->next() && !(shouldPaintBackgroundImage && bgLayer->hasOpaqueImage(this) && bgLayer->hasRepeatXY())) {

Same here.

> LayoutTests/ChangeLog:13
> +        * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added.
> +        * fast/backgrounds/background-opaque-images-over-color.html: Added.
> +        * platform/chromium/TestExpectations:

Why was this not a ref tests? Without pixel results, the test is useless.

[Justin Novosad]

(In reply to comment #25)
> (From update of attachment 175821 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175821&action=review
> 
> I wish I'd had a chance to review this before it was landed.

Noted, and don't worry I'll post a follow-up patch to address your concerns early next week.

> 
> > Source/WebCore/loader/cache/CachedImage.cpp:482
> > +    if (image->isBitmapImage())
> > +        image->nativeImageForCurrentFrame(); // force decode
> 
> Seem unfortunate to force a decode here. Would that decode have always happened anyway, or would it be better to just say that it has alpha?

Yes, a decode will happen anyways because this method is only ever called immediately before drawing the image.  I documented this side-effect in the header in case anyone else is tempted to use this method for other purposes.
> 
> 
> > Source/WebCore/rendering/RenderBox.cpp:1012
> > +    if (fillLayer->next() && (!fillLayer->hasOpaqueImage(this) || !fillLayer->image()->canRender(this, style()->effectiveZoom()) || !fillLayer->hasRepeatXY()))
> > +        paintFillLayers(paintInfo, c, fillLayer->next(), rect, bleedAvoidance, op, backgroundObject);
> >      paintFillLayer(paintInfo, c, fillLayer, rect, bleedAvoidance, op, backgroundObject);
> 
> I don't see how this correctly handles background-clip (which means the image might not fill the entire border box).

OK, I'm surprised no existing test picked-up on that 
I'll come-up with a layout test and a fix for that ASAP.

> 
> > LayoutTests/ChangeLog:13
> > +        * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added.
> > +        * fast/backgrounds/background-opaque-images-over-color.html: Added.
> > +        * platform/chromium/TestExpectations:
> 
> Why was this not a ref tests? Without pixel results, the test is useless.

It is, I just submitted it without the pixel baselines.  I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic

[Simon Fraser (smfr)]

(In reply to comment #26)
> > > LayoutTests/ChangeLog:13
> > > +        * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added.
> > > +        * fast/backgrounds/background-opaque-images-over-color.html: Added.
> > > +        * platform/chromium/TestExpectations:
> > 
> > Why was this not a ref tests? Without pixel results, the test is useless.
> 
> It is, I just submitted it without the pixel baselines.  I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic

A ref tests would have a -expected.html, and no .txt file.

[Zan Dobersek]

(In reply to comment #27)
> (In reply to comment #26)
> > > > LayoutTests/ChangeLog:13
> > > > +        * fast/backgrounds/background-opaque-images-over-color-expected.txt: Added.
> > > > +        * fast/backgrounds/background-opaque-images-over-color.html: Added.
> > > > +        * platform/chromium/TestExpectations:
> > > 
> > > Why was this not a ref tests? Without pixel results, the test is useless.
> > 
> > It is, I just submitted it without the pixel baselines.  I'll pick-up and submit the baselines from the bots with Tools/Script/webkit-patch garden-o-matic
> 
> A ref tests would have a -expected.html, and no .txt file.

Please make this a reftest unless it's absolutely necessary to test the generated render tree. It's easier for everyone to maintain a reference output of a 200x100px green rectangle than to potentially have many ports generate unneeded pixel baselines.

OTOH, the test is producing a text failure on every port (except mac where the test is skipped).
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=fast%2Fbackgrounds%2Fbackground-opaque-images-over-color.html

[János Badics]

Unfortunately, new fast/backgrounds/background-opaque-images-over-color.html started to fail on Qt. I have created a bug for the issue at

https://bugs.webkit.org/show_bug.cgi?id=103227

I will skip this test on Qt until it's fixed, if you don't mind.

[János Badics]

The test has been skipped on Qt in
http://trac.webkit.org/changeset/135698

Please unskip it with the proper fix

(see the related bug at
https://bugs.webkit.org/show_bug.cgi?id=103227 )

[Simon Fraser (smfr)]

I think we should revert this change if we don't get a fix soon.

[Justin Novosad]

Follow-ups:

Ref test:
https://bugs.webkit.org/show_bug.cgi?id=103275


Clip issue:
https://bugs.webkit.org/show_bug.cgi?id=103276