Fixes a bug when the framebuffer is cleared with invalid scissor rect

Fixes a bug when the framebuffer is cleared with invalid scissor rect.

BUG=170305
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179498

[jamesr, 2013-01-24 20:59:24]

Can you add or extend the tests in cc/gl_renderer_pixeltest.cc to cover this? 
There's a very simple pixel test for the oversize render surface caching code in
there currently.

[danakj, 2013-01-24 20:59:40]

Why does it need to be cleared? Once that part is meant to be used, won't it be
cleared then?

[piman, 2013-01-24 21:10:40]

How does that not break partial updates?

[slavi, 2013-01-24 21:40:24]

On 2013/01/24 20:59:40, danakj wrote:
> Why does it need to be cleared? Once that part is meant to be used, won't it
be
> cleared then?

The patch is still not quite correct and I'm working on it. See the bug for more
info of the breakage. I can send you a test case from the bug to see how this
fixes the issue. Unfortunately, you're right, it's not as simple as that.

[slavi, 2013-01-24 23:16:34]

I think I finally got a proper fix. I'll try to add a test to our unit tests to
catch such bugs in the future.

Dana, James, does that look reasonable?

[danakj, 2013-01-24 23:23:18]

This seems like it would break partial swaps still.

You make a surface 50x50, gets a texture 50x50.
You shrink the surface to 40x50, the texture stays 50x50.
You damage the surface 10x10.
It clears 50x50.

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc#newcode148
cc/direct_renderer.cc:148: texture->setNeedsFullClear(true);
Doesn't this clear the whole texture every frame once your surface shrinks down
by 1px?

[jamesr, 2013-01-24 23:28:00]

The other thing that's a little weird about this is why are we sampling outside
of the render surface bounds?  I.e. if I start with a 50x50 surface, then later
need a 20x20 surface, when we go to render it why does it matter what the pixel
values outside the rect (0,0) -> (20,20) are?

[slavi, 2013-01-24 23:37:38]

On 2013/01/24 23:23:18, danakj wrote:
> This seems like it would break partial swaps still.
> 
> You make a surface 50x50, gets a texture 50x50.
> You shrink the surface to 40x50, the texture stays 50x50.
> You damage the surface 10x10.
> It clears 50x50.

You must be mistaken somewhere. My patch reverts to the behavior direct_renderer
had before James' patch for larger-than-necessary textures
https://chromiumcodereview.appspot.com/11420079/.

Before, if the texture size has changed, we just allocate a new *cleared*
texture with the new size.
After, James' patch, if the texture size has changed, but it's smaller than what
we need, we keep the old big texture (*uncleared*). With this patch, I just
clear the big texture, as if we're in the 'Before' case and the texture is newly
allocated.

What is more, this patch passes *all* the layout tests, so it must be correct.
Or at least not entirely incorrect.

> https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc
> File cc/direct_renderer.cc (right):
> 
>
https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc#newcode148
> cc/direct_renderer.cc:148: texture->setNeedsFullClear(true);
> Doesn't this clear the whole texture every frame once your surface shrinks
down
> by 1px?

Yes. That's what we used to do before James's patch by reallocating the texture
on every frame.

[danakj, 2013-01-24 23:38:04]

I'm unclear how clearing outside the scissor is helping, maybe you can explain?

Anything that we're going to show on screen should be inside the scissor, so I
don't get how something outside the scissor ends up getting drawn.

Is it maybe a case of the renderer thinking the texture is "complete" and
doesn't need to be rendered?

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc#newcode147
cc/direct_renderer.cc:147: else if (texture->size() != requiredSize)
What happens if you texture->setIsComplete(false) here instead?

[danakj, 2013-01-24 23:39:25]

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc
File cc/direct_renderer.cc (right):

https://codereview.chromium.org/12036091/diff/7/cc/direct_renderer.cc#newcode148
cc/direct_renderer.cc:148: texture->setNeedsFullClear(true);
On 2013/01/24 23:23:18, danakj wrote:
> Doesn't this clear the whole texture every frame once your surface shrinks
down
> by 1px?

Before it would have reallocated a new texture of the right size, and not
cleared it every frame.

[danakj, 2013-01-25 00:26:10]

I strongly believe this is the fix you want:

diff --git a/cc/direct_renderer.cc b/cc/direct_renderer.cc
index 31f48e5..f9fb233 100644
--- a/cc/direct_renderer.cc
+++ b/cc/direct_renderer.cc
@@ -240,8 +240,11 @@ void DirectRenderer::drawRenderPass(DrawingFrame& frame,
const RenderPass* rende
         setScissorTestRect(moveScissorToWindowSpace(frame, renderPassScissor));
     }
 
-    if (frame.currentRenderPass != frame.rootRenderPass ||
m_client->shouldClearRootRenderPass())
+    if (frame.currentRenderPass != frame.rootRenderPass ||
m_client->shouldClearRootRenderPass()) {
+        if (!usingScissorAsOptimization)
+            ensureScissorTestDisabled();
         clearFramebuffer(frame);
+    }
 
     const QuadList& quadList = renderPass->quad_list;


Let me explain why.

You have 3 renderpasses, A B R (R is root).

A contains one quad Q1 with is_clipped and some clip rect CR.
B contains one quad Q2 that is larger than Q1 and not opaque.


1) Fill B's texture with garbage.
2) When Q1 is drawn, the scissor is set to CR.
3) When B starts to be drawn and gets cleared, the scissor is still st to CR. So
only CR is cleared.
4) When Q2 is drawn it will be blended with the garbage outside of CR.

A unit test could confirm or deny that the scissor rect is set to CR when B is
cleared, which I really think is the problem.

I confirmed with partial swap no problem exists, but then we are always setting
the scissor before clear.
And with the above diff, the problem is fixed on non-partial swap.

[slavi, 2013-01-25 02:16:47]

On 2013/01/25 00:26:10, danakj wrote:
> I strongly believe this is the fix you want:
> 
> diff --git a/cc/direct_renderer.cc b/cc/direct_renderer.cc
> index 31f48e5..f9fb233 100644
> --- a/cc/direct_renderer.cc
> +++ b/cc/direct_renderer.cc
> @@ -240,8 +240,11 @@ void DirectRenderer::drawRenderPass(DrawingFrame& frame,
> const RenderPass* rende
>          setScissorTestRect(moveScissorToWindowSpace(frame,
renderPassScissor));
>      }
>  
> -    if (frame.currentRenderPass != frame.rootRenderPass ||
> m_client->shouldClearRootRenderPass())
> +    if (frame.currentRenderPass != frame.rootRenderPass ||
> m_client->shouldClearRootRenderPass()) {
> +        if (!usingScissorAsOptimization)
> +            ensureScissorTestDisabled();
>          clearFramebuffer(frame);
> +    }


That indeed works. And is way simpler fix than tracking the used size of
CachedResource that I was doing.

PTAL

[danakj, 2013-01-25 02:19:07]

Ya looks good, can you please add a test to make sure we don't regress this?

[slavi, 2013-01-28 22:55:37]

On 2013/01/25 02:19:07, danakj wrote:
> Ya looks good, can you please add a test to make sure we don't regress this?

Dana, I put a check whether scissor testing is enabled in
FakeRendererGL::clearFramebuffer as we spoke offline.

PTAL

[danakj, 2013-01-29 00:00:46]

https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer.h
File cc/gl_renderer.h (right):

https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer.h#newcode91
cc/gl_renderer.h:91: bool isScissorEnabledForTesting() { return
m_isScissorEnabled; }
const method

https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer_unittest.cc
File cc/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:127: EXPECT_FALSE(isScissorEnabledForTesting());
Do we have any unit tests that would fail this without your changes in
direct_renderer.cc? Which one(s)?

[slavi, 2013-01-29 20:47:29]

On 2013/01/29 00:00:46, danakj wrote:
> https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer.h
> File cc/gl_renderer.h (right):
> 
> https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer.h#newcode91
> cc/gl_renderer.h:91: bool isScissorEnabledForTesting() { return
> m_isScissorEnabled; }
> const method

Done.

> 
> https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer_unittest.cc
> File cc/gl_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/12036091/diff/24004/cc/gl_renderer_unittest.c...
> cc/gl_renderer_unittest.cc:127: EXPECT_FALSE(isScissorEnabledForTesting());
> Do we have any unit tests that would fail this without your changes in
> direct_renderer.cc? Which one(s)?

The last one scissorTestWhenClearing in cc/gl_renderer_unittest.cc fails without
the change.

[danakj, 2013-01-29 21:39:20]

Cool, thanks for the test! It looks good but I don't think you need to add any
ForTesting() hooks into the renderer. You can trigger the bad situation by
mimicing what that webpage was doing to cause bad behaviour. That'd be more
robust/preferrable here I think.

https://codereview.chromium.org/12036091/diff/34004/cc/gl_renderer_unittest.cc
File cc/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/12036091/diff/34004/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:670: renderer.enableScissorForTesting();
Instead of this, I think you could set a clip_rect and is_clipped=true on the
last quad in the child render pass. That would more mimic the real situation.

[slavi, 2013-01-29 22:46:06]

On 2013/01/29 21:39:20, danakj wrote:
> Cool, thanks for the test! It looks good but I don't think you need to add any
> ForTesting() hooks into the renderer. You can trigger the bad situation by
> mimicing what that webpage was doing to cause bad behaviour. That'd be more
> robust/preferrable here I think.
> 
> https://codereview.chromium.org/12036091/diff/34004/cc/gl_renderer_unittest.cc
> File cc/gl_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/12036091/diff/34004/cc/gl_renderer_unittest.c...
> cc/gl_renderer_unittest.cc:670: renderer.enableScissorForTesting();
> Instead of this, I think you could set a clip_rect and is_clipped=true on the
> last quad in the child render pass. That would more mimic the real situation.

Done.

[danakj, 2013-01-29 22:47:42]

LGTM

https://codereview.chromium.org/12036091/diff/34008/cc/gl_renderer_unittest.cc
File cc/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/12036091/diff/34008/cc/gl_renderer_unittest.c...
cc/gl_renderer_unittest.cc:678: addClippedQuad(grandChildPass, viewportRect,
SK_ColorYELLOW);
can use gfx::Rect(25, 25) or something for this quad. The other rects could all
be viewportRect if you like.

[commit-bot: I haz the power, 2013-01-29 22:54:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/12036091/35006

[commit-bot: I haz the power, 2013-01-30 01:32:30]

Change committed as 179498