[chromium] Allow modification of size of partially occluded quads. [not for review, yet]
Created attachment 122573 [details] Patch
Comment on attachment 122573 [details] Patch This patch is not quite done yet. I would appreciate comments on the approach, specifically w.r.t. to the two issues mentioned below. 1) It seems ugly having to have the special modification for CCTileDrawQuad, yet it fails without it. When we re-size the quad, we need to say where the new quad starts in the texture ... We can't just do this via the clip rect, as this is shared with all quads in the layer. 2) The code specific to CCTileDrawQuad in CCQuadCuller seems to invoke some sort of pointer error when running CCQuadCullerTest.verifyCullCenterTileOnly (which is the only CCQuadCuller unit test that will invoke the quad resizing logic). When run, it gives the following (truncated) error dump: [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from CCQuadCullerTest [ RUN ] CCQuadCullerTest.verifyCullCenterTileOnly *** glibc detected *** out/Debug/webkit_unit_tests: munmap_chunk(): invalid pointer: 0x000000000242c640 *** ======= Backtrace: ========= /lib/libc.so.6(+0x775b6)[0x7f90bc03f5b6] . . . 183:0115/163925:532185838963:ERROR:process_util_posix.cc(138)] Received signal 6 base::debug::StackTrace::StackTrace() [0x7f90c4675492] base::(anonymous namespace)::StackDumpSignalHandler() [0x7f90c46d2cfd] 0x7f90bbffbaf0 0x7f90bbffba75 0x7f90bbfff5c0 0x7f90bc0354fb 0x7f90bc03f5b6 WTF::deleteOwnedPtr<>() [0x7f90c56b9567] WTF::OwnPtr<>::~OwnPtr() [0x7f90c56b954d] WTF::VectorDestructor<>::destruct() [0x7f90c56b9466] WTF::VectorTypeOperations<>::destruct() [0x7f90c56b9360] WTF::Vector<>::shrink() [0x7f90c56b90ef] WTF::Vector<>::~Vector() [0x7f90c56b8e09] (anonymous namespace)::CCQuadCullerTest_verifyCullCenterTileOnly_Test::TestBody() [0x7f90c56b720c] testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x7f90c5a6af05]
Created attachment 122624 [details] Patch
Comment on attachment 122624 [details] Patch Fixed logic error in test for bounds = region (was failing for 'L'-shaped regions). Note: this patch should work OK for all quad types except perhaps CustomDrawQuads and RenderSurfaceDraw quads - I haven't been able to test with those yet.
Comment on attachment 122624 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122624&action=review > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:63 > + rectRegion.subtract(Region(boundsRect)); You don't need to Region() as the constructor is not explicit. Another review asked me to just pass the IntRect in this case before. This is rectRegion.subtract(rectRegion.bounds()). When will this return anything but an empty set? > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:65 > + if (boundsRect.isEmpty() || rectRegion.isEmpty()) boundsRect is the containing bounds of rectRegion. Is not one empty if and only if the other is empty? I feel like maybe there are some tests missing? > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:93 > + if (drawQuad->material() == CCDrawQuad::TiledContent) { virtual function?
(In reply to comment #5) > (From update of attachment 122624 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122624&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:63 > > + rectRegion.subtract(Region(boundsRect)); > > You don't need to Region() as the constructor is not explicit. Another review asked me to just pass the IntRect in this case before. > > This is rectRegion.subtract(rectRegion.bounds()). When will this return anything but an empty set? > > > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:65 > > + if (boundsRect.isEmpty() || rectRegion.isEmpty()) > > boundsRect is the containing bounds of rectRegion. Is not one empty if and only if the other is empty? I feel like maybe there are some tests missing? rectRegion, at this point, may not be rectangular (we've subtracted off the intersection between it and the occluded region). Perhaps needs a new name. At any rate, I've changed the logic (subtracted things in the wrong order), based on the code you quote above. > > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:93 > > + if (drawQuad->material() == CCDrawQuad::TiledContent) { > > virtual function? I think these were explicitly avoided during the original design of the DrawQuad classes (see LayerRendererChromium::drawQuad()), so I don't want to introduce them here until Enne has looked at it.
Created attachment 122641 [details] Patch
Comment on attachment 122641 [details] Patch Fixed the unit test - we were deriving from CCDrawQuad, but setting the materials to be TiledContent ... bad idea if you pass it to code that believes what it's told by material().
Comment on attachment 122641 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122641&action=review > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:97 > + // FIXME: We shouldn't need to handle special quad cases here. Should > + // CCTileDrawQuad somehow do this computation internally and automatically, > + // by assuming changes to it's drawQuad always remap the texture offset? Another option might be to store the visible rect separately from quad rect on CCDrawQuad, and just have the specialized quad drawing functions handle it, since they're already special-cased by quad type.
(In reply to comment #9) > (From update of attachment 122641 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122641&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:97 > > + // FIXME: We shouldn't need to handle special quad cases here. Should > > + // CCTileDrawQuad somehow do this computation internally and automatically, > > + // by assuming changes to it's drawQuad always remap the texture offset? > > Another option might be to store the visible rect separately from quad rect on CCDrawQuad, and just have the specialized quad drawing functions handle it, since they're already special-cased by quad type. Excellent idea! Will revise and re-submit shortly ...
Created attachment 122955 [details] Patch
Comment on attachment 122955 [details] Patch Attachment 122955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11283321 New failing tests: compositing/color-matching/image-color-matching.html
Comment on attachment 122955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122955&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539 > + const IntRect& tileRect = quad->quadVisibleRect(); What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect? I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision. > Source/WebCore/platform/graphics/chromium/cc/CCTileDrawQuad.h:42 > + void setTextureOffset(const IntPoint& newOffset) { m_textureOffset = newOffset; } nit: This isn't used anywhere.
Comment on attachment 122955 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122955&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539 >> + const IntRect& tileRect = quad->quadVisibleRect(); > > What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect? > > I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision. Question: at present we're always making a vertical or horizontal cut to get the visible rect, so one edge will line up (leave it as-is?), one edge won't (turn off edgeAA if it used to be on?), but the remaining edges line up, but are *shorter* than the original edge (here leave edgeAA on if it was originally on, I guess, as these shorter edges still line up with where they use to?). I agree it could change in the future, although we could perhaps guard against that by using an ASSERT. That said, I'm happy to figure out the logic to test each edge if it has edgeAA, and turn it off if the edge has 'moved'. I'd rather not fall back to just using the whole rect if we can avoid it. Does that seem reasonable? >> Source/WebCore/platform/graphics/chromium/cc/CCTileDrawQuad.h:42 >> + void setTextureOffset(const IntPoint& newOffset) { m_textureOffset = newOffset; } > > nit: This isn't used anywhere. Thanks, missed removing this. > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:29 > +#include "cc/CCTileDrawQuad.h" This needs to go too ...
(In reply to comment #14) > (From update of attachment 122955 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122955&action=review > > >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:539 > >> + const IntRect& tileRect = quad->quadVisibleRect(); > > > > What about edge AA? Perhaps if the visible rect edge isn't the same as the quad rect edge, then you need to turn off edge AA for that edge? Or, if there's any edge AA, then use the quad rect? > > > > I mean, it's the case that you don't set a visible rect if the layer isn't axis aligned, and we also don't have edge AA either when that happens, but that feels like coincidence (that could change in the future), rather than an explicit decision. > > Question: at present we're always making a vertical or horizontal cut to get the visible rect, so one edge will line up (leave it as-is?), one edge won't (turn off edgeAA if it used to be on?), but the remaining edges line up, but are *shorter* than the original edge (here leave edgeAA on if it was originally on, I guess, as these shorter edges still line up with where they use to?). > > I agree it could change in the future, although we could perhaps guard against that by using an ASSERT. That said, I'm happy to figure out the logic to test each edge if it has edgeAA, and turn it off if the edge has 'moved'. Sure, that seems like a reasonable approach.
Created attachment 123128 [details] Patch
Added disabling of AA on edges that differ between quadRect and quadVisibleRect.
Comment on attachment 123128 [details] Patch Attachment 123128 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11253516 New failing tests: compositing/color-matching/image-color-matching.html
(In reply to comment #18) > (From update of attachment 123128 [details]) > Attachment 123128 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11253516 > > New failing tests: > compositing/color-matching/image-color-matching.html Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied.
(In reply to comment #19) > (In reply to comment #18) > > (From update of attachment 123128 [details] [details]) > > Attachment 123128 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/11253516 > > > > New failing tests: > > compositing/color-matching/image-color-matching.html > > Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied. Debugging it right now :-)
(In reply to comment #20) > (In reply to comment #19) > > (In reply to comment #18) > > > (From update of attachment 123128 [details] [details] [details]) > > > Attachment 123128 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/11253516 > > > > > > New failing tests: > > > compositing/color-matching/image-color-matching.html > > > > Can you fix this? There's a nice big horizontal ~1px black line across the bottom set of images with your patch applied. > > Debugging it right now :-) Fix found, waiting for 76658 to clear.
Created attachment 123547 [details] Patch
This patch should fix the failing layout test.
Comment on attachment 123547 [details] Patch LGTM.
I can't tell from the title, ChangeLog, or bug comments what this patch is actually trying to accomplish. What problem is this solving? What functionality does it enable?
(In reply to comment #25) > I can't tell from the title, ChangeLog, or bug comments what this patch is actually trying to accomplish. What problem is this solving? What functionality does it enable? At present, the draw-culler either culls a quad because it is completely occluded, or it draws the entire quad (even if it is partially occluded). This patch allows us to draw only the visible part of partially occluded quads. It seems to help quite a bit in reducing overdraw.
Created attachment 123729 [details] Patch
Comment on attachment 123729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123729&action=review > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:62 > + m_quadVisibleRect = quadVisibleRect; > + m_quadVisibleRect.intersect(m_quadRect); this sort of logic belongs in the .cpp, and the behavior should be documented in the header since this isn't a simple setter > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:107 > + IntRect transformedQuadRect(enclosedIntRect(floatTransformedRect)); > + > + IntRect transformedVisibleQuadRect = rectSubtractRegion(opaqueCoverageThusFar, transformedQuadRect); > + bool keepQuad = !transformedVisibleQuadRect.isEmpty(); this looks wrong. consider quad A which is opaque and extends from (5, 5) to (10, 10) and quad B which is behind quad A and extends from (4.1, 4.1) to (10.9, 10.9). this logic would calculate (5, 5, 10, 10) for quad B's transformedQuadRect, since it uses enclosedIntRect(), and keepQuad would be set to false and we wouldn't draw it, even though we should!. keep in mind that since this is all per render surface the whole thing could be greatly scaled up later on and produce very visible errors. i think that in order to cull correctly and only deal with integer quads/rects, we need to be conservative. that means to decide if a given quad is culled inflate to integer boundaries with enclosingIntRect() and then when marking a quad as opaque for further culling deflate with enclosedIntRect(). even if my analysis is wrong, please add some test coverage for this. > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:41 > +#define MakeTileQuad(s, r) \ > + CCTileDrawQuad::create((s), (r), 1, IntPoint(1, 1), IntSize(100, 100), 0, false, false, false, false, false) why isn't this a function? we very rarely use macros for things like this, and if we did it would be NAMED_LIKE_THIS()
Created attachment 123978 [details] Patch
(In reply to comment #28) > (From update of attachment 123729 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123729&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCDrawQuad.h:62 > > + m_quadVisibleRect = quadVisibleRect; > > + m_quadVisibleRect.intersect(m_quadRect); > > this sort of logic belongs in the .cpp, and the behavior should be documented in the header since this isn't a simple setter Done. > > Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:107 > > + IntRect transformedQuadRect(enclosedIntRect(floatTransformedRect)); > > + > > + IntRect transformedVisibleQuadRect = rectSubtractRegion(opaqueCoverageThusFar, transformedQuadRect); > > + bool keepQuad = !transformedVisibleQuadRect.isEmpty(); > > this looks wrong. consider quad A which is opaque and extends from (5, 5) to (10, 10) and quad B which is behind quad A and extends from (4.1, 4.1) to (10.9, 10.9). this logic would calculate (5, 5, 10, 10) for quad B's transformedQuadRect, since it uses enclosedIntRect(), and keepQuad would be set to false and we wouldn't draw it, even though we should!. keep in mind that since this is all per render surface the whole thing could be greatly scaled up later on and produce very visible errors. > > i think that in order to cull correctly and only deal with integer quads/rects, we need to be conservative. that means to decide if a given quad is culled inflate to integer boundaries with enclosingIntRect() and then when marking a quad as opaque for further culling deflate with enclosedIntRect(). > > even if my analysis is wrong, please add some test coverage for this. Your analysis is correct. Fixed, and two new tests added. > > Source/WebKit/chromium/tests/CCQuadCullerTest.cpp:41 > > +#define MakeTileQuad(s, r) \ > > + CCTileDrawQuad::create((s), (r), 1, IntPoint(1, 1), IntSize(100, 100), 0, false, false, false, false, false) > > why isn't this a function? we very rarely use macros for things like this, and if we did it would be NAMED_LIKE_THIS() Fixed.
Comment on attachment 123978 [details] Patch R=me, thanks for the additional tests.
Comment on attachment 123978 [details] Patch Clearing flags on attachment: 123978 Committed r106076: <http://trac.webkit.org/changeset/106076>
All reviewed patches have been landed. Closing bug.