Bug 76336

Created attachment 122552 [details] Patch

Created attachment 122564 [details] Patch

Comment on attachment 122564 [details] Patch Attachment 122564 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11242195

Created attachment 122565 [details] Patch

Created attachment 122566 [details] Patch

Comment on attachment 122566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122566&action=review This seems so much more simple > Source/WebCore/ChangeLog:11 > + 2. Avoid generatig intermediate surface for occasions where they're not necessary, generating > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:281 > + // We apply the transform that is generated to compensate for painting into a surface. We apply the following transform to compensate for painting into a surface.. Maybe the last part could be described better > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:358 > + return boundingRect(m_transform.combined().inverse()); will there always be an inverse? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:361 > +IntRect TextureMapperNode::boundingRect(const TransformationMatrix& matrix) Maybe we should rename this method? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:389 > + // Tested by compositing/reflections/reflection-opacity.html I love these comments! > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:416 > + // FIXME: blend the two if both exist. Any test for that? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:458 > + // If we painted the replica, the mask is already baked in so we don't need to paint it again. included? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:463 > + TransformationMatrix targetTransform = TransformationMatrix().multiply(options.transform).multiply(m_transform.combined()).translate(options.offset.width(), options.offset.height()); Maybe write this on a couple of lines

Comment on attachment 122566 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122566&action=review > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:34 > + // If the surface has only one reference, we can safely reuse it. Please add something like "... only one reference (the reference of m_texturePool), we can...". > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:285 > + TransformationMatrix transform = > + TransformationMatrix(options.transform) > + .multiply(m_transform.combined()) > + .translate(options.offset.width(), options.offset.height()); Since we now compute the transforms just before painting, it feels wasted to do: - Compute the transforms for the whole graphic layer tree before we start painting. - During painting, possibly reverse a whole part of the transformed branch in case we are rendering to an intermediate surface. So it might be simpler and more efficient to integrate computeTransformsRecursive into paintSelf and decide if we combine with the parent transform or not. I'm not sure how much work that would be so this could be done in another patch. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:422 > + replicaOptions.transform > + .multiply(m_state.replicaLayer->m_transform.combined()) > + .multiply(m_transform.combined().inverse()); If I understand well I think that "replicaOptions.transform = TransformationMatrix(m_state.replicaLayer->m_transform.combined()).multiply(m_transform.combined().inverse())" would be clearer and would avoid an extra TransformationMatrix::multiply call. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:453 > + paintOptions.transform = m_transform.combinedForChildren().inverse(); Humm, this is stretching my brain a bit, but I think that this should be m_transform.combined().inverse(), else this would revert the perspective transform in combinedForChildren which needs to act on the Z component, and rendering on the intermediate surface will flatten the Z. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:464 > + TransformationMatrix targetTransform = TransformationMatrix().multiply(options.transform).multiply(m_transform.combined()).translate(options.offset.width(), options.offset.height()); > + options.textureMapper->drawTexture(*surface.get(), surfaceRect, targetTransform, options.opacity, maskTexture.get()); Same thing here, TransformationMatrix().multiply(options.transform) can be simplified to TransformationMatrix(options.transform).

(In reply to comment #7) > If I understand well I think that "replicaOptions.transform = TransformationMatrix(m_state.replicaLayer->m_transform.combined()).multiply(m_transform.combined().inverse())" would be clearer and would avoid an extra TransformationMatrix::multiply call. Ok sorry, didn't see that replicaOptions was containing the transform of options, ignore that one please.

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:416 > > + // FIXME: blend the two if both exist. > > Any test for that? No test. That's why I didn't bother with it at this point. Also, very few sites use reflections at all, or in that way specifically (having a mask on a layer, and then reflecting it with an additional mask). > Since we now compute the transforms just before painting, it feels wasted to do: > - Compute the transforms for the whole graphic layer tree before we start painting. > - During painting, possibly reverse a whole part of the transformed branch in case we are rendering to an intermediate surface. > > So it might be simpler and more efficient to integrate computeTransformsRecursive into paintSelf and decide if we combine with the parent transform or not. I'm not sure how much work that would be so this could be done in another patch. True, those inverses are kind of yucky. Though in the future I want to allow computing the transform only if needed, as an optimization. Also, the bounding rect for root is still needed for visible-rect calculations. In short, I'd rather leave this for another patch. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:453 > > + paintOptions.transform = m_transform.combinedForChildren().inverse(); > > Humm, this is stretching my brain a bit, but I think that this should be m_transform.combined().inverse(), else this would revert the perspective transform in combinedForChildren which needs to act on the Z component, and rendering on the intermediate surface will flatten the Z. This has to use combinedForChildren(). Otherwise it doesn't flatten non-preserve-3d surfaces correctly. I'll add a coment.

> Since we now compute the transforms just before painting, it feels wasted to do: > - Compute the transforms for the whole graphic layer tree before we start painting. > - During painting, possibly reverse a whole part of the transformed branch in case we are rendering to an intermediate surface. I tried to do this, but it makes replica rendering tricky...

Created attachment 122642 [details] Patch

Created attachment 122644 [details] Patch

Anything still missing for this? I have several patches that depend on it...

Comment on attachment 122644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122644&action=review LGTM along with some nitpicks. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:430 > +void TextureMapperNode::paintRecursive(TextureMapperPaintOptions options) The implicit copy of this TextureMapperPaintOptions isn't necessary anymore if no intemediate surface is used. Would it be possible to use a const reference and do the additional copy to apply the opacity on the intermediate surface only when you know you'll use it? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:455 > + // We have to use combinedForChildren() and not combined(), otherwise layer 3D doesn't work. layer 3D -> preserves 3D?

Comment on attachment 122644 [details] Patch Not an easy patch, but I think I get it now :). r=me, but Jocelyn's comments make sense. Would be good to apply before landing.

(In reply to comment #15) > (From update of attachment 122644 [details]) > Not an easy patch, but I think I get it now :). r=me, but Jocelyn's comments make sense. Would be good to apply before landing. Thanks Simon, I'm writing some non-easy patches in order to make it easier for others later :)

Created attachment 123185 [details] Patch

Comment on attachment 123185 [details] Patch Clearing flags on attachment: 123185 Committed r105467: <http://trac.webkit.org/changeset/105467>

All reviewed patches have been landed. Closing bug.