Bug 105051 – [chromium] Enable LCD AA if text is painted inside opaque region

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Layout and Rendering

[Alok Priyadarshi]

We already mark layers whether they can use LCD text based on their transform and opacity. Now actually paint LCD text if it lies inside opaque region.

[Alok Priyadarshi]

Created attachment 179517 [details]
Patch

[Alok Priyadarshi]

Uploading for early feedback. I still need to add unit-test for OpaqueRegionSkia::isOpaqueInRect().

This version of patch always uses NULL rect for text. So it only enables LCD on layers that have an opaque background (including root layer).

[Dana Jansens]

Comment on attachment 179517 [details]
Patch

OpaqueRegionSkia changes look good. It would be nice to use/verify isOpaqueInRect() in the PlatformContextSkia tests.

[James Robinson]

Comment on attachment 179517 [details]
Patch

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

Won't this produce inconsistently rendered text within a layer depending on how the paint rect interacts with the opaque region?

This patch doesn't appear to actually populate the rect at all, so I can't tell where it is going to come from in general.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:449
> +bool PlatformContextSkia::adjustPaintFlagsForText(SkPaint* paint, const SkRect* rect) const

I can't find any callers that pass in a non-null rect parameter - where are they?

[James Robinson]

(In reply to comment #2)
> Uploading for early feedback. I still need to add unit-test for OpaqueRegionSkia::isOpaqueInRect().
> 
> This version of patch always uses NULL rect for text. So it only enables LCD on layers that have an opaque background (including root layer).

Ah, I see.  So this doesn't actually do rect-region comparisons.

When it does, where do you anticipate the rect being compared to the opaque region will come from?

[Alok Priyadarshi]

Comment on attachment 179517 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:449
>> +bool PlatformContextSkia::adjustPaintFlagsForText(SkPaint* paint, const SkRect* rect) const
> 
> I can't find any callers that pass in a non-null rect parameter - where are they?

They will come from estimating text bounds using something like this:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/FontChromiumWin.cpp#L240

I chose to not compute bounds in this patch because text drawing code scattered in platform-specific files. I will add them for each platform in separate patches. But if you prefer I can get rid of the rect parameter for now. I could just always check device-clip-rect.

[Alok Priyadarshi]

(In reply to comment #4)
> (From update of attachment 179517 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179517&action=review
> 
> Won't this produce inconsistently rendered text within a layer depending on how the paint rect interacts with the opaque region?
> 

Yes this is a possibility. There is no way to un-paint LCD text in immediate mode. We may be able to avoid it with impl-side painting where we can defer the decision until after recording the SkPicture.

[Alok Priyadarshi]

I have decided to abandon this patch and move the refactoring changes to two separate patches here:
https://bugs.webkit.org/show_bug.cgi?id=106266
https://bugs.webkit.org/show_bug.cgi?id=106267