WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
85829
[chromium] Don't draw when canDraw() is false
https://bugs.webkit.org/show_bug.cgi?id=85829
Summary
[chromium] Don't draw when canDraw() is false
Dana Jansens
Reported
2012-05-07 14:21:59 PDT
[chromium] Don't draw in single threaded compositor when canDraw() is false
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
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Dana Jansens
Comment 1
2012-05-07 15:22:11 PDT
Created
attachment 140603
[details]
Patch
James Robinson
Comment 2
2012-05-07 15:24:31 PDT
What crashes is this intended to fix? How are we getting into this state currently?
Dana Jansens
Comment 3
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?
Dana Jansens
Comment 4
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.
Dana Jansens
Comment 5
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.
Dana Jansens
Comment 6
2012-05-07 15:56:30 PDT
Created
attachment 140612
[details]
Patch rebased, and cleanup of CCLTHImplTests common setup code.
James Robinson
Comment 7
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?
Dana Jansens
Comment 8
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.
James Robinson
Comment 9
2012-05-08 14:44:26 PDT
When did single-threaded LTHTests regress? Can we revert that?
Dana Jansens
Comment 10
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.
Adrienne Walker
Comment 11
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?
Dana Jansens
Comment 12
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.
Adrienne Walker
Comment 13
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.
Dana Jansens
Comment 14
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.
Dana Jansens
Comment 15
2012-05-09 16:55:21 PDT
Created
attachment 141053
[details]
Patch
Adrienne Walker
Comment 16
2012-05-09 17:20:21 PDT
Comment on
attachment 141053
[details]
Patch R=me.
WebKit Review Bot
Comment 17
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
>
WebKit Review Bot
Comment 18
2012-05-09 17:40:49 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug