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.
Created attachment 181240 [details] Patch
Comment on attachment 181240 [details] Patch TY.
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
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
(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.
Created attachment 181550 [details] Patch
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
Created attachment 181582 [details] Patch
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?
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.
(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!
(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
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.
Created attachment 182379 [details] Patch for landing
Comment on attachment 182379 [details] Patch for landing Clearing flags on attachment: 182379 Committed r139479: <http://trac.webkit.org/changeset/139479>
All reviewed patches have been landed. Closing bug.