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.
Modified the title of the bug because the z-ordering is in fact correct.
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
(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.
Created attachment 236857 [details] Patch
*** Bug 133066 has been marked as a duplicate of this bug. ***
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.
Created attachment 236913 [details] Patch
(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.
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.
Created attachment 236919 [details] Patch
(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.
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.
Created attachment 403578 [details] Patch
(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.
Why don't you use a depth buffer? There is BitmapTextureGL::initializeDepthBuffer(), but ti isn't called from anywhere.
Created attachment 403654 [details] test case 2
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.
(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.
(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).
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?
(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?
(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.
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/
The WIP patch makes transforms/2d/preserve3d-not-fixed-container.html fail.
(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.
Created attachment 409663 [details] WIP patch
Created attachment 409862 [details] Patch
Comment on attachment 409862 [details] Patch Clearing flags on attachment: 409862 Committed r267711: <https://trac.webkit.org/changeset/267711>
All reviewed patches have been landed. Closing bug.
<rdar://problem/69713871>
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.
Yup. I didn't implement it because it's too difficult to me. Filed: Bug 217080 – [TextureMapper] Order Independent Transparency support
(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.
Will fix.
(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