Bug 90078

Summary:

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

Product:

WebKit

Reporter:

Sergio Villar Senin <svillar>

Component:

Layout and Rendering

Assignee:

Hyowon Kim <hw1008.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

alex, cgarcia, cmarcelo, commit-queue, don.olmstead, fujii.hironori, hw1008.kim, kenneth, kondapallykalyan, larsgk, luiz, magomez, mrobinson, noam, sabouhallawa, simon.fraser, svillar, webkit-bug-importer, yael, zan

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Linux

See Also:

https://bugs.webkit.org/show_bug.cgi?id=244526

Attachments:

Description	Flags
Simple z-ordering test	none
Patch	none
Patch	none
Patch	none
Patch	none
test case 2	none
WIP patch	none
WIP patch	none
Patch	none

Sergio Villar Senin

Reported 2012-06-27 08:43:13 PDT

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.

Attachments
Simple z-ordering test (928 bytes, text/html) 2012-06-27 08:43 PDT, Sergio Villar Senin	no flags	Details
Patch (8.70 KB, patch) 2014-08-19 23:44 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (8.77 KB, patch) 2014-08-20 22:07 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (8.68 KB, patch) 2014-08-21 02:30 PDT, Hyowon Kim	no flags	Details Formatted Diff Diff
Patch (9.50 KB, patch) 2020-07-06 01:07 PDT, Sergio Villar Senin	no flags	Details Formatted Diff Diff
test case 2 (920 bytes, text/html) 2020-07-06 19:31 PDT, Fujii Hironori	no flags	Details
WIP patch (4.52 KB, patch) 2020-07-20 23:41 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
WIP patch (5.21 KB, patch) 2020-09-25 00:29 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Patch (8.76 KB, patch) 2020-09-27 17:07 PDT, Fujii Hironori	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Sergio Villar Senin

Comment 1 2012-07-03 11:00:39 PDT

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

Kenneth Rohde Christiansen

Comment 2 2012-07-03 21:28:33 PDT

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

Comment 3 2012-07-04 00:59:16 PDT

(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

Comment 4 2014-08-19 23:44:30 PDT

Created attachment 236857 [details] Patch

Hyowon Kim

Comment 5 2014-08-20 00:24:57 PDT

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

Sergio Villar Senin

Comment 6 2014-08-20 03:58:29 PDT

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

Comment 7 2014-08-20 22:07:21 PDT

Created attachment 236913 [details] Patch

Hyowon Kim

Comment 8 2014-08-20 22:20:02 PDT

(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 9 2014-08-21 00:42:25 PDT

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

Comment 10 2014-08-21 02:30:45 PDT

Created attachment 236919 [details] Patch

Hyowon Kim

Comment 11 2014-08-21 02:31:49 PDT

(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 12 2016-09-17 07:10:47 PDT

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

Comment 13 2020-07-06 01:07:11 PDT

Created attachment 403578 [details] Patch

Sergio Villar Senin

Comment 14 2020-07-06 01:08:20 PDT

(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

Comment 15 2020-07-06 16:49:07 PDT

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

Fujii Hironori

Comment 16 2020-07-06 19:31:17 PDT

Created attachment 403654 [details] test case 2

Noam Rosenthal

Comment 17 2020-07-07 00:04:33 PDT

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

Comment 18 2020-07-07 00:49:39 PDT

(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

Comment 19 2020-07-07 06:46:45 PDT

(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

Comment 20 2020-07-12 17:10:23 PDT

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

Comment 21 2020-07-13 00:59:43 PDT

(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

Comment 22 2020-07-14 00:55:48 PDT

(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

Comment 23 2020-07-20 23:41:42 PDT

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

Comment 24 2020-07-21 18:38:41 PDT

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

Fujii Hironori

Comment 25 2020-09-25 00:28:36 PDT

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

Comment 26 2020-09-25 00:29:45 PDT

Created attachment 409663 [details] WIP patch

Fujii Hironori

Comment 27 2020-09-27 17:07:14 PDT

Created attachment 409862 [details] Patch

Fujii Hironori

Comment 28 2020-09-28 13:05:03 PDT

Comment on attachment 409862 [details] Patch Clearing flags on attachment: 409862 Committed r267711: <https://trac.webkit.org/changeset/267711>

Fujii Hironori

Comment 29 2020-09-28 13:05:08 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 30 2020-09-28 13:06:23 PDT

<rdar://problem/69713871>

Noam Rosenthal

Comment 31 2020-09-28 23:01:42 PDT

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

Comment 32 2020-09-29 00:34:44 PDT

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

Noam Rosenthal

Comment 33 2020-09-29 00:38:12 PDT

(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

Comment 34 2020-09-29 01:50:30 PDT

Will fix.

Fujii Hironori

Comment 35 2020-09-29 13:53:33 PDT

(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

Note You need to log in before you can comment on or make changes to this bug.