Bug 217943 – TextureMapperLayer::paintWithIntermediateSurface: Reduce BitmapTextures by unifying replicaSurface and mainSurface

• Component: Platform
• Status: RESOLVED FIXED

[Fujii Hironori]

TextureMapperLayer::paintWithIntermediateSurface: Reduce BitmapTextures by unifying replicaSurface and mainSurface

TextureMapperLayer::paintWithIntermediateSurface is using two BitmapTextures for replicaSurface and mainSurface.
But, IIUC, a single BitmapTexture suffices.

[Fujii Hironori]

Created attachment 411841 [details]
Patch

[Noam Rosenthal]

Comment on attachment 411841 [details]
Patch

IIRC the second surface is necessary. Since the replica might have a different opacity/transform/mask than the main surface, it needs to blend separately, and it also needs a different size surface due to the different transform.

This patch would result in the main surface clipping the reflection, and blending it incorrectly. 
I'm sure some of the tests in compositing/reflection would catch this, not sure if they're skipped for GTK/WPE

[Fujii Hironori]

I'm testing with GTK port which runs all compositing/reflection 30 test cases. I observers no regressions.

[Noam Rosenthal]

(In reply to Fujii Hironori from comment #3)
> I'm testing with GTK port which runs all compositing/reflection 30 test
> cases. I observers no regressions.

OK then. Will let current Texmap people review :)

[Radar WebKit Bug Importer]

<rdar://problem/70710946>

[Miguel Gomez]

I'm not an expert in this replica code, but I've been looking into how it works and what this patch does.

* From what I see, TextureMapperLayer::paintWithIntermediateSurface() paints the replicaLayer first into a surface with opacity 1 and its own transformation.

* If the original opacity is 1 it already paints the surface content into the frambuffer with the original opacity.

* Then it paints the main layer with opacity 1 into a new surface.

* If the original opacity is not 1 (which would mean that the replica surface has already been painted), the main surface is painted into the replica surface with opacity 1, and then the result is painted to the framebuffer with the real opacity.

From this code I see that both the main layer and the replica layer are painted with the same opacity.
The rect received to paint is the same for both layers, and its size includes the size for both layers: if the original layer is 200x200 and the reflection is below, the passed rect will be 200x400.
The transformation is different for those layers, but the replica one just performs the move&flip for the reflection. I've been checking the CSS doc and I don't see how a replica layer can have more transformations than those (can't be scaled or rotated or anything else, am I wrong?), which means that it should fit into the target rect.

So, taking into account all those things, this patch seems to have the same behavior than the existent code:

* The allocated surface is has the size of the passed rect, which is enough to paint both layers.
* The replica layer is painted into the surface with opacity 1
* The main layer is painted into the same surface with opacity 1
* The result is painted into the framebuffer with the original opacity

So, unless I'm missing some use cases that I don't know about, I think that this patch should be ok.

[Noam Rosenthal]

(In reply to Miguel Gomez from comment #6)
> I'm not an expert in this replica code, but I've been looking into how it
> works and what this patch does.
> 
> * From what I see, TextureMapperLayer::paintWithIntermediateSurface() paints
> the replicaLayer first into a surface with opacity 1 and its own
> transformation.
> 
> * If the original opacity is 1 it already paints the surface content into
> the frambuffer with the original opacity.
> 
> * Then it paints the main layer with opacity 1 into a new surface.
> 
> * If the original opacity is not 1 (which would mean that the replica
> surface has already been painted), the main surface is painted into the
> replica surface with opacity 1, and then the result is painted to the
> framebuffer with the real opacity.
> 
> From this code I see that both the main layer and the replica layer are
> painted with the same opacity.
> The rect received to paint is the same for both layers, and its size
> includes the size for both layers: if the original layer is 200x200 and the
> reflection is below, the passed rect will be 200x400.
> The transformation is different for those layers, but the replica one just
> performs the move&flip for the reflection. I've been checking the CSS doc
> and I don't see how a replica layer can have more transformations than those
> (can't be scaled or rotated or anything else, am I wrong?), which means that
> it should fit into the target rect.
> 
> So, taking into account all those things, this patch seems to have the same
> behavior than the existent code:
> 
> * The allocated surface is has the size of the passed rect, which is enough
> to paint both layers.
> * The replica layer is painted into the surface with opacity 1
> * The main layer is painted into the same surface with opacity 1
> * The result is painted into the framebuffer with the original opacity
> 
> So, unless I'm missing some use cases that I don't know about, I think that
> this patch should be ok.

I wrote the original replica code and I believe the additional surface was originally needed because of something to do with the surface sizes, which was since changed.

btw CSS reflections have never been fully standardized and are not supported by Firefox :(
TextureMapper can probably become much simpler if it didn't support those.

[Fujii Hironori]

Thank you for the review, Miguel and Noam.

[Fujii Hironori]

Comment on attachment 411841 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:451
>          paintOptions.backdropLayer = nullptr;

I moved paintOptions to the local scope of if.  I can remove this line.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:-458
> -    surface = surface->applyFilters(options.textureMapper, m_currentFilters);

I shouldn't remove this applyFilters. The subsequent applyMask is using it. As far as I tested, the combination of reflection and mask doesn't work as expected in TextureMapper. I'll add a test case in a new bug.

[Fujii Hironori]

Created attachment 412508 [details]
Patch

[Fujii Hironori]

Comment on attachment 412508 [details]
Patch

Clearing flags on attachment: 412508

Committed r269116: <https://trac.webkit.org/changeset/269116>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.