Bug 81823

Created attachment 133093 [details] Patch

This patch is still rough around the edges, but combined with https://chromiumcodereview.appspot.com/9699125/ it does fix the underlying issue with thumbnail generation using renderer when tabs finish loading in the background. I am still doing some manual testing and confirming gpu resource usage figures, as well as making sure this patch can land safely before chromium patch.

Comment on attachment 133093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133093&action=review Pretty cool! > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:189 > +class ScopedEnsureFramebufferAllocationImpl : public LayerRendererChromium::ScopedEnsureFramebufferAllocation { why subclass? you can do LRC { private: class Foo; } Then have the entire class actually only declared here... class LRC::Foo { ... } I think > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:211 > + bool framebufferWasInitiallyDiscarded; m_ > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:309 > + if (extensions->supports("GL_CHROMIUM_gpu_memory_manager")) { move to lrc capabilities, test capaibilities > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:316 > + m_isFramebufferDiscarded = false; you set this on :233, do you need to again here? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1163 > + if (extensions->supports("GL_EXT_discard_framebuffer")) { you should move this to a bool on LayerRendererCapabilities that you probe at initialize time rather than hammering extensions > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:75 > + OwnPtr<LayerRendererChromium::ScopedEnsureFramebufferAllocation> sfa(m_layerTreeHostImpl->layerRenderer()->createScopedEnsureFramebufferAllocation()); For these kinds of thigns you dont need the pointer stuff. Look at CCThread.h debugscopedsomethignorother. Copy that style.

Comment on attachment 133093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133093&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:189 >> +class ScopedEnsureFramebufferAllocationImpl : public LayerRendererChromium::ScopedEnsureFramebufferAllocation { > > why subclass? > > you can do > > LRC { > private: > class Foo; > } > > Then have the entire class actually only declared here... > > class LRC::Foo { > ... > } > > I think Tried that (well similar), but since outside clients will be calling the destructor, the class needs to be fully defined. What you describe can be done if the only caller of destructor is inside the cc file where the class is defined, as is the case with PIMPL. I maybe don't need to be so clever with friend impl class if I add more to the public definition, but I wanted to keep as little public as possible. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:309 >> + if (extensions->supports("GL_CHROMIUM_gpu_memory_manager")) { > > move to lrc capabilities, test capaibilities Okay, no problem. For future reference, I thought capabilities was to inform clients of LRC capabilities, or is it also for efficiency so we dont scan strings each time? >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:316 >> + m_isFramebufferDiscarded = false; > > you set this on :233, do you need to again here? I wasn't sure if initialize gets called more than once (like after context loss?), but if not, it should be removed. This reminds me to test killing gpu process. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1163 >> + if (extensions->supports("GL_EXT_discard_framebuffer")) { > > you should move this to a bool on LayerRendererCapabilities that you probe at initialize time rather than hammering extensions will do. >> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:75 >> + OwnPtr<LayerRendererChromium::ScopedEnsureFramebufferAllocation> sfa(m_layerTreeHostImpl->layerRenderer()->createScopedEnsureFramebufferAllocation()); > > For these kinds of thigns you dont need the pointer stuff. Look at CCThread.h debugscopedsomethignorother. Copy that style. will do.

Created attachment 133128 [details] Patch

Comment on attachment 133128 [details] Patch Alright, fixed the nits and cleaned up a few things. Also fixed two related bugs that happened when rapid tab switching: 1. Don't swap with a discarded fb 2. Always call ensure fb when given an allocation requesting one. Usually ends up a no-op, but sometimes previous allocation is delivered very late (since it is asynchronous). This race only appeared when using non-threaded composting, perhaps because impl thread is so much more responsive that race becomes unlikely. This patch should and can land before the chromium side patch lands.

Are you able to make unit tests for these cases you fixed?

The problem only arose during rapid tab switching and only rarely. I think it would be difficult to make a reliable unit test. I will see if there are any existing rapid tab switching related tests.

(In reply to comment #8) > The problem only arose during rapid tab switching and only rarely. I think it would be difficult to make a reliable unit test. > > I will see if there are any existing rapid tab switching related tests. There aren't tab switching tests, but you could forcibly drop a front buffer on an LRC and make sure there was no swap and it got recreated properly. We have fake graphics contexts, so you could create an LRC in a unit test.

(In reply to comment #9) > (In reply to comment #8) > > The problem only arose during rapid tab switching and only rarely. I think it would be difficult to make a reliable unit test. > > > > I will see if there are any existing rapid tab switching related tests. > > There aren't tab switching tests, but you could forcibly drop a front buffer on an LRC and make sure there was no swap and it got recreated properly. We have fake graphics contexts, so you could create an LRC in a unit test. In particular, look at CCLayerTreeHostImplTest.cpp.

Created attachment 133152 [details] Patch

Comment on attachment 133152 [details] Patch Added a unittest to make sure we do the right thing regarding swaps and discarded framebuffers. This test will likely be removed once we have automatic recreation of backbuffer upon first use.

Comment on attachment 133152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133152&action=review Looks good in general. Here's a bunch of small things to fix, and then we can land this. Thanks so much for the test! > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1078 > + m_client->setFullRootLayerDamage(); Is this redundant? It looks like you already set the full root layer damage during discardFramebuffer. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1110 > + if (m_capabilities.usingDiscardFramebuffer) { style nit: early out here, rather than indenting this whole block of code. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1127 > + if (m_capabilities.usingDiscardFramebuffer) { style nit: early out here too. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:169 > + friend class LayerRendererGpuMemoryAllocationChangedCallbackAdapter; > + friend class ScopedEnsureFramebufferAllocation; > + void discardFramebuffer(); > + void ensureFramebuffer(); I think you should just make these functions public for the sake of future design, add an isFramebufferDiscarded() function, and remove the friend class. At some point, we'd like to make a CCRenderer interface that LayerRendererChromium can implement, and I think that interface would end up using these framebuffer functions directly. > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:75 > + ScopedEnsureFramebufferAllocation sefa(m_layerTreeHostImpl->layerRenderer()); style nit: please don't use abbreviations. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:94 > + ScopedEnsureFramebufferAllocation sefa(m_layerTreeHostImpl->layerRenderer()); style nit: please don't use abbreviations. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:53 > + virtual const IntSize& viewportSize() const { static IntSize sz; return sz; } style nit: please don't use abbreviations. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64 > + LayerRendererChromiumTest(LayerRendererChromiumClient* lrcc, PassRefPtr<GraphicsContext3D> context) style nit: please don't use abbreviations. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:83 > + LayerRendererChromiumTest lrc(&client, context.release()); style nit: please don't use abbreviations.

View in context: https://bugs.webkit.org/attachment.cgi?id=133152&action=review Getting there... you want your tests to test the feature you're adding --- this is (a) responding to the callback in both directions, squelching swaps, issuing full root layer damage at the right time, and temporarily recreating the framebuffer during forced draws. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:59 > + virtual void setFullRootLayerDamage() { } you need to also test that full rld is set at the right place > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:93 > + lrc.discardFramebuffer(); I think you're missing the key part --- you need to not swap and you need to issue full damage. you should make a test that advertise the extension and then trigger the callback, and expect that you got a discard then you need to try drawing and expect to see an ensure call, followed by a discard. and, you should then test that firing the callback again and setting it to 1 trigers an ensure again

Comment on attachment 133152 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133152&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1078 >> + m_client->setFullRootLayerDamage(); > > Is this redundant? It looks like you already set the full root layer damage during discardFramebuffer. This isn't redundant because setFullRootLayerDamage() only applies for the next frame, and the partial swap code does not yet handle discarding a swap. I've coordinated with Shawn to make sure the damage tracker is not out of synch at this point, but we do need to reset damage so that once backbuffer is recreated we push a full frame.

Created attachment 133276 [details] Patch

Comment on attachment 133276 [details] Patch Fixed nits and expanded unit test significantly. Figuring out how to force extensions was a pain (examples in other unittests dont have the word "extension" anywhere and so was not grep-able). extensions->ensureEnabled does not do what I expected it to do :( You live, you learn.

Comment on attachment 133276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133276&action=review Thanks for all the additional tests. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:353 > + // FIXME: Remove this once framebuffer is automatically recreated on first use > + ensureFramebuffer(); Do you want to ensure the framebuffer even on setVisible(false)? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1080 > + if (m_isFramebufferDiscarded) { > + m_client->setFullRootLayerDamage(); > + return; > + } style nit: four space indent. Kind of surprised stylebot didn't yell. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:134 > + // Reset back to suggestHaveBackbufferNo allocation. > + mockContext.setMemoryAllocation(suggestHaveBackbufferYes); s/No/Yes/ > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:146 > + // Make sure swaps are ignored while framebuffer is droped, and that setFullRootLayerDamage is called. s/droped/dropped/

Comment on attachment 133276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133276&action=review >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:353 >> + ensureFramebuffer(); > > Do you want to ensure the framebuffer even on setVisible(false)? No, I don't. It wouldn't do anything at this moment, but that may change once we start plumbing visibility to gpu process directly from browser. Thanks, I'll make the change. >> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1080 >> + } > > style nit: four space indent. Kind of surprised stylebot didn't yell. Indeed! >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:134 >> + mockContext.setMemoryAllocation(suggestHaveBackbufferYes); > > s/No/Yes/ Thanks. >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:146 >> + // Make sure swaps are ignored while framebuffer is droped, and that setFullRootLayerDamage is called. > > s/droped/dropped/ Thanks.

Created attachment 133296 [details] Patch

Comment on attachment 133296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133296&action=review Better, thank you. However, while you've gotten good coverage with these tests, they're not quite written in a stylistically correct way. The thing you need now is to split these up so you have 1 test per unit of behavior, whereas now you've got one giant megasuperduperubertest covering tons of behavior. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:111 > + You need to follow standard test style. Change these so that they are different TEST_ macros. A TEST_ macro should test 1 thing in isolation.... You can take everything from line 101 to line 110 and put them in a test fixture class as follows (or you can read this doc http://code.google.com/p/googletest/source/browse/trunk/samples/sample3_unittest.cc) class DiscardTestFixture : public testing::Test { virtual void SetUp() { ... set up stuff, stuff variables } LRC m_layerRendererChromium } Once ou have that, then TEST_F( DiscardTestFixture, SuggestHaveBackbuffersYesShouldNotSwap) { this is actually a member function of the discard test fixture } > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:113 > + layerRendererChromium.swapBuffers(IntRect()); I dont understand the poitn of this test. You're testing the test harness here, I'm not sure you need to do this one. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:119 > + // Send suggestHaveBackbufferYes allocation, and expect no difference. "Expect no difference" is a content free statement. Expect ____ something.... i think you meant to say "sending an allocationYes when you already have a yes should not cause anything to happen." Try to remember, these are going to pop and someone != you is going to have to understand what you were intending to test, what is going on here and so on. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:121 > + layerRendererChromium.swapBuffers(IntRect()); This seems like a different test. "Expect that a newly initialized lrc swaps. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:122 > + EXPECT_EQ(2, mockContext.frameCount()); once you use a test fixture, you dont have to reset or track these numbers anymore. The test fixture is deleted and recreated after each test --- that is, each TEST_F gets its own instance of the fixture. > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:127 > + // Send suggestHaveBackbufferNo allocation. Yes, and what do you expect. This should become the name of the TEST_F function., E.g. TEST_F(..., SuggestBackbufferNoShouldDamageRootLayerAndDiscardFrameBuffer)

Comment on attachment 133296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133296&action=review > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:42 > + // This method would normally do a glSwapBuffers under the hood. Same comment as below. Put a comment that groups the interface methods separately from the added-value methods > Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:68 > + virtual const IntSize& viewportSize() const { static IntSize fakeSize; return fakeSize; } Make a comment saying // LayerRendererChromiumClient implementation to separate out interface from extra methods

Comment on attachment 133296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=133296&action=review >> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:111 >> + > > You need to follow standard test style. Change these so that they are different TEST_ macros. A TEST_ macro should test 1 thing in isolation.... > > You can take everything from line 101 to line 110 and put them in a test fixture class as follows (or you can read this doc http://code.google.com/p/googletest/source/browse/trunk/samples/sample3_unittest.cc) > > class DiscardTestFixture : public testing::Test { > virtual void SetUp() { > ... set up stuff, stuff variables > } > LRC m_layerRendererChromium > > } > > Once ou have that, then > > TEST_F( DiscardTestFixture, SuggestHaveBackbuffersYesShouldNotSwap) { > this is actually a member function of the discard test fixture > } Thanks, this evolved from a simple test which is how we got here, but you are 100% correct and I've updated. FYI, seems TEST_F does not make the test a member function, but rather a subclass, so locals have to be marked protected.

Created attachment 133328 [details] Patch

Comment on attachment 133328 [details] Patch Found a typo, new patch incoming.

Created attachment 133331 [details] Patch

Comment on attachment 133331 [details] Patch LGTM ty

Comment on attachment 133331 [details] Patch I am just going to rename GL_EXT_discard_framebuffer to GL_CHROMIUM_discard_framebuffer since it doesn't yet do exactly what standard extension requires, and chromium side patch reviewers wanted to see this change for now.

Created attachment 133341 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 133341 [details] Patch Looks great! R=me

Created attachment 133348 [details] Patch

Comment on attachment 133348 [details] Patch Yeah, the extensions haven't changed because of this patch, so no need to change the comments on the public API, I think.

Looks fine to me (I also think that changing a comment doesn't really count as changing the API).

Comment on attachment 133348 [details] Patch Clearing flags on attachment: 133348 Committed r111777: <http://trac.webkit.org/changeset/111777>

All reviewed patches have been landed. Closing bug.

(In reply to comment #35) > (From update of attachment 133348 [details]) > Clearing flags on attachment: 133348 > > Committed r111777: <http://trac.webkit.org/changeset/111777> I believe CCLayerTreeHostImplTest.visibilityChangeResetsDamage started failing after this change.

Thanks for finding that Tony. This test is expected to fail now, since we no longer drop backbuffer on visibility change (we wait for an allocation, which usually happens some time after the visibility change, but may not). Because we don't, we don't need to force reset damage.

Patch incoming

I assumed CQ should have caught this, so filed a bug: https://bugs.webkit.org/show_bug.cgi?id=82007

Yeh CQ/EWS doesn't run unit tests right now. It did for a short while. One day it may again.

Committed r111816: <http://trac.webkit.org/changeset/111816>