Bug 91537 - [Chromium] Textures drawn during occlusion are incorrectly re-used when unoccluded.
Summary: [Chromium] Textures drawn during occlusion are incorrectly re-used when unocc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-17 13:42 PDT by zlieber
Modified: 2012-07-19 15:35 PDT (History)
7 users (show)

See Also:


Attachments
Patch (71.92 KB, patch)
2012-07-17 13:54 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (65.78 KB, patch)
2012-07-18 07:29 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (78.59 KB, patch)
2012-07-19 09:50 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (77.06 KB, patch)
2012-07-19 10:24 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (80.26 KB, patch)
2012-07-19 13:00 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (82.18 KB, patch)
2012-07-19 14:00 PDT, zlieber
no flags Details | Formatted Diff | Diff
Patch (82.22 KB, patch)
2012-07-19 14:02 PDT, zlieber
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zlieber 2012-07-17 13:42:24 PDT
Textures drawn during occlusion are incorrectly re-used when unoccluded.
Comment 1 zlieber 2012-07-17 13:54:55 PDT
Created attachment 152819 [details]
Patch
Comment 2 Dana Jansens 2012-07-17 14:22:08 PDT
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
Comment 3 zlieber 2012-07-18 07:29:58 PDT
Created attachment 153010 [details]
Patch
Comment 4 Adrienne Walker 2012-07-18 13:41:56 PDT
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.
Comment 5 Dana Jansens 2012-07-18 18:19:48 PDT
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.
Comment 6 zlieber 2012-07-19 08:18:21 PDT
(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.
Comment 7 zlieber 2012-07-19 09:50:27 PDT
Created attachment 153281 [details]
Patch

Added handling of early out in CCLTHi plus more unit tests
Comment 8 zlieber 2012-07-19 10:24:10 PDT
Created attachment 153294 [details]
Patch

Rebased
Comment 9 Adrienne Walker 2012-07-19 10:31:44 PDT
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.
Comment 10 zlieber 2012-07-19 10:49:02 PDT
(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?
Comment 11 Adrienne Walker 2012-07-19 10:53:51 PDT
(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.
Comment 12 WebKit Review Bot 2012-07-19 11:32:30 PDT
Comment on attachment 153294 [details]
Patch

Attachment 153294 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13277934
Comment 13 zlieber 2012-07-19 12:00:19 PDT
(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.
Comment 14 zlieber 2012-07-19 13:00:53 PDT
Created attachment 153329 [details]
Patch

Changed condition in CCOcclusionTracker
Comment 15 Adrienne Walker 2012-07-19 13:19:28 PDT
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.
Comment 16 WebKit Review Bot 2012-07-19 13:25:01 PDT
Comment on attachment 153329 [details]
Patch

Attachment 153329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13308024
Comment 17 zlieber 2012-07-19 14:00:46 PDT
Created attachment 153341 [details]
Patch

Removed redundant if, added missing CCQuadSink
Comment 18 zlieber 2012-07-19 14:02:51 PDT
Created attachment 153342 [details]
Patch

Re-upload with reviewer info
Comment 19 WebKit Review Bot 2012-07-19 15:34:54 PDT
Comment on attachment 153342 [details]
Patch

Clearing flags on attachment: 153342

Committed r123154: <http://trac.webkit.org/changeset/123154>
Comment 20 WebKit Review Bot 2012-07-19 15:35:00 PDT
All reviewed patches have been landed.  Closing bug.