Bug 85829 - [chromium] Don't draw when canDraw() is false
Summary: [chromium] Don't draw when canDraw() is false
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: Dana Jansens
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-07 14:21 PDT by Dana Jansens
Modified: 2012-05-09 17:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (13.31 KB, patch)
2012-05-07 15:22 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (16.32 KB, patch)
2012-05-07 15:56 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2012-05-09 16:55 PDT, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2012-05-07 14:21:59 PDT
[chromium] Don't draw in single threaded compositor when canDraw() is false
Comment 1 Dana Jansens 2012-05-07 15:22:11 PDT
Created attachment 140603 [details]
Patch
Comment 2 James Robinson 2012-05-07 15:24:31 PDT
What crashes is this intended to fix?  How are we getting into this state currently?
Comment 3 Dana Jansens 2012-05-07 15:26:28 PDT
http://crash.corp.google.com/reportdetail?reportid=31cdd8d5c8c805ad#crashing_thread

The crash is not very clear to me, but it immediately seemed incorrect that SingleThreadedProxy is calling drawLayers() when canDraw() is false.

Maybe that stack trace makes more sense to you?
Comment 4 Dana Jansens 2012-05-07 15:27:46 PDT
Oh, additionally the crash started happening around the time we added the prepareToDraw() step, and there was a bit of churn around the whole drawLayers() code.
Comment 5 Dana Jansens 2012-05-07 15:30:17 PDT
Last but not least.. I should have stuck this in the changelog maybe.

I noticed that prepareToDraw() is checking m_rootLayerImpl != NULL, and that's totally bogus IMO. prepareToDraw() should return false if the frame is not going to be correct (checkerboarding stuff), not be overloaded with checked from canDraw().

This CL removes that check from prepareToDraw() and makes prepareToDraw() always produce a valid (tho maybe not desirable) frame. That is how it was intended to work.

Guarding prepareToDraw() (and then drawLayers() too) on canDraw() is the correct way to use the method.
Comment 6 Dana Jansens 2012-05-07 15:56:30 PDT
Created attachment 140612 [details]
Patch

rebased, and cleanup of CCLTHImplTests common setup code.
Comment 7 James Robinson 2012-05-08 14:32:56 PDT
So we have a crash in single threaded mode, but we don't really know how it's happening and this patch doesn't have any unit tests for the single threaded proxy?  How is this good?
Comment 8 Dana Jansens 2012-05-08 14:39:47 PDT
I agree it's not.

The check in prepareToDraw() was introduced in the revert of Sami's layer scrolling code, and should be removed. The revert of Daniel's code removed the threaded-mode guarding of prepareToDraw/drawLayers, which should be replaced to go with it.

We ignored single thread when doing all the above work previously, and it needs to guard similarly to the threaded mode.

This patch is fixing threaded mode again, which the test addresses. But also making single-thread mode match this time around.

I agree I would like a test for single-thread too, it requires https://bugs.webkit.org/show_bug.cgi?id=85690 to land which enables single-threaded LTHTests.
Comment 9 James Robinson 2012-05-08 14:44:26 PDT
When did single-threaded LTHTests regress?  Can we revert that?
Comment 10 Dana Jansens 2012-05-08 14:50:04 PDT
At least a few months ago, I expect much longer though. The only single-threaded tests we run don't actually use the event loop, they simply run a single function and end the test immediately.
Comment 11 Adrienne Walker 2012-05-09 11:37:18 PDT
Comment on attachment 140612 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:355
>        m_layerTreeHostImpl->animate(monotonicTime, wallClockTime);
> +
> +      if (!m_layerTreeHostImpl->canDraw())
> +          return false;
> +

Is there any reason to animate() if !canDraw()?  Can you early out earlier?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:643
> -    bool drawFrame = m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw;
> +    bool drawFrame = m_layerTreeHostImpl->canDraw() && (m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw);

Just as a sanity check, this canDraw is currently redundant since the scheduler should never call this function if !canDraw()?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:677
> -    ASSERT(drawFrame || (!drawFrame && !forcedDraw));
> +    ASSERT(drawFrame || (!drawFrame && (!forcedDraw || (forcedDraw && !m_layerTreeHostImpl->canDraw()))));

This is an awkward ASSERT and I'm not sure what it's buying us.  Can it be removed?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:64
> +        m_hostImpl->initializeLayerRenderer(createContext(), adoptPtr(new FakeTextureUploader));
> +        m_hostImpl->setViewportSize(IntSize(10, 10));

I like the changes in this file, but they look like they're part of some other patch entirely.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:844
> +// FIXME: Make this run in single threaded mode too.

If there's not one already, can you file a bug for this and put a link here?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:859
> +        // Only the initial draw should bring us here.
> +        EXPECT_TRUE(impl->canDraw());

Can you assert about the source frame number here too?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:867
> +            // Make the viewport empty so the host says it can't draw.
> +            m_layerTreeHost->setViewportSize(IntSize(0, 0));

Is there a place to assert explicitly that canDraw is false?
Comment 12 Dana Jansens 2012-05-09 11:45:08 PDT
Comment on attachment 140612 [details]
Patch

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

thanks for comments

>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:355
>> +
> 
> Is there any reason to animate() if !canDraw()?  Can you early out earlier?

From Ian: This lets animations start, so that javascript attached to the animations can run.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:643
>> +    bool drawFrame = m_layerTreeHostImpl->canDraw() && (m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw);
> 
> Just as a sanity check, this canDraw is currently redundant since the scheduler should never call this function if !canDraw()?

No. Forced draws must come here in order to signal completion and unblock the other thread. They will indicate success=false if they can't draw though.

>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:677
>> +    ASSERT(drawFrame || (!drawFrame && (!forcedDraw || (forcedDraw && !m_layerTreeHostImpl->canDraw()))));
> 
> This is an awkward ASSERT and I'm not sure what it's buying us.  Can it be removed?

What it's attempting to do is check that we either:
1) did draw
2) if not, it's not a forced draw
3) if it is a forced draw, it's because we couldn't draw.

But i guess it's basically asserting on the assignment of drawFrame and duplicating it. Will remove.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:64
>> +        m_hostImpl->setViewportSize(IntSize(10, 10));
> 
> I like the changes in this file, but they look like they're part of some other patch entirely.

canDraw() checks !viewportSize.isEmpty(). So canDraw will be false and suddenly block drawing or cause asserts in prepareToDraw() in a number of tests. So I moved setViewportSize() up here.

Same thing for m_layerRendererInitialized.

Other option is adding these lines to the now-failing tests, but I like this more?

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:844
>> +// FIXME: Make this run in single threaded mode too.
> 
> If there's not one already, can you file a bug for this and put a link here?

k

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:859
>> +        EXPECT_TRUE(impl->canDraw());
> 
> Can you assert about the source frame number here too?

yup sounds right!

>> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:867
>> +            m_layerTreeHost->setViewportSize(IntSize(0, 0));
> 
> Is there a place to assert explicitly that canDraw is false?

Sure we can do that here.
Comment 13 Adrienne Walker 2012-05-09 12:10:23 PDT
Comment on attachment 140612 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:355
>>> +
>> 
>> Is there any reason to animate() if !canDraw()?  Can you early out earlier?
> 
> From Ian: This lets animations start, so that javascript attached to the animations can run.

Oh, I see.  In other words, doComposite() behaves as if it were a forcedDraw.

>>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:643
>>> +    bool drawFrame = m_layerTreeHostImpl->canDraw() && (m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw);
>> 
>> Just as a sanity check, this canDraw is currently redundant since the scheduler should never call this function if !canDraw()?
> 
> No. Forced draws must come here in order to signal completion and unblock the other thread. They will indicate success=false if they can't draw though.

Thanks for the explanation.  I misunderstood how canDraw() was supposed to work.  Can you add some comments about the relationship of canDraw to prepareToDraw and drawLayers as well as canDraw to scheduledActionDrawAndSwap?

>>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:64
>>> +        m_hostImpl->setViewportSize(IntSize(10, 10));
>> 
>> I like the changes in this file, but they look like they're part of some other patch entirely.
> 
> canDraw() checks !viewportSize.isEmpty(). So canDraw will be false and suddenly block drawing or cause asserts in prepareToDraw() in a number of tests. So I moved setViewportSize() up here.
> 
> Same thing for m_layerRendererInitialized.
> 
> Other option is adding these lines to the now-failing tests, but I like this more?

Nah, this approach is ok.
Comment 14 Dana Jansens 2012-05-09 16:37:33 PDT
Comment on attachment 140612 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:643
>>>> +    bool drawFrame = m_layerTreeHostImpl->canDraw() && (m_layerTreeHostImpl->prepareToDraw(frame) || forcedDraw);
>>> 
>>> Just as a sanity check, this canDraw is currently redundant since the scheduler should never call this function if !canDraw()?
>> 
>> No. Forced draws must come here in order to signal completion and unblock the other thread. They will indicate success=false if they can't draw though.
> 
> Thanks for the explanation.  I misunderstood how canDraw() was supposed to work.  Can you add some comments about the relationship of canDraw to prepareToDraw and drawLayers as well as canDraw to scheduledActionDrawAndSwap?

done.
Comment 15 Dana Jansens 2012-05-09 16:55:21 PDT
Created attachment 141053 [details]
Patch
Comment 16 Adrienne Walker 2012-05-09 17:20:21 PDT
Comment on attachment 141053 [details]
Patch

R=me.
Comment 17 WebKit Review Bot 2012-05-09 17:40:44 PDT
Comment on attachment 141053 [details]
Patch

Clearing flags on attachment: 141053

Committed r116587: <http://trac.webkit.org/changeset/116587>
Comment 18 WebKit Review Bot 2012-05-09 17:40:49 PDT
All reviewed patches have been landed.  Closing bug.