Bug 106047 – RenderGeometryMap and TransformState disagree with sub-pixel layout and translations

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

RenderGeometryMap has an optimized code path for integer transforms that merely translate (such as the all-too-common translatez(0) ) that applies the translate as an offset. This leads to pixel snapping occurring later in the RenderGeometryMap case. I think this should carry over to the TransformState case (this could be argued) and propose mirroring that optimization there. This should also be a performance win.

[Levi Weintraub]

Created attachment 181240 [details]
Patch

[Levi Weintraub]

Comment on attachment 181240 [details]
Patch

TY.

[WebKit Review Bot]

Comment on attachment 181240 [details]
Patch

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

New failing tests:
fast/layers/geometry-map-transform-state-translation-mismatch.html

[Build Bot]

Comment on attachment 181240 [details]
Patch

Attachment 181240 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15686050

New failing tests:
compositing/visible-rect/3d-transform-style.html
compositing/visible-rect/nested-transform.html
svg/as-image/img-preserveAspectRatio-support-2.html
compositing/visible-rect/iframe-and-layers.html
compositing/visible-rect/clipped-visible-rect.html
fast/layers/geometry-map-transform-state-translation-mismatch.html
compositing/visible-rect/2d-transformed.html
compositing/visible-rect/animated.html
compositing/visible-rect/3d-transformed.html
compositing/visible-rect/animated-from-none.html

[Levi Weintraub]

(In reply to comment #4)
> (From update of attachment 181240 [details])
> Attachment 181240 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15686050
> 
> New failing tests:
> compositing/visible-rect/3d-transform-style.html
> compositing/visible-rect/nested-transform.html
> svg/as-image/img-preserveAspectRatio-support-2.html
> compositing/visible-rect/iframe-and-layers.html
> compositing/visible-rect/clipped-visible-rect.html
> fast/layers/geometry-map-transform-state-translation-mismatch.html
> compositing/visible-rect/2d-transformed.html
> compositing/visible-rect/animated.html
> compositing/visible-rect/3d-transformed.html
> compositing/visible-rect/animated-from-none.html

It looks like we may do the wrong thing in GraphicsLayerCA::computeVisibleRect with this change. I'm taking a look.

[Levi Weintraub]

Created attachment 181550 [details]
Patch

[Build Bot]

Comment on attachment 181550 [details]
Patch

Attachment 181550 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15740837

New failing tests:
fast/layers/geometry-map-transform-state-translation-mismatch.html

[Levi Weintraub]

Created attachment 181582 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 181582 [details]
Patch

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

> Source/WebCore/platform/graphics/transforms/TransformState.h:81
> +        // We need to clear the accumulated offset when resetting the quad or we'll incorrectly apply the
> +        // accumulated offset to this new quad. This only works if we're not also mapping a point, otherwise
> +        // we'd lose the accumulated offset for that point.
> +        ASSERT(!m_mapPoint);

This is weird. Isn't blowing away m_accumulatedOffset the wrong thing to do here?

[Levi Weintraub]

Comment on attachment 181582 [details]
Patch

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

>> Source/WebCore/platform/graphics/transforms/TransformState.h:81
>> +        ASSERT(!m_mapPoint);
> 
> This is weird. Isn't blowing away m_accumulatedOffset the wrong thing to do here?

Sorry for the delayed response. Because we only use m_accumulatedOffset when we're flattening or don't have a transform, it's guaranteed to only be a translate. Prior to m_accumulatedOffset, these translates would have been applied directly to the point/quad and then forgotten, so when you set a new Quad, they wouldn't get applied.

[Levi Weintraub]

(In reply to comment #10)
> (From update of attachment 181582 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181582&action=review
> 
> >> Source/WebCore/platform/graphics/transforms/TransformState.h:81
> >> +        ASSERT(!m_mapPoint);
> > 
> > This is weird. Isn't blowing away m_accumulatedOffset the wrong thing to do here?
> 
> Sorry for the delayed response. Because we only use m_accumulatedOffset when we're flattening or don't have a transform, it's guaranteed to only be a translate. Prior to m_accumulatedOffset, these translates would have been applied directly to the point/quad and then forgotten, so when you set a new Quad, they wouldn't get applied.

It seemed weird to me as well until I dug into why we started failing tests on Mac. Let me know if you have any more questions or don't like the approach.  I'd love to fix this last source of assertions!

[Simon Fraser (smfr)]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 181582 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181582&action=review
> > 
> > >> Source/WebCore/platform/graphics/transforms/TransformState.h:81
> > >> +        ASSERT(!m_mapPoint);
> > > 
> > > This is weird. Isn't blowing away m_accumulatedOffset the wrong thing to do here?
> > 
> > Sorry for the delayed response. Because we only use m_accumulatedOffset when we're flattening or don't have a transform, it's guaranteed to only be a translate. Prior to m_accumulatedOffset, these translates would have been applied directly to the point/quad and then forgotten, so when you set a new Quad, they wouldn't get applied.
> 
> It seemed weird to me as well until I dug into why we started failing tests on Mac. Let me know if you have any more questions or don't like the approach.  I'd love to fix this last source of assertions!

So before m_accumulatedOffset existed, moving would just offset the point and/or the quad. 

http://trac.webkit.org/changeset/137847/trunk/Source/WebCore/platform/graphics/transforms/TransformState.cpp changed this to accumulate into the LayoutSize if flattening, or otherwise apply any existing offset, zero m_accumulatedOffset, then move the point/quad/transform, I think?

I think the key here is that setQuad() implicitly assumes that the q

[Simon Fraser (smfr)]

Trying again:

I think the key here is that setQuad() implicitly assumes that the quad is in the coordinate system of the current state (so it's OK to throw away the accumulated offset), and that in turn requires that no point is being mapped.

So I think the comment could be improved but the change is sound.

[Levi Weintraub]

Created attachment 182379 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182379 [details]
Patch for landing

Clearing flags on attachment: 182379

Committed r139479: <http://trac.webkit.org/changeset/139479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.