Bug 90078 – [TextureMapper] Enable a depth buffer for preserve-3d

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

[Sergio Villar Senin]

Created attachment 149755 [details]
Simple z-ordering test

I think Z-ordering has some issues in TextureMapperLayer. I'm attaching a test case which consists on the following

- a 300x300 blue div with no transparency defining a new rendering context (preserve-3d)
- a smaller lime div translated -100px in the Z axis

The problem is that the lime div is always visible no matter if the blue div covers it. I just added some animation to make it clearer but it can be safely removed and the bug will be visible as well.

Correct me if I'm wrong but it looks like that when computing the z-ordering, that is only done for the children, so they will be always painted no matter if the first layer obscures them or not.

[Sergio Villar Senin]

Modified the title of the bug because the z-ordering is in fact correct.

[Kenneth Rohde Christiansen]

It is known that we have issues. Lars tried to port the Chromium code for fixing this. I am not sure about the status so cc'ing him

[Sergio Villar Senin]

(In reply to comment #2)
> It is known that we have issues. Lars tried to port the Chromium code for fixing this. I am not sure about the status so cc'ing him

Cool. I'm willing to help with this. Looking forward to hearing about the current status.

[Hyowon Kim]

Created attachment 236857 [details]
Patch

[Hyowon Kim]

*** Bug 133066 has been marked as a duplicate of this bug. ***

[Sergio Villar Senin]

Comment on attachment 236857 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:379
> +        Vector<TextureMapperLayer*> sublayers = layer->children();

Shouldn't this be a reference? Otherwise you're copying the vector.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:384
> +}

I am not an expert on this part of the code, so I won't officially review it, but this kind of recursive operations involving copies into vectors seems terribly expensive to me in painting code.

[Hyowon Kim]

Created attachment 236913 [details]
Patch

[Hyowon Kim]

(In reply to comment #6)
> (From update of attachment 236857 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236857&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:379
> > +        Vector<TextureMapperLayer*> sublayers = layer->children();
> 
> Shouldn't this be a reference? Otherwise you're copying the vector.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:384
> > +}
> 
> I am not an expert on this part of the code, so I won't officially review it, but this kind of recursive operations involving copies into vectors seems terribly expensive to me in painting code.

I've removed the vector copy by adding a reference parameter. Thank you.

[Sergio Villar Senin]

Comment on attachment 236913 [details]
Patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:374
> +    for (size_t i = 0; i < candidates.size(); ++i) {

You can use a modern for loop here. 

for (auto& layer : candidates) {
}

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:395
> +    for (size_t i = 0; i < layersIn3D.size(); ++i)

Same here.

[Hyowon Kim]

Created attachment 236919 [details]
Patch

[Hyowon Kim]

(In reply to comment #9)
> (From update of attachment 236913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236913&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:374
> > +    for (size_t i = 0; i < candidates.size(); ++i) {
> 
> You can use a modern for loop here. 
> 
> for (auto& layer : candidates) {
> }

Okay, done.

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:395
> > +    for (size_t i = 0; i < layersIn3D.size(); ++i)
> 
> Same here.

Okay, done.

Thank you for your review.

[Michael Catanzaro]

Comment on attachment 236919 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.

[Sergio Villar Senin]

Created attachment 403578 [details]
Patch

[Sergio Villar Senin]

(In reply to Sergio Villar Senin from comment #13)
> Created attachment 403578 [details]
> Patch

I've rebased the patch, renamed some operations and fixed some nits. The test was also modernized although it's essentially the same.

[Fujii Hironori]

Why don't you use a depth buffer?
There is BitmapTextureGL::initializeDepthBuffer(), but ti isn't called from anywhere.

[Fujii Hironori]

Created attachment 403654 [details]
test case 2

[Noam Rosenthal]

Recalling our challenge with this bug from 2012...
The proposed patch, and any solution that relies on ordering the layers by their center z, would only fix certain cases, and using depth buffer on its own is only applicable to opaque layers.

What’s needed here is something like “depth peeling”. Back then I remember trying this: http://developer.download.nvidia.com/SDK/10/opengl/src/dual_depth_peeling/doc/DualDepthPeeling.pdf

But mobile hardware of the time and the extra memory needed wasn’t worth the edge cases.

[Miguel Gomez]

(In reply to Noam Rosenthal from comment #17)
> Recalling our challenge with this bug from 2012...
> The proposed patch, and any solution that relies on ordering the layers by
> their center z, would only fix certain cases, and using depth buffer on its
> own is only applicable to opaque layers.

Exactly. The problem of ordering the layers is quite complex. There might be layers that don't have a constant z component, and for those layers the painting order maybe different depending on the point of the layer where the z is sampled. Also, there might be collisions between the layers, and in those cases the layers need to be divided in pieces in order to be painted, which is even more complex.

And, as Noam mentions, the depth buffer helps only with opaque layers.

[Noam Rosenthal]

(In reply to Miguel Gomez from comment #18)
> (In reply to Noam Rosenthal from comment #17)
> > Recalling our challenge with this bug from 2012...
> > The proposed patch, and any solution that relies on ordering the layers by
> > their center z, would only fix certain cases, and using depth buffer on its
> > own is only applicable to opaque layers.
> 
> Exactly. The problem of ordering the layers is quite complex. There might be
> layers that don't have a constant z component, and for those layers the
> painting order maybe different depending on the point of the layer where the
> z is sampled. Also, there might be collisions between the layers, and in
> those cases the layers need to be divided in pieces in order to be painted,
> which is even more complex.
> 
> And, as Noam mentions, the depth buffer helps only with opaque layers.

At the time I also played with a fix that split the layers by 3d intersection edges. This will probably work better than depth peeling, but not easy in terms of how the intersection edges look, and there were lots of weird edge cases (you get to them quickly with a 3d-transform animation and several intersecting preserve-3d elements).

[Fujii Hironori]

Interesting.
So, there are three cases,

if (has3DtransformedLayers) {
   if (hasSemiTransparentLayers) {
     // Needs Order Independent Transparency. Use depth peeling or dual depth peeling?
   } else {
     // Use a single depth buffer
   }
} else {
  // sortByZOrder and use no depth buffer
}

It suffices for the cases using only opaque layers (e.g. comment 1 and comment 16 test cases) to use a single depth buffer,
Right?
Will you try it, Sergio?

[Noam Rosenthal]

(In reply to Fujii Hironori from comment #20)
> Interesting.
> So, there are three cases,
> 
> if (has3DtransformedLayers) {
>    if (hasSemiTransparentLayers) {
>      // Needs Order Independent Transparency. Use depth peeling or dual
> depth peeling?
It could be that you can avoid this complex option in more cases - you only need it when you have 2 or more semi-transparent layers intersecting in all dimensions (roughly speaking). 

Also, full-on depth peeling might be an overkill because each layer on its own is 2d.

It might be enough to do the following:
- find the z-intersection points, divide the intersecting layer groups into non-intersecting z ranges
- paint those one after the other, sorting each one of them separately - essentially paint several passes of the sub-scene - one per each non-intersecting z-range.

>    } else {
>      // Use a single depth buffer
>    }
> } else {
>   // sortByZOrder and use no depth buffer
> }
> 
> It suffices for the cases using only opaque layers (e.g. comment 1 and
> comment 16 test cases) to use a single depth buffer,
> Right?
Yes, those would work fine with depth buffer.

> Will you try it, Sergio?

[Sergio Villar Senin]

(In reply to Fujii Hironori from comment #20)
> Interesting.
> So, there are three cases,
> 
> if (has3DtransformedLayers) {
>    if (hasSemiTransparentLayers) {
>      // Needs Order Independent Transparency. Use depth peeling or dual
> depth peeling?
>    } else {
>      // Use a single depth buffer
>    }
> } else {
>   // sortByZOrder and use no depth buffer
> }
> 
> It suffices for the cases using only opaque layers (e.g. comment 1 and
> comment 16 test cases) to use a single depth buffer,
> Right?
> Will you try it, Sergio?

No sorry. I just tried to quickly revive the bug but I don't have time ATM to work on this. Happy to hand it over to anyone who cares.

[Fujii Hironori]

Created attachment 404799 [details]
WIP patch

This WIP patch works well for the test cases (comment 1 and comment 16) and https://webkit.org/blog/386/3d-transforms/

[Fujii Hironori]

The WIP patch makes transforms/2d/preserve3d-not-fixed-container.html fail.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #24)
> The WIP patch makes transforms/2d/preserve3d-not-fixed-container.html fail.

Here is the content.

>   <div class="preserve3d box" style="margin: 150px 50px;">
>     <div class="fixed box">
>         <div class="transformed box"></div> <!-- Necessary to activate preserve3d compositing -->
>     </div>
>   </div>

This test is failing becuase the second div is placed under the first div.
The both div elements are placed in the same depth.
Specifying glDepthFunc(GL_LEQUAL) renders the second div on the first div.

[Fujii Hironori]

Created attachment 409663 [details]
WIP patch

[Fujii Hironori]

Created attachment 409862 [details]
Patch

[Fujii Hironori]

Comment on attachment 409862 [details]
Patch

Clearing flags on attachment: 409862

Committed r267711: <https://trac.webkit.org/changeset/267711>

[Fujii Hironori]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/69713871>

[Noam Rosenthal]

As I mentioned in previous comments, this would not work well with transparency, and there are no tests with semi-transparent layers. The preserve-3d check should also check that all the 3d-preserving layers are fully opaque.

Also, begin/end Preserve3d is not a stack - I don't think this would work correctly with a deep tree of 3d-preserving layers. If this is incorrect, would be good to have a test for that.

[Fujii Hironori]

Yup. I didn't implement it because it's too difficult to me.
Filed: Bug 217080 – [TextureMapper] Order Independent Transparency support

[Noam Rosenthal]

(In reply to Fujii Hironori from comment #32)
> Yup. I didn't implement it because it's too difficult to me.
> Filed: Bug 217080 – [TextureMapper] Order Independent Transparency support

Understood, it's indeed difficult - but without at least checking that all the 3d-preserved layers are opaque, the current patch will cause regressions, and will look really bad in some situations: if you have two transparent layers with 
a z intersection, they would totally lose their blending with each other.

[Fujii Hironori]

Will fix.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #34)
> Will fix.

Filed: Bug 217103 – [TextureMapper] A semi-transparent parent layer is rendered as a opaque layer for children layers