Bug 78549 – [Chromium] New CCOcclusionTracker class with tests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[Chromium] New CCOcclusionTracker class with tests

[Dana Jansens]

Created attachment 126861 [details]
Patch

Create the CCOcclusionTracker object, along with unit tests to verify its functionality.

The object encapsulates the logic of CCLayerTreeHost's paint culling, and CCQuadCuller's draw culling together in an object that can be used for both scenarios.

It tracks occlusion within the target surface, and within screen space. You can do a simple occluded() true/false test, or give it a rect in content space, and it
will return a sub-rect that contains all the unoccluded parts (in both the target surface and screen space) of the original rect.

[Dana Jansens]

Comment on attachment 126861 [details]
Patch

For @wjmaclean review

[W. James MacLean]

Overall comments: 

Generally, looks good.

I've heard some murmurings that "RenderSurfaces" could be on the road to extinction. Best to find out if that's true, and how (if at all) it will affect this CL.

It seems expensive computing both both "Screen-then-Target-surface, or Target-surface-then-Screen" in unoccluded{Surface}ContentRect. Is there no way to avoid this and predict the correct (conservative) occlusion in advance?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h
>
> 43 template<typename RenderSurfaceType> void enterTargetRenderSurface(RenderSurfaceType* newTarget);
> 44    template<typename RenderSurfaceType> void leaveToTargetRenderSurface(RenderSurfaceType* newTarget);

Seems awkward ... seems like they're the same, just the second assumes we're already in a target surface? Or are the names just misleading?

> 51 template<typename LayerType> bool occluded(LayerType*, const IntRect& contentRect) const;
> 52    // Gives an unoccluded sub-rect of |contentRect| in the content space of the layer. Used when considering occlusion for a layer that paints/draws something.

Comment doesn't seem to match.

> 65    FloatQuad transformedBoundsQuad = transform.mapQuad(FloatQuad(centeredBounds));
> 66    if (!transformedBoundsQuad.isRectilinear())
> 67        return Region();

Add comment specifying what transform types will lead to rectilinear bounds.

> 118    // Scale from content space to layer space
> 119    transform.scaleNonUniform(boundsInLayerSpace.width() / static_cast<double>(boundsInContentSpace.width()),
> 120                              boundsInLayerSpace.height() / static_cast<double>(boundsInContentSpace.height()));

Can this potentially fail for non-trivial transforms? E.g. rotation? Or is the transform from content to layer space always assumed to be translation + scale?I.e., how do we know when this is safe or is it always? Ditto for similar computations below.

Nice occlusion diagrams :-)

[Dana Jansens]

(In reply to comment #3)
> Overall comments: 
> 
> Generally, looks good.

Thanks!

> I've heard some murmurings that "RenderSurfaces" could be on the road to extinction. Best to find out if that's true, and how (if at all) it will affect this CL.

They are getting layers so that we don't have all this layer-like state on a RenderSurface. In this case, it will change the loops which call CCOcclusion tracker, and clean them up some hopefully. The CCOcclusionTracker should be minimally affected, mostly in the transform calculation functions.

> It seems expensive computing both both "Screen-then-Target-surface, or Target-surface-then-Screen" in unoccluded{Surface}ContentRect. Is there no way to avoid this and predict the correct (conservative) occlusion in advance?

Described it to @vollick briefly and he helped me see I can return the Rect for screen and target, and just take the intersection back in content space. This passes the unit tests for these functions that are somewhat particular, so it will be accurate enough. Thanks.

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h
> >
> > 43 template<typename RenderSurfaceType> void enterTargetRenderSurface(RenderSurfaceType* newTarget);
> > 44    template<typename RenderSurfaceType> void leaveToTargetRenderSurface(RenderSurfaceType* newTarget);
> 
> Seems awkward ... seems like they're the same, just the second assumes we're already in a target surface? Or are the names just misleading?

The first is done when we see a new layer. It says "hey we're in this target surface (if we weren't already)". The second is done when we see a contributing layer. It says "hey we were in this target surface but now we're done with it, and moving up to its parent". It's a merge-up operation. I'll add comments.

> > 51 template<typename LayerType> bool occluded(LayerType*, const IntRect& contentRect) const;
> > 52    // Gives an unoccluded sub-rect of |contentRect| in the content space of the layer. Used when considering occlusion for a layer that paints/draws something.
> 
> Comment doesn't seem to match.

The comment matches the function below it. I'll add one for occluded() too.

> > 65    FloatQuad transformedBoundsQuad = transform.mapQuad(FloatQuad(centeredBounds));
> > 66    if (!transformedBoundsQuad.isRectilinear())
> > 67        return Region();
> 
> Add comment specifying what transform types will lead to rectilinear bounds.

done.

> > 118    // Scale from content space to layer space
> > 119    transform.scaleNonUniform(boundsInLayerSpace.width() / static_cast<double>(boundsInContentSpace.width()),
> > 120                              boundsInLayerSpace.height() / static_cast<double>(boundsInContentSpace.height()));
> 
> Can this potentially fail for non-trivial transforms? E.g. rotation? Or is the transform from content to layer space always assumed to be translation + scale?I.e., how do we know when this is safe or is it always? Ditto for similar computations below.

content<->layer space is only a scale.

> Nice occlusion diagrams :-)

[Dana Jansens]

Created attachment 127055 [details]
Patch

For @enne review.

[Dana Jansens]

Created attachment 127505 [details]
Patch

- make pointers const all over
- add the CSS filter stuff that was added to CCLayerTreeHost, and unit test for it!
- add finishedTargetRenderSurface() to do cleanup/processing for the surface once all layers have been visited in it.
- add surfaceOccluded() which tells if part of the surface is occluded (by things outside the surface), and unit tests for this!

and..
- add currentOcclusionInScreenSpace() and currentOcclusionInTargetSpace() temporarily to make transition CLs easier.

[Adrienne Walker]

Comment on attachment 127505 [details]
Patch

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

So, this is just a patch to land independent functionality and then you'll land another patch to transition to switch over to using this?

Yay, for lots of tests.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:210
> +    if (!transformedQuad.isRectilinear())
> +        return false;
> +    return occlusion.contains(transformedQuad.enclosingBoundingBox());

Why the conditional? Even if not rectilinear, if the occlusion still contains the enclosing bounding box, then this contentRect is still occluded, yeah?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:275
> +    // The use of enclosingIntRect, with floating point rounding, can give us a result that is not a sub-rect of contentRect. Shrink the result
> +    // down so that contentRect contains the result we return.
> +    int x = max(unoccludedRect.x(), contentRect.x());
> +    int y = max(unoccludedRect.y(), contentRect.y());
> +    int maxX = min(unoccludedRect.maxX(), contentRect.maxX());
> +    int maxY = min(unoccludedRect.maxY(), contentRect.maxY());
> +    return IntRect(x, y, maxX - x, maxY - y);

Why not just unoccludedRect.intersect(contentRect)?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:297
> +IntRect CCOcclusionTrackerBase::surfaceUnoccludedContentRect(const LayerType* layer, const IntRect& surfaceContentRect) const

Where is this function going to get used? The other functions seem like a mirror of existing functionality in paint culling, but this and surfaceOccluded seems new and I'm not sure where they fit in the culling algorithm.

Also, can you write a test for this function that does more than take in a rect and spit the same rect back out?

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:80
> +    typedef void RenderSurfaceVoidType;
> +
> +    struct StackObject {
> +        const RenderSurfaceVoidType* surface;

Along with the fact that a pair of types gets used consistently across all these functions, this piece of code says to me really says that your class should be templated.  Maybe I'm missing something, but I don't see a need for void pointer shenanigans.

Also, if the class were templated, you could just typedef CCOcclusionTracker(Impl) rather than adding boilerplate functions that thunk through.

[Adrienne Walker]

Comment on attachment 127505 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:297
>> +IntRect CCOcclusionTrackerBase::surfaceUnoccludedContentRect(const LayerType* layer, const IntRect& surfaceContentRect) const
> 
> Where is this function going to get used? The other functions seem like a mirror of existing functionality in paint culling, but this and surfaceOccluded seems new and I'm not sure where they fit in the culling algorithm.
> 
> Also, can you write a test for this function that does more than take in a rect and spit the same rect back out?

Oops, line 870 does have a such a test.  My bad.

[Dana Jansens]

(In reply to comment #7)
> (From update of attachment 127505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127505&action=review
> 
> So, this is just a patch to land independent functionality and then you'll land another patch to transition to switch over to using this?

Precisely. I'll make sure it's at feature-parity with any changes 'upstream' as I use it to replace existing code, and verify that any unit tests removed have the same cases covered for this class as I go.

> Yay, for lots of tests.  :)
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:210
> > +    if (!transformedQuad.isRectilinear())
> > +        return false;
> > +    return occlusion.contains(transformedQuad.enclosingBoundingBox());
> 
> Why the conditional? Even if not rectilinear, if the occlusion still contains the enclosing bounding box, then this contentRect is still occluded, yeah?

Yep! Thanks.

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:275
> > +    // The use of enclosingIntRect, with floating point rounding, can give us a result that is not a sub-rect of contentRect. Shrink the result
> > +    // down so that contentRect contains the result we return.
> > +    int x = max(unoccludedRect.x(), contentRect.x());
> > +    int y = max(unoccludedRect.y(), contentRect.y());
> > +    int maxX = min(unoccludedRect.maxX(), contentRect.maxX());
> > +    int maxY = min(unoccludedRect.maxY(), contentRect.maxY());
> > +    return IntRect(x, y, maxX - x, maxY - y);
> 
> Why not just unoccludedRect.intersect(contentRect)?

Oh, duh.. thank you :)

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:297
> > +IntRect CCOcclusionTrackerBase::surfaceUnoccludedContentRect(const LayerType* layer, const IntRect& surfaceContentRect) const
> 
> Where is this function going to get used? The other functions seem like a mirror of existing functionality in paint culling, but this and surfaceOccluded seems new and I'm not sure where they fit in the culling algorithm.

I am planning to use it for culling of the surface itself on draw side. Right now we don't cull surface quads at all, but we can cull them based on things outside the surface in the future.

The bool surfaceOccluded() method probably won't be used at the moment, but I included it just for completeness.

> Also, can you write a test for this function that does more than take in a rect and spit the same rect back out?

As we discussed, CCOcclusionTrackerTest.surfaceOcclusionInScreenSpace is meant to verify that this function works as intended. That said, if you can think of something more to test I'm very happy to!

> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:80
> > +    typedef void RenderSurfaceVoidType;
> > +
> > +    struct StackObject {
> > +        const RenderSurfaceVoidType* surface;
> 
> Along with the fact that a pair of types gets used consistently across all these functions, this piece of code says to me really says that your class should be templated.  Maybe I'm missing something, but I don't see a need for void pointer shenanigans.
> 
> Also, if the class were templated, you could just typedef CCOcclusionTracker(Impl) rather than adding boilerplate functions that thunk through.

As per offline discussion, going with templates and typedefs. It really cleaned up the .h so yay.

[Dana Jansens]

Created attachment 128120 [details]
Patch

For @enne review. Comments addressed!

[Adrienne Walker]

Comment on attachment 128120 [details]
Patch

Thanks for those changes! I don't think I have anything else to add.  :)

[Dana Jansens]

Comment on attachment 128120 [details]
Patch

Thanks enne! For @jamesr review then.

[James Robinson]

Comment on attachment 128120 [details]
Patch

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

Works for me! R=me with one style nit, feel free to address it when you address the other FIXME in that header.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.h:94
> +    WTF_MAKE_NONCOPYABLE(CCOcclusionTrackerBase);

style nit: normally we put this right below the class declaration and above the first public:

[WebKit Review Bot]

Comment on attachment 128120 [details]
Patch

Clearing flags on attachment: 128120

Committed r108453: <http://trac.webkit.org/changeset/108453>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

Hi,

Chromium's webkit_unit_tests started to fail since r108453:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Linux%20%28dbg%29/builds/3657/steps/webkit_unit_tests/logs/stdio

[ RUN      ] CCOcclusionTrackerTest.surfaceOcclusionWithOverlappingSiblingSurfaces
ASSERTION FAILED: layer->targetRenderSurface() == m_stack.last().surface
third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp(217) : bool WebCore::CCOcclusionTrackerBase<LayerType, RenderSurfaceType>::occluded(const LayerType*, const WebCore::IntRect&) const [with LayerType = WebCore::LayerChromium, RenderSurfaceType = WebCore::RenderSurfaceChromium]
1   0x1140ffc
2   0x546edc
3   0x791cc1
4   0x78e99e
5   0x7833a2
6   0x783bc6
7   0x7842bc
8   0x789093
9   0x792ebb
10  0x78f361
11  0x787bb0
12  0x7a31d5
13  0x41fc7d
14  0x7f63e50c2c4d __libc_start_main
15  0x41fb79
[56389:56389:0222/011121:15415134960585:ERROR:process_util_posix.cc(142)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x1f69a3e]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x1f1a9a5]
	0x7f63e50d7af0
	WebCore::CCOcclusionTrackerBase<>::occluded() [0x1141006]
	(anonymous namespace)::CCOcclusionTrackerTest_surfaceOcclusionWithOverlappingSiblingSurfaces_Test::TestBody() [0x546edc]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x791cc1]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x78e99e]
	testing::Test::Run() [0x7833a2]
	testing::TestInfo::Run() [0x783bc6]
	testing::TestCase::Run() [0x7842bc]
	testing::internal::UnitTestImpl::RunAllTests() [0x789093]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x792ebb]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x78f361]
	testing::UnitTest::Run() [0x787bb0]
	base::TestSuite::Run() [0x7a31d5]
	main [0x41fc7d]
	0x7f63e50c2c4d
	0x41fb79

Furthermore, one layout test started to fail and I'm suspecting this could be also due to r108453 (not 100% sure though):
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fcanvas%2Fwebgl%2Ftex-image-with-format-and-type.html

Could you take a look and fix them ASAP? I'm going to roll out the change unless I hear back from you.

[Yuta Kitamura]

I'm rolling out the change. Please fix the issue and land again.

[Yuta Kitamura]

Reverted r108453 for reason:

Broke Chromium's webkit_unit_tests.

Committed r108467: <http://trac.webkit.org/changeset/108467>

[Yuta Kitamura]

(In reply to comment #16)
> Furthermore, one layout test started to fail and I'm suspecting this could be also due to r108453 (not 100% sure though):

This wasn't true, because the test keeps failing even after the rollout.

[Dana Jansens]

Comment on attachment 128120 [details]
Patch

Ok, going to resubmit then.

[W. James MacLean]

Created attachment 128213 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128213 [details]
Patch for landing

Rejecting attachment 128213 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11560504

[W. James MacLean]

Created attachment 128236 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128236 [details]
Patch for landing

Clearing flags on attachment: 128236

Committed r108521: <http://trac.webkit.org/changeset/108521>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yuta Kitamura]

(In reply to comment #20)
> (From update of attachment 128120 [details])
> Ok, going to resubmit then.

I'm sorry I wasn't clear, but I meant only layout test failure wasn't your fault. Your change still causes the following assertion failure.

ASSERTION FAILED: layer->targetRenderSurface() == m_stack.last().surface

Please investigate further as soon as possible. This error causes webkit_unit_tests to abort in the middle of the run, which is undesirable.

[Yuta Kitamura]

Um, you seem to fix the bug in r108587 (bug 79275).

Please leave some comments to show the linkage of bugs, and notify WebKit gardeners about build fixes from next time. Thanks.

[Dana Jansens]

Yup. Sorry! Enne knew about it at least, but I should have linked them.