Bug 90092 - [chromium] CanvasLayerTextureUpdater needs to convert opaque rects back to content space.
Summary: [chromium] CanvasLayerTextureUpdater needs to convert opaque rects back to co...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: vollick
URL:
Keywords:
Depends on: 90278
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-27 12:36 PDT by vollick
Modified: 2012-06-30 08:56 PDT (History)
8 users (show)

See Also:


Attachments
Patch (15.56 KB, patch)
2012-06-27 12:43 PDT, vollick
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (640.33 KB, application/zip)
2012-06-27 17:09 PDT, WebKit Review Bot
no flags Details
Patch (15.67 KB, patch)
2012-06-28 10:11 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (13.70 KB, patch)
2012-06-28 11:52 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (12.81 KB, patch)
2012-06-28 19:18 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.40 KB, patch)
2012-06-28 20:30 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.40 KB, patch)
2012-06-29 08:26 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.33 KB, patch)
2012-06-29 11:37 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch for landing (32.40 KB, patch)
2012-06-29 12:29 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.24 KB, patch)
2012-06-29 19:30 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch for landing (32.11 KB, patch)
2012-06-29 19:33 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.19 KB, patch)
2012-06-30 06:35 PDT, vollick
no flags Details | Formatted Diff | Diff
Patch (32.32 KB, patch)
2012-06-30 07:48 PDT, vollick
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vollick 2012-06-27 12:36:56 PDT
Currently, the content layer delegate returns opaque rects in layer space, so we need to map them correctly into content space.
Comment 1 vollick 2012-06-27 12:43:11 PDT
Created attachment 149787 [details]
Patch
Comment 2 Adrienne Walker 2012-06-27 13:59:39 PDT
Comment on attachment 149787 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
> -    m_painter->paint(canvas, scaledContentRect, resultingOpaqueRect);
> +    m_painter->paint(canvas, scaledContentRect, contentsScale, resultingOpaqueRect);

As a sanity-preserving constraint, if it's possible, I'd like any paint(rect, &resultingOpaque) to return resultingOpaque in the same space as rect.

So, with that in mind, what about leaving the signature of LayerPainterChromium the same and just transforming the resultingOpaqueRect here from layer space back to content space? CanvasLayerTextureUpdater is the one that's introducing this extra scale, so it kind of makes sense to me that it should be the one to undo it before returning the result.

(Also, for clarity, can you maybe rename scaledContentRect and resultingOpaqueRect to include the name of the space they're in in the variable name, like layerRect and resultingOpaqueLayerRect?)

> Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegate.cpp:-59
> -    // Record transform prior to painting, as all opaque tracking will be
> -    // relative to this current value.
> -    AffineTransform canvasToContentTransform = context.getCTM().inverse();
> -

I don't follow these changes to OpaqueRectTrackingContentLayerDelegate? Are these tested?
Comment 3 Dana Jansens 2012-06-27 14:01:34 PDT
Comment on attachment 149787 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
>> +    m_painter->paint(canvas, scaledContentRect, contentsScale, resultingOpaqueRect);
> 
> As a sanity-preserving constraint, if it's possible, I'd like any paint(rect, &resultingOpaque) to return resultingOpaque in the same space as rect.
> 
> So, with that in mind, what about leaving the signature of LayerPainterChromium the same and just transforming the resultingOpaqueRect here from layer space back to content space? CanvasLayerTextureUpdater is the one that's introducing this extra scale, so it kind of makes sense to me that it should be the one to undo it before returning the result.
> 
> (Also, for clarity, can you maybe rename scaledContentRect and resultingOpaqueRect to include the name of the space they're in in the variable name, like layerRect and resultingOpaqueLayerRect?)

I had requested that the function return the scaled rect to avoid doing enclosingIntRect(), then scaling, then enclosingIntRect() again, making it all very lossy. The function could return a FloatRect for the resultingOpaqueRect instead though.
Comment 4 Adrienne Walker 2012-06-27 14:12:51 PDT
Comment on attachment 149787 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
>>> +    m_painter->paint(canvas, scaledContentRect, contentsScale, resultingOpaqueRect);
>> 
>> As a sanity-preserving constraint, if it's possible, I'd like any paint(rect, &resultingOpaque) to return resultingOpaque in the same space as rect.
>> 
>> So, with that in mind, what about leaving the signature of LayerPainterChromium the same and just transforming the resultingOpaqueRect here from layer space back to content space? CanvasLayerTextureUpdater is the one that's introducing this extra scale, so it kind of makes sense to me that it should be the one to undo it before returning the result.
>> 
>> (Also, for clarity, can you maybe rename scaledContentRect and resultingOpaqueRect to include the name of the space they're in in the variable name, like layerRect and resultingOpaqueLayerRect?)
> 
> I had requested that the function return the scaled rect to avoid doing enclosingIntRect(), then scaling, then enclosingIntRect() again, making it all very lossy. The function could return a FloatRect for the resultingOpaqueRect instead though.

PlatformContextSkia tracks opaqueness in layer space.  If we want that rect in content space, somebody needs to scale and call enclosedIntRect on it somewhere.  I'm suggesting that it gets done in CanvasLayerTextureUpdater instead of in ContentLayerPainter.  Where is the extra round-off coming from?
Comment 5 Dana Jansens 2012-06-27 14:22:31 PDT
Comment on attachment 149787 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
>>>> +    m_painter->paint(canvas, scaledContentRect, contentsScale, resultingOpaqueRect);
>>> 
>>> As a sanity-preserving constraint, if it's possible, I'd like any paint(rect, &resultingOpaque) to return resultingOpaque in the same space as rect.
>>> 
>>> So, with that in mind, what about leaving the signature of LayerPainterChromium the same and just transforming the resultingOpaqueRect here from layer space back to content space? CanvasLayerTextureUpdater is the one that's introducing this extra scale, so it kind of makes sense to me that it should be the one to undo it before returning the result.
>>> 
>>> (Also, for clarity, can you maybe rename scaledContentRect and resultingOpaqueRect to include the name of the space they're in in the variable name, like layerRect and resultingOpaqueLayerRect?)
>> 
>> I had requested that the function return the scaled rect to avoid doing enclosingIntRect(), then scaling, then enclosingIntRect() again, making it all very lossy. The function could return a FloatRect for the resultingOpaqueRect instead though.
> 
> PlatformContextSkia tracks opaqueness in layer space.  If we want that rect in content space, somebody needs to scale and call enclosedIntRect on it somewhere.  I'm suggesting that it gets done in CanvasLayerTextureUpdater instead of in ContentLayerPainter.  Where is the extra round-off coming from?

Oh okay, we're probably talking about the same things. This code has so many long/similar class names it's impossible to hold in my head.

OpaqueRectTrackingContentLayerDelegate did a enclosedIntRect(mapRect()). What I didn't want was for someone else to take that and map/enclose it again. As long as the transform from source->target is all done in one fell swoop I don't mind where :)
Comment 6 WebKit Review Bot 2012-06-27 17:09:23 PDT
Comment on attachment 149787 [details]
Patch

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

New failing tests:
compositing/culling/unscrolled-within-boxshadow.html
Comment 7 WebKit Review Bot 2012-06-27 17:09:33 PDT
Created attachment 149828 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 vollick 2012-06-28 10:11:37 PDT
Created attachment 149966 [details]
Patch

Fixed the layout test. We needed to grab the transform from the canvas _before_
painting (just like was being done in the OpaqueRectTrackingContentLayerDelegate
previously).

(In reply to comment #2)
> (From update of attachment 149787 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149787&action=review
>
> > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
> > -    m_painter->paint(canvas, scaledContentRect, resultingOpaqueRect);
> > +    m_painter->paint(canvas, scaledContentRect, contentsScale, resultingOpaqueRect);
>
> As a sanity-preserving constraint, if it's possible, I'd like any paint(rect, &resultingOpaque) to return resultingOpaque in the same space as rect.
sgtm.
>
> So, with that in mind, what about leaving the signature of LayerPainterChromium the same and just transforming the resultingOpaqueRect here from layer space back to content space? CanvasLayerTextureUpdater is the one that's introducing this extra scale, so it kind of makes sense to me that it should be the one to undo it before returning the result.
Done.
>
> (Also, for clarity, can you maybe rename scaledContentRect and resultingOpaqueRect to include the name of the space they're in in the variable name, like layerRect and resultingOpaqueLayerRect?)
Done.
>
> > Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegate.cpp:-59
> > -    // Record transform prior to painting, as all opaque tracking will be
> > -    // relative to this current value.
> > -    AffineTransform canvasToContentTransform = context.getCTM().inverse();
> > -
>
> I don't follow these changes to OpaqueRectTrackingContentLayerDelegate? Are these tested?
They weren't -- I'd missed this -- but the unit test now uses the ORTCLD.
Comment 9 James Robinson 2012-06-28 10:30:43 PDT
Comment on attachment 149966 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
> +    PlatformContextSkia platformContext(canvas);
> +    GraphicsContext context(&platformContext);

Sorry this is a layering no-no.  CanvasLayerTextureUpdater needs to not be aware of GraphicsContext / PlatformContext, only skia concepts since it's part of the compositor implementation.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:83
> +    canvasToContentTransform *= context.getCTM().inverse();

Are you just trying to do matrix math here, or actually use GraphicsContext for something else?
Comment 10 James Robinson 2012-06-28 10:35:08 PDT
Comment on attachment 149966 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:83
>> +    canvasToContentTransform *= context.getCTM().inverse();
> 
> Are you just trying to do matrix math here, or actually use GraphicsContext for something else?

also, I'm not sure if getCTM even works with a picture-backed canvas (https://bugs.webkit.org/show_bug.cgi?id=89888)

I'm confused about what this is trying to accomplish.  We should know at this point exactly what transforms exist on the canvas without having to construct extra wrapper classes around it.
Comment 11 vollick 2012-06-28 11:52:35 PDT
Created attachment 149980 [details]
Patch

(In reply to comment #9)
> (From update of attachment 149966 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149966&action=review
>
> > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
> > +    PlatformContextSkia platformContext(canvas);
> > +    GraphicsContext context(&platformContext);
>
> Sorry this is a layering no-no.  CanvasLayerTextureUpdater needs to not be aware of GraphicsContext / PlatformContext, only skia concepts since it's part of the compositor implementation.
Fixed.
>
> > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:83
> > +    canvasToContentTransform *= context.getCTM().inverse();
>
> Are you just trying to do matrix math here, or actually use GraphicsContext for something else?

Yep, I was just trying to do matrix math here, but as you pointed out, we know
everything about the transforms here, so not only do we not need to ask the
canvas for its transform, the matrix math isn't necessary,
either. We will always end up with a straight translation.
Comment 12 vollick 2012-06-28 19:18:45 PDT
Created attachment 150065 [details]
Patch

The last patch had made it so that WebKit::WebContentLayerClient took a rect
in layer space, but returned it in canvas space. This was all to get around a
rounding error that can happen with non-integer scale factors, but it doesn't
make sense to have the rects in different spaces; it puts too much of a burden
on consumers to take pains to deal with the inconsistency. So instead, I've left
OpaqueRectTrackingContentLayerDelegate as it was. It continues to take and
produce layer space rects, and the resulting opaque rect is transformed back
into content space in CanvasLayerTextureUpdater. This can still lead to rounding
errors due to repeatly scaling and converting back to int rects. Eventually,
the WebKit::WebContentLayerClient interface should be modified to return opaque
rects as FloatRects so that they can be scaled a whole lot without accumulating
error before finally being converted to the content space opaque rect.
Comment 13 Dana Jansens 2012-06-28 19:52:22 PDT
Comment on attachment 150065 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [chromium] ContentLayerPainter should generate opaque rects in content space

let's update this title then to something more appropriate
Comment 14 vollick 2012-06-28 20:30:52 PDT
Created attachment 150073 [details]
Patch

Switching to float rects.

Depends on https://chromiumcodereview.appspot.com/10700028/

(In reply to comment #13)
> (From update of attachment 150065 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150065&action=review
>
> > Source/WebCore/ChangeLog:3
> > +        [chromium] ContentLayerPainter should generate opaque rects in content space
>
> let's update this title then to something more appropriate

Done.
Comment 15 WebKit Review Bot 2012-06-28 20:34:03 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 16 WebKit Review Bot 2012-06-28 22:00:35 PDT
Comment on attachment 150073 [details]
Patch

Attachment 150073 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13121185
Comment 17 vollick 2012-06-29 08:26:23 PDT
Created attachment 150183 [details]
Patch

Re-uploading to take another crack at the ews bots now that the chromium side of
the patch and the DEPS roll have landed.
Comment 18 vollick 2012-06-29 11:37:32 PDT
Created attachment 150226 [details]
Patch

Rebasing again. Hopefully this patch will apply
Comment 19 Adrienne Walker 2012-06-29 12:04:00 PDT
Comment on attachment 150226 [details]
Patch

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

R=me.  I find this far more easy to understand than the previous patch.  Thanks also for going ahead and making the FloatRect change.

Also, if you're going to modify function signatures, can you add override while you're at it?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:61
> +    virtual void paint(SkCanvas*, const IntRect& contentRect, FloatRect& opaque);

OVERRIDE

> Source/WebKit/chromium/src/WebContentLayerImpl.h:44
> +    virtual void paintContents(SkCanvas*, const WebCore::IntRect& clip, WebCore::FloatRect& opaque);

OVERRIDE

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:3670
> +    virtual void paintContents(SkCanvas*, const IntRect& clip, FloatRect& opaque) { }

OVERRIDE

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1145
> +    virtual void paintContents(SkCanvas*, const IntRect&, FloatRect&)

OVERRIDE

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1239
> +    void paintContents(SkCanvas*, const IntRect&, FloatRect&) { }

OVERRIDE

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:73
> +    virtual void paint(SkCanvas*, const IntRect&, FloatRect&) { }

OVERRIDE

> Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp:1508
> +    virtual void paint(SkCanvas*, const IntRect& contentRect, FloatRect&)

OVERRIDE
Comment 20 vollick 2012-06-29 12:29:24 PDT
Created attachment 150235 [details]
Patch for landing

Added OVERRIDES.
Comment 21 WebKit Review Bot 2012-06-29 17:25:59 PDT
Comment on attachment 150235 [details]
Patch for landing

Rejecting attachment 150235 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
TextureUpdater.cpp: In member function 'void WebCore::CanvasLayerTextureUpdater::paintContents(SkCanvas*, const WebCore::IntRect&, float, float, WebCore::IntRect&)':
Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:82: error: 'class WebKit::WebTransformationMatrix' has no member named 'mapRect'
make: *** [out/Debug/obj.target/webcore_chromium_compositor/Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13115501
Comment 22 James Robinson 2012-06-29 17:32:50 PDT
(In reply to comment #21)
> (From update of attachment 150235 [details])
> Rejecting attachment 150235 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> TextureUpdater.cpp: In member function 'void WebCore::CanvasLayerTextureUpdater::paintContents(SkCanvas*, const WebCore::IntRect&, float, float, WebCore::IntRect&)':
> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:82: error: 'class WebKit::WebTransformationMatrix' has no member named 'mapRect'
> make: *** [out/Debug/obj.target/webcore_chromium_compositor/Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.o] Error 1
> make: *** Waiting for unfinished jobs....
> 
> Full output: http://queues.webkit.org/results/13115501

Yeah, that's gone (http://trac.webkit.org/changeset/121583).  Think about how you want to handle clipping on the w=0 and use the appropriate CCMathUtil class - I'm pretty sure in this case you will never clip since you're just doing translation/scale, so comment+ASSERT() as such.
Comment 23 vollick 2012-06-29 19:30:16 PDT
Created attachment 150293 [details]
Patch

No longer uses mapRect.
Comment 24 vollick 2012-06-29 19:33:35 PDT
Created attachment 150294 [details]
Patch for landing

Removed unnecessary include.
Comment 25 WebKit Review Bot 2012-06-29 21:56:12 PDT
Comment on attachment 150294 [details]
Patch for landing

Rejecting attachment 150294 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
                 WebKit::WebRect::WebRect(const WebKit::WebRect&)
Source/WebKit/chromium/tests/ContentLayerChromiumTest.cpp:83: error: no match for 'operator=' in 'opaque = resultingOpaqueRect'
out/Debug/obj/gen/webcore_headers/FloatRect.h:81: note: candidates are: WebCore::FloatRect& WebCore::FloatRect::operator=(const WebCore::FloatRect&)
make: *** [out/Debug/obj.target/webkit_unit_tests/Source/WebKit/chromium/tests/ContentLayerChromiumTest.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13117518
Comment 26 vollick 2012-06-30 06:35:13 PDT
Created attachment 150317 [details]
Patch

It looks like you can't use stuff in WEBKIT_IMPLEMENTATION blocks in unit tests.
Comment 27 WebKit Review Bot 2012-06-30 06:42:49 PDT
Comment on attachment 150317 [details]
Patch

Attachment 150317 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13108732
Comment 28 vollick 2012-06-30 07:48:44 PDT
Created attachment 150320 [details]
Patch

Was still using code hidden in WEBKIT_IMPLEMENTATION blocks. Should hopefully be fixed now.
Comment 29 Dana Jansens 2012-06-30 08:16:19 PDT
You will be able to tho https://bugs.webkit.org/show_bug.cgi?id=90094

I thot it had landed but it also got reverted apparently.
Comment 30 Dana Jansens 2012-06-30 08:17:38 PDT
Comment on attachment 150320 [details]
Patch

Assuming that R? is actually a CQ? as you've got the reviewer listed.
Comment 31 WebKit Review Bot 2012-06-30 08:56:11 PDT
Comment on attachment 150320 [details]
Patch

Clearing flags on attachment: 150320

Committed r121628: <http://trac.webkit.org/changeset/121628>
Comment 32 WebKit Review Bot 2012-06-30 08:56:21 PDT
All reviewed patches have been landed.  Closing bug.