Bug 76336 – [Texmap] TextureMapper creates two many big intermediate surfaces

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Currently TextureMapper is very generous in creating intermediate sufraces. The current algorithm always uses the viewport size for an intermediate surface, and it chooses to create a surface for unnecessary layers, such as ones with reflection and no opacity.

Since intermediate surfaces are a performance and memory hog, optimizing this is of high value.

[Noam Rosenthal]

Created attachment 122552 [details]
Patch

[Noam Rosenthal]

Created attachment 122564 [details]
Patch

[Early Warning System Bot]

Comment on attachment 122564 [details]
Patch

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

[Noam Rosenthal]

Created attachment 122565 [details]
Patch

[Noam Rosenthal]

Created attachment 122566 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Jocelyn Turcotte]

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

[Jocelyn Turcotte]

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

[Noam Rosenthal]

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

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 122642 [details]
Patch

[Noam Rosenthal]

Created attachment 122644 [details]
Patch

[Noam Rosenthal]

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

[Jocelyn Turcotte]

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?

[Simon Hausmann]

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.

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 123185 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123185 [details]
Patch

Clearing flags on attachment: 123185

Committed r105467: <http://trac.webkit.org/changeset/105467>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.