Bug 74183 – [Chromium] With the skia port, setting LCD text filtering is causing texture cache invalidations of gpu canvas backing store

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Justin Novosad]

Function isCanvasMultiLayered in SkiaFontWin.cpp creates
a layer iterator for the sole purpose of verifying the existence of multiple
layers. The iterator assumes that it is used for drawing, and therefore makes a
call to SkDevice::accessBitmap, which triggers a pixels changed notification, resulting in a GPU texture invalidation in Skia's caching system.  The unnecessary invalidations are preventing the cache form working properly.

[Justin Novosad]

The main use case where this causes trouble is with canvas to canvas draws from a software canvas to a GPU accelerated canvas.  The backing store of the source canvas gets cached on the GPU.

[Jin Yang]

Bug 73186 is also related to the generation id management.

[Justin Novosad]

Created attachment 135840 [details]
Patch

[Justin Novosad]

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

In this patch I removed all unnecessary uses of LayerIter that could be replaced with SkCanvas::isDrawingToLayer, not just the one in SkiaFontWin.cpp

[Dana Jansens]

Comment on attachment 135840 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:143
> +    } else if (m_createdTransparencyLayer || m_platformContext->canvas()->isDrawingToLayer()) {

None of these changes are functionally the same, however. The old code checked if there was > 1 layer present. The new code checks if there is > 0 layers present.

[Justin Novosad]

(In reply to comment #5)
> (From update of attachment 135840 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135840&action=review
> 
> > Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:143
> > +    } else if (m_createdTransparencyLayer || m_platformContext->canvas()->isDrawingToLayer()) {
> 
> None of these changes are functionally the same, however. The old code checked if there was > 1 layer present. The new code checks if there is > 0 layers present.

Actually, behavior is the same. It is a little confusing, but the fLayerCount in SkCanvas represents the number of saved layers, which does not include the root layer.

[Justin Novosad]

(In reply to comment #6)
 
> Actually, behavior is the same. It is a little confusing, but the fLayerCount in SkCanvas represents the number of saved layers, which does not include the root layer.

There is clearly a readability issue here. I think I will rename fLayerCount to fSavedLayerCount, to make things clearer in SkCanvas.

[Dana Jansens]

Comment on attachment 135840 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp:143
>>> +    } else if (m_createdTransparencyLayer || m_platformContext->canvas()->isDrawingToLayer()) {
>> 
>> None of these changes are functionally the same, however. The old code checked if there was > 1 layer present. The new code checks if there is > 0 layers present.
> 
> Actually, behavior is the same. It is a little confusing, but the fLayerCount in SkCanvas represents the number of saved layers, which does not include the root layer.

I see, thanks for the clarification. Making that more clear in Skia would be great :)

[Stephen White]

Comment on attachment 135840 [details]
Patch

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

> Source/WebKit/chromium/src/WebFontImpl.cpp:116
> +    if (canvasIsOpaque && SkColorGetA(color) == 0xFF && !const_cast<SkCanvas*>(canvas)->isDrawingToLayer()) {

Not your fault, but is this const_cast necessary anymore?  isDrawingToLayer() is const.

[Justin Novosad]

Created attachment 135858 [details]
Patch

[Stephen White]

Comment on attachment 135858 [details]
Patch

Looks good!  r=me

[WebKit Review Bot]

Comment on attachment 135858 [details]
Patch

Clearing flags on attachment: 135858

Committed r113345: <http://trac.webkit.org/changeset/113345>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.