WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
108641
[TexMap] Backing store paints all tiles
https://bugs.webkit.org/show_bug.cgi?id=108641
Summary
[TexMap] Backing store paints all tiles
Allan Sandfeld Jensen
Reported
2013-02-01 08:29:05 PST
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.
Attachments
Patch
(2.56 KB, patch)
2013-02-01 08:33 PST
,
Allan Sandfeld Jensen
noam
: review-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Allan Sandfeld Jensen
Comment 1
2013-02-01 08:33:13 PST
Created
attachment 186056
[details]
Patch
Kenneth Rohde Christiansen
Comment 2
2013-02-01 08:34:24 PST
Comment on
attachment 186056
[details]
Patch Nice find, LGTM.
Noam Rosenthal
Comment 3
2013-02-01 08:39:08 PST
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
Comment 4
2013-02-01 09:30:29 PST
(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
Comment 5
2013-02-01 09:32:47 PST
(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
Comment 6
2013-02-01 09:33:44 PST
> > > > 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
Comment 7
2013-02-01 10:32:35 PST
(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
Comment 8
2013-02-12 06:58:23 PST
This is unimportant at this time. The painting is already clipped later, and any performance with better tiling is better done in Coordinated Graphics.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug