[chromium] CanvasLayerTextureUpdater needs to convert opaque rects back to content space.

• Component: WebKit Misc.
• Status: RESOLVED FIXED

Currently, the content layer delegate returns opaque rects in layer space, so we need to map them correctly into content space.

Created attachment 149787 [details]
Patch

[Adrienne Walker]

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?

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

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 :)

[WebKit Review Bot]

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

[WebKit Review Bot]

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

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.

[James Robinson]

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?

[James Robinson]

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.

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.

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.

[Dana Jansens]

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

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.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Comment on attachment 150073 [details]
Patch

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

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.

Created attachment 150226 [details]
Patch

Rebasing again. Hopefully this patch will apply

[Adrienne Walker]

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

Created attachment 150235 [details]
Patch for landing

Added OVERRIDES.

[WebKit Review Bot]

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

[James Robinson]

(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.

Created attachment 150293 [details]
Patch

No longer uses mapRect.

Created attachment 150294 [details]
Patch for landing

Removed unnecessary include.

[WebKit Review Bot]

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

Created attachment 150317 [details]
Patch

It looks like you can't use stuff in WEBKIT_IMPLEMENTATION blocks in unit tests.

[WebKit Review Bot]

Comment on attachment 150317 [details]
Patch

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

Created attachment 150320 [details]
Patch

Was still using code hidden in WEBKIT_IMPLEMENTATION blocks. Should hopefully be fixed now.

[Dana Jansens]

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.

[Dana Jansens]

Comment on attachment 150320 [details]
Patch

Assuming that R? is actually a CQ? as you've got the reviewer listed.

[WebKit Review Bot]

Comment on attachment 150320 [details]
Patch

Clearing flags on attachment: 150320

Committed r121628: <http://trac.webkit.org/changeset/121628>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.