Bug 83608 – [chromium] Refactor opaque content transform out of Skia context

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Refactor opaque content transform out of Skia context

[Adrienne Walker]

This may look like a mostly irrelevant refactoring, but it will make adding additional tracking for text regions in the platform context simpler, as everything can be done in device space and transformed later.

I think this also is a better layering separation, where callers of the context can transform into the space they want things to be in.  I think it's also better performing in that it transforms one rect vs. transforming every rect on every Skia draw command.

[Adrienne Walker]

Created attachment 136519 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136519 [details]
Patch

Attachment 136519 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12382577

New failing tests:
compositing/scrollbar-painting.html
compositing/overflow/clip-content-under-overflow-controls.html

[WebKit Review Bot]

Created attachment 136601 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dana Jansens]

Comment on attachment 136519 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:95
> +    IntRect opaqueRect;

how would you feel about calling this resultingOpaqueRect also?

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:49
> +void CanvasLayerTextureUpdater::paintContents(GraphicsContext& context, PlatformContextSkia& platformContext, const IntRect& contentRect, float contentsScale, IntRect* resultingOpaqueRect)

If it's not optional it should be a IntRect& shouldn't it?

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:72
> +        if (transform.preservesAxisAlignment()) {

Why do you check this? If anything it seems like it would make a good assert?

The transform should only be the translate/scale set in this function.

> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.cpp:63
> +    IntRect opaqueRect;

resultingOpaqueRect also maybe?

> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:-256
> -    // Apply the transform to device coordinate space.
> -    SkMatrix canvasTransform = context->canvas()->getTotalMatrix();
> -    if (!canvasTransform.mapRect(&targetRect))
> -        fillsBounds = false;
> -

This needs to stay. This is moving things to the SkCanvas's device space. We track all paints in this space. The transform to layer space is just the one below that you removed. Do the PlatformContextSkiaTest unit tests pass with this change..?

> Source/WebKit/chromium/tests/LayerTextureUpdaterTest.cpp:262
> +    IntRect scaledRect(5, 10, 24, 37);

I'm confused by this.. the rect moves left/up but is half the size?

[Adrienne Walker]

Comment on attachment 136519 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:95
>> +    IntRect opaqueRect;
> 
> how would you feel about calling this resultingOpaqueRect also?

Wouldn't that conflict with the existing resultingOpaqueRect parameter?

>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:72
>> +        if (transform.preservesAxisAlignment()) {
> 
> Why do you check this? If anything it seems like it would make a good assert?
> 
> The transform should only be the translate/scale set in this function.

I was mimicking the removal of the code on OpaqueRegionSkia.cpp:294.

>> Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.cpp:63
>> +    IntRect opaqueRect;
> 
> resultingOpaqueRect also maybe?

Same issue? I could make resultingOpaqueRect not optional, if that would make things clearer.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:-256
>> -
> 
> This needs to stay. This is moving things to the SkCanvas's device space. We track all paints in this space. The transform to layer space is just the one below that you removed. Do the PlatformContextSkiaTest unit tests pass with this change..?

Oops! Nice catch.  I think I was being overzealous after removing the one on line 294.

>> Source/WebKit/chromium/tests/LayerTextureUpdaterTest.cpp:262
>> +    IntRect scaledRect(5, 10, 24, 37);
> 
> I'm confused by this.. the rect moves left/up but is half the size?

Ack! Now that I look at this, it's totally wrong.  If I draw a canvas has a given opaque rect, then the resulting final opaque rect is content scale * canvas opaque rect, and not canvas opaque rect / content scale.

[Adrienne Walker]

Created attachment 136776 [details]
Fix tests

[Adrienne Walker]

That accidentally removed code caused all the problems, both with the weird unit test and the layout test failures.  I totally misunderstood how content scale worked previously, but I'm all sorted now.

[Dana Jansens]

Comment on attachment 136519 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:95
>>> +    IntRect opaqueRect;
>> 
>> how would you feel about calling this resultingOpaqueRect also?
> 
> Wouldn't that conflict with the existing resultingOpaqueRect parameter?

Could you just pass that one in, instead of making a local?

>>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:72
>>> +        if (transform.preservesAxisAlignment()) {
>> 
>> Why do you check this? If anything it seems like it would make a good assert?
>> 
>> The transform should only be the translate/scale set in this function.
> 
> I was mimicking the removal of the code on OpaqueRegionSkia.cpp:294.

Ah right. That code didn't really know what the transform was and treated it opaquely. I think we should just assert here. Previously we didn't assert on axis-alignment, which would be good to check.

[Dana Jansens]

Comment on attachment 136776 [details]
Fix tests

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

> Source/WebCore/ChangeLog:7
> +

Some prose/poetry to explain why?

[Adrienne Walker]

Created attachment 136960 [details]
Poetry, use resultingOpaqueRect

[Dana Jansens]

Comment on attachment 136960 [details]
Poetry, use resultingOpaqueRect

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

Thanks for the changes, tiny nit below. Otherwise LGTM !

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:67
>          // Transform tracked opaque paints back to our layer's content space.
>          ASSERT(context.getCTM().isInvertible());

last nits: put these lines down with the new line 72 ASSERT. And check this assert on "transform" directly? Then maybe a comment/renaming to make it clear its intentional that "transform" is saved before painting.

[Adrienne Walker]

Created attachment 136962 [details]
Address nits

[WebKit Review Bot]

Comment on attachment 136962 [details]
Address nits

Attachment 136962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12395439

New failing tests:
svg/text/font-size-below-point-five.svg

[WebKit Review Bot]

Created attachment 136972 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adrienne Walker]

(In reply to comment #15)
> Created an attachment (id=136972) [details]
> Archive of layout-test-results from ec2-cr-linux-04
> 
> The attached test failures were seen while running run-webkit-tests on the chromium-ews.
> Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

This is almost certainly flake, since this test doesn't involve compositing.

[Adrienne Walker]

senorblanco, jamesr: ping for review?

[James Robinson]

Comment on attachment 136962 [details]
Address nits

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

Looks great.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:49
> +void CanvasLayerTextureUpdater::paintContents(GraphicsContext& context, PlatformContextSkia& platformContext, const IntRect& contentRect, float contentsScale, IntRect* resultingOpaqueRect)

The parameter probably should be IntRect& if the implementation isn't going to null check it - webkit seems to prefer a non-const reference for non-optional out params (unlike chromium style)

[Adrienne Walker]

Created attachment 138191 [details]
Patch for landing

[Adrienne Walker]

Comment on attachment 138191 [details]
Patch for landing

I changed all the parameters for prepareToUpdate to be references instead of pointers.

[Dana Jansens]

(In reply to comment #20)
> (From update of attachment 138191 [details])
> I changed all the parameters for prepareToUpdate to be references instead of pointers.

Yay, sorry for doing that wrong in the first place.

[WebKit Review Bot]

Comment on attachment 138191 [details]
Patch for landing

Clearing flags on attachment: 138191

Committed r114827: <http://trac.webkit.org/changeset/114827>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.