Bug 83608 - [chromium] Refactor opaque content transform out of Skia context
Summary: [chromium] Refactor opaque content transform out of Skia context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks: 83609 84275
  Show dependency treegraph
 
Reported: 2012-04-10 12:42 PDT by Adrienne Walker
Modified: 2012-04-20 18:49 PDT (History)
7 users (show)

See Also:


Attachments
Patch (25.85 KB, patch)
2012-04-10 12:52 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.30 MB, application/zip)
2012-04-10 18:39 PDT, WebKit Review Bot
no flags Details
Fix tests (25.59 KB, patch)
2012-04-11 16:08 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Poetry, use resultingOpaqueRect (25.93 KB, patch)
2012-04-12 13:31 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Address nits (26.29 KB, patch)
2012-04-12 13:48 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.26 MB, application/zip)
2012-04-12 14:23 PDT, WebKit Review Bot
no flags Details
Patch for landing (42.56 KB, patch)
2012-04-20 16:30 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-04-10 12:42:44 PDT
[chromium] Refactor opaque content transform out of Skia context
Comment 1 Adrienne Walker 2012-04-10 12:51:17 PDT
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.
Comment 2 Adrienne Walker 2012-04-10 12:52:49 PDT
Created attachment 136519 [details]
Patch
Comment 3 WebKit Review Bot 2012-04-10 18:39:21 PDT
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
Comment 4 WebKit Review Bot 2012-04-10 18:39:26 PDT
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
Comment 5 Dana Jansens 2012-04-11 10:00:46 PDT
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?
Comment 6 Adrienne Walker 2012-04-11 12:55:51 PDT
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.
Comment 7 Adrienne Walker 2012-04-11 16:08:23 PDT
Created attachment 136776 [details]
Fix tests
Comment 8 Adrienne Walker 2012-04-11 16:10:10 PDT
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.
Comment 9 Dana Jansens 2012-04-11 17:06:54 PDT
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.
Comment 10 Dana Jansens 2012-04-11 17:12:05 PDT
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?
Comment 11 Adrienne Walker 2012-04-12 13:31:23 PDT
Created attachment 136960 [details]
Poetry, use resultingOpaqueRect
Comment 12 Dana Jansens 2012-04-12 13:37:42 PDT
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.
Comment 13 Adrienne Walker 2012-04-12 13:48:28 PDT
Created attachment 136962 [details]
Address nits
Comment 14 WebKit Review Bot 2012-04-12 14:23:45 PDT
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
Comment 15 WebKit Review Bot 2012-04-12 14:23:51 PDT
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
Comment 16 Adrienne Walker 2012-04-12 16:31:56 PDT
(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.
Comment 17 Adrienne Walker 2012-04-16 11:48:06 PDT
senorblanco, jamesr: ping for review?
Comment 18 James Robinson 2012-04-18 21:07:12 PDT
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)
Comment 19 Adrienne Walker 2012-04-20 16:30:24 PDT
Created attachment 138191 [details]
Patch for landing
Comment 20 Adrienne Walker 2012-04-20 16:31:32 PDT
Comment on attachment 138191 [details]
Patch for landing

I changed all the parameters for prepareToUpdate to be references instead of pointers.
Comment 21 Dana Jansens 2012-04-20 16:33:24 PDT
(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.
Comment 22 WebKit Review Bot 2012-04-20 18:48:46 PDT
Comment on attachment 138191 [details]
Patch for landing

Clearing flags on attachment: 138191

Committed r114827: <http://trac.webkit.org/changeset/114827>
Comment 23 WebKit Review Bot 2012-04-20 18:49:14 PDT
All reviewed patches have been landed.  Closing bug.