Bug 108641 – [TexMap] Backing store paints all tiles

• Component: Layout and Rendering
• Status: RESOLVED WONTFIX

[Allan Sandfeld Jensen]

The backing issues paint commands to all tiles regardless of clipping. When using the TextureMapperImageBuffer, this may cause all the tiles to be processed and internally painted before they are clipped when painting to graphics context.

We should either avoid requesting paint of fully clipped tiles, or exit early when painting a fully clipped tile.

[Allan Sandfeld Jensen]

Created attachment 186056 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 186056 [details]
Patch

Nice find, LGTM.

[Noam Rosenthal]

Comment on attachment 186056 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:144
> +    GraphicsContext* context = textureMapper->graphicsContext();
> +    if (!context)
> +        return;

I don't like relying on textureMapper->graphicsContext() here. It's not assured that we're going to have that.
Instead, TextureMapper should have a clipBounds function and abstract it away internally; E.g. TextureMapperGL has its own clip bounds based on the clipStack.

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (From update of attachment 186056 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186056&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:144
> > +    GraphicsContext* context = textureMapper->graphicsContext();
> > +    if (!context)
> > +        return;
> 
> I don't like relying on textureMapper->graphicsContext() here. It's not assured that we're going to have that.
> Instead, TextureMapper should have a clipBounds function and abstract it away internally; E.g. TextureMapperGL has its own clip bounds based on the clipStack.

When can that happen, painting to a surface? 

Btw, is this improvement even that helpfull to TextureMapperGL? Otherwise I can move the check to TextureMapperImageBuffer::drawTexture.

[Noam Rosenthal]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 186056 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=186056&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:144
> > > +    GraphicsContext* context = textureMapper->graphicsContext();
> > > +    if (!context)
> > > +        return;
> > 
> > I don't like relying on textureMapper->graphicsContext() here. It's not assured that we're going to have that.
> > Instead, TextureMapper should have a clipBounds function and abstract it away internally; E.g. TextureMapperGL has its own clip bounds based on the clipStack.
> 
> When can that happen, painting to a surface? 
> 
> Btw, is this improvement even that helpfull to TextureMapperGL? Otherwise I can move the check to TextureMapperImageBuffer::drawTexture.

But we have that check already inside QPainter... does this actually improve anything? Same with GL when we have a scissor.

[Noam Rosenthal]

> > 
> > Btw, is this improvement even that helpfull to TextureMapperGL? Otherwise I can move the check to TextureMapperImageBuffer::drawTexture.
> 
> But we have that check already inside QPainter... does this actually improve anything? Same with GL when we have a scissor.

More to the point, doing an inverse transform and clip tests here is not necessarily less expensive than going through the motions until we get to the raster/GL engine.

[Allan Sandfeld Jensen]

(In reply to comment #5)
> But we have that check already inside QPainter... does this actually improve anything? Same with GL when we have a scissor.

For TextureMapperImageBuffer it does because MaskTexture might be applied in TextureMapperImageBuffer::drawTexture,  the image might be copied in ImageBuffer::draw, and a shadow applied in StillImage::draw. On top of that I haven't checked how fast QPainter exits on clips. 

Also I prefer doing this check early so QPainter doesn't have to do it for every tile individually. I guess even for GL it would be great to limit the number of GL commands send, even if the driver or hardware is smart enough to ignore most of them.

[Allan Sandfeld Jensen]

This is unimportant at this time. The painting is already clipped later, and any performance with better tiling is better done in Coordinated Graphics.