Bug 85829 – [chromium] Don't draw when canDraw() is false

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

[Dana Jansens]

[chromium] Don't draw in single threaded compositor when canDraw() is false

[Dana Jansens]

Created attachment 140603 [details]
Patch

[James Robinson]

What crashes is this intended to fix?  How are we getting into this state currently?

[Dana Jansens]

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?

[Dana Jansens]

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.

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 140612 [details]
Patch

rebased, and cleanup of CCLTHImplTests common setup code.

[James Robinson]

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?

[Dana Jansens]

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.

[James Robinson]

When did single-threaded LTHTests regress?  Can we revert that?

[Dana Jansens]

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.

[Adrienne Walker]

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?

[Dana Jansens]

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.

[Adrienne Walker]

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.

[Dana Jansens]

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.

[Dana Jansens]

Created attachment 141053 [details]
Patch

[Adrienne Walker]

Comment on attachment 141053 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 141053 [details]
Patch

Clearing flags on attachment: 141053

Committed r116587: <http://trac.webkit.org/changeset/116587>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.