Bug 91537 – [Chromium] Textures drawn during occlusion are incorrectly re-used when unoccluded.

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

Textures drawn during occlusion are incorrectly re-used when unoccluded.

Created attachment 152819 [details]
Patch

[Dana Jansens]

Comment on attachment 152819 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:630
> +    contentsTexture->setIsComplete(!quad->hasExternalOcclusion());

I think this should go in drawRenderPass, since that is where the texture is actually drawn to. So the flag should be in CCRenderPass.

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.h:47
> +    bool hasExternalOcclusion() { return m_hasExternalOcclusion; }

I think this "hasExternalOcclusion" name used throughout could be more descriptive. It makes lots of sense in the context of this diff, but if another person happened upon it, it would not be clear at all.

hasOcclusionFromOutsideTargetSurface.. hasOcclusionFromNonSibling.. I'm not sure exactly what would be right.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:60
> +    CCQuadCuller quadCuller(m_quadList, layer, occlusionTracker, layer->hasDebugBorders(), false);

please use a temporary variable instead of passing a boolean value directly.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:68
> +    if (!m_targetSurface->hasExternalOcclusion())
> +        m_targetSurface->setHasExternalOcclusion(quadCuller.hasExternalOcclusion());

Can we just set it on the render pass here instead?

> Source/WebCore/platform/graphics/chromium/cc/CCRenderPass.cpp:75
> +    CCQuadCuller quadCuller(m_quadList, layer, occlusionTracker, layer->hasDebugBorders(), true);

please use a temporary variable instead of passing a boolean value directly.

> Source/WebCore/platform/graphics/chromium/cc/CCRenderSurface.h:164
> +    bool m_hasExternalOcclusion;

I think belongs on RenderPass

Created attachment 153010 [details]
Patch

[Adrienne Walker]

Comment on attachment 153010 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Textures drawn during occlusion are incorrectly re-used when unoccluded.

[chromium]

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:148
> +        : CCScopedTexture(allocator),
> +          m_isComplete(false)

style nit: Comma vertically aligned with colon.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:445
> +    if (hasOcclusionFromOutsideTargetSurface)
> +        *hasOcclusionFromOutsideTargetSurface = (unoccludedInScreen != unoccludedInTarget);

I think you should be checking the occlusion on m_stack[lastIndex - 1].occlusionInScreen, which is the external occlusion for this surface prior to considering anything in the surface.  Any layers that draw into this target surface will update m_stack.last().occlusionInScreen as they go (if the transform to the screen is known), which muddles internal and external occlusion.

> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:56
> +    , m_forSurface(forSurface)

I feel like this just shifts complexity around, but doesn't result in any net simplification.  On the other hand, CCQuadCuller is so tied to the occlusion iteration that I can't come up with any example for when you'd ever want to call append and appendSurface on the same quad culler.  Eh.

[Dana Jansens]

Comment on attachment 153010 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCQuadCuller.cpp:56
>> +    , m_forSurface(forSurface)
> 
> I feel like this just shifts complexity around, but doesn't result in any net simplification.  On the other hand, CCQuadCuller is so tied to the occlusion iteration that I can't come up with any example for when you'd ever want to call append and appendSurface on the same quad culler.  Eh.

Ya you never will since its a temporary thing for the scope for the CCRenderPass::append___() methods.

(In reply to comment #4)
> > Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:445
> > +    if (hasOcclusionFromOutsideTargetSurface)
> > +        *hasOcclusionFromOutsideTargetSurface = (unoccludedInScreen != unoccludedInTarget);
> 
> I think you should be checking the occlusion on m_stack[lastIndex - 1].occlusionInScreen, which is the external occlusion for this surface prior to considering anything in the surface.  Any layers that draw into this target surface will update m_stack.last().occlusionInScreen as they go (if the transform to the screen is known), which muddles internal and external occlusion.

I think what you're proposing is essentially checking (external != 0), "external" being the external occlusion for the layer - right? What (I think) I'm currently checking is (external + internal != internal). I'd say the latter is a little bit more precise, even though with boolean flag the result will be the same.

If layer A occludes B partially in the surface, and both of them are occluded by layer E outside (but B's occlusion by E is smaller than B's occlusion by A), I would only get a positive result on A, whereas you would get it on both. With boolean flag I cannot see where this would fail; if we move to a rect instead, there may be benefit in only checking A, though I cannot think of an example right now.

Also I already have both rects computed anyhow, so it's simpler to implement.

Created attachment 153281 [details]
Patch

Added handling of early out in CCLTHi plus more unit tests

Created attachment 153294 [details]
Patch

Rebased

[Adrienne Walker]

Thanks for all of the explanation.  That makes a lot of sense.  :)

Can you also convince me that != is the correct operation here rather than  intersection(targetUnoccluded, screenUnoccluded) == targetUnoccluded?

The case I'm thinking of is a surface that's rotated some non-zero amount.  It has no external occlusion, and it has internal occlusion that covers exactly half of it.  Now you're considering a layer that takes up the whole bounds of the surface.  There's no screen space occlusion because the layer is not rectilinear in screen space.  There's target space occlusion of half the layer.  However, since there's no external occlusion, you'd want to set hasOcclusionFromOutsideTargetSurface to false, but a simple inequality would return true.

(In reply to comment #9)
> Thanks for all of the explanation.  That makes a lot of sense.  :)
> 
> Can you also convince me that != is the correct operation here rather than  intersection(targetUnoccluded, screenUnoccluded) == targetUnoccluded?
> 
> The case I'm thinking of is a surface that's rotated some non-zero amount.  It has no external occlusion, and it has internal occlusion that covers exactly half of it.  Now you're considering a layer that takes up the whole bounds of the surface.  There's no screen space occlusion because the layer is not rectilinear in screen space.  There's target space occlusion of half the layer.  However, since there's no external occlusion, you'd want to set hasOcclusionFromOutsideTargetSurface to false, but a simple inequality would return true.

The assumption that I had is that targetUnoccluded will always be bigger than screenUnoccluded and include it entirely. If that assumption is correct, the two conditions are equivalent, so checking for their intersection is equivalent to checking for their equality.

In your example, wouldn't we have a very small screenUnoccluded resulting from bounding rect of target occlusion?

[Adrienne Walker]

(In reply to comment #10)

> The assumption that I had is that targetUnoccluded will always be bigger than screenUnoccluded and include it entirely. If that assumption is correct, the two conditions are equivalent, so checking for their intersection is equivalent to checking for their equality.

Yeah, I'm not sure you can make that assumption.
 
> In your example, wouldn't we have a very small screenUnoccluded resulting from bounding rect of target occlusion?

Right now, no.  In CCOcclusionTrackerBase<_,_>::markOccludedBehindLayer (line 342), it early outs if the layer is not rectilinear in screen space before marking any screen space occlusion.  Even if that FIXME was addressed and the code did find the interior rect inside that rotated occlusion rect and marked that, then I think screenUnoccluded could still be bigger than targetUnoccluded.

[WebKit Review Bot]

Comment on attachment 153294 [details]
Patch

Attachment 153294 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13277934

(In reply to comment #11)
> (In reply to comment #10)
> 
> > The assumption that I had is that targetUnoccluded will always be bigger than screenUnoccluded and include it entirely. If that assumption is correct, the two conditions are equivalent, so checking for their intersection is equivalent to checking for their equality.
> 
> Yeah, I'm not sure you can make that assumption.
> 
> > In your example, wouldn't we have a very small screenUnoccluded resulting from bounding rect of target occlusion?
> 
> Right now, no.  In CCOcclusionTrackerBase<_,_>::markOccludedBehindLayer (line 342), it early outs if the layer is not rectilinear in screen space before marking any screen space occlusion.  Even if that FIXME was addressed and the code did find the interior rect inside that rotated occlusion rect and marked that, then I think screenUnoccluded could still be bigger than targetUnoccluded.

You are right, and I verified this with a unit test. I'll switch this to checking the intersection.

Created attachment 153329 [details]
Patch

Changed condition in CCOcclusionTracker

[Adrienne Walker]

Comment on attachment 153329 [details]
Patch

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

R=me.  The tests are great.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:455
> +    if (hasOcclusionFromOutsideTargetSurface)
> +        *hasOcclusionFromOutsideTargetSurface = (intersection(unoccludedInScreen, unoccludedInTarget) != unoccludedInTarget);
> +
> +    if (unoccludedInScreen.isEmpty())
> +        return unoccludedInScreen;
> +
>      return intersection(unoccludedInScreen, unoccludedInTarget);

I don't think you need this second if statement.

> Source/WebCore/platform/graphics/chromium/cc/CCOcclusionTracker.cpp:512
> +    if (unoccludedInScreen.isEmpty())
> +        return unoccludedInScreen;
> +

Nor this.

[WebKit Review Bot]

Comment on attachment 153329 [details]
Patch

Attachment 153329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13308024

Created attachment 153341 [details]
Patch

Removed redundant if, added missing CCQuadSink

Created attachment 153342 [details]
Patch

Re-upload with reviewer info

[WebKit Review Bot]

Comment on attachment 153342 [details]
Patch

Clearing flags on attachment: 153342

Committed r123154: <http://trac.webkit.org/changeset/123154>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.