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.
Created attachment 186056 [details] Patch
Comment on attachment 186056 [details] Patch Nice find, LGTM.
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.
(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.
(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.
> > > > 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.
(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.
This is unimportant at this time. The painting is already clipped later, and any performance with better tiling is better done in Coordinated Graphics.