Bug 81823 – [chromium] LayerRendererChromium should use GpuMemoryAllocationChanged callback to explicitly manage framebuffer.

• Component: New Bugs
• Status: RESOLVED FIXED

[Michal Mocny]

[chromium] LayerRendererChromium should use GpuMemoryAllocationChanged callback to explicitly manage framebuffer.

[Michal Mocny]

Created attachment 133093 [details]
Patch

[Michal Mocny]

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.

[Nat Duca]

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.

[Michal Mocny]

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.

[Michal Mocny]

Created attachment 133128 [details]
Patch

[Michal Mocny]

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.

[Dana Jansens]

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

[Michal Mocny]

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.

[Adrienne Walker]

(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.

[Adrienne Walker]

(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.

[Michal Mocny]

Created attachment 133152 [details]
Patch

[Michal Mocny]

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.

[Adrienne Walker]

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.

[Nat Duca]

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

[Michal Mocny]

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.

[Michal Mocny]

Created attachment 133276 [details]
Patch

[Michal Mocny]

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.

[Adrienne Walker]

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/

[Michal Mocny]

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.

[Michal Mocny]

Created attachment 133296 [details]
Patch

[Nat Duca]

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)

[Nat Duca]

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

[Michal Mocny]

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.

[Michal Mocny]

Created attachment 133328 [details]
Patch

[Michal Mocny]

Comment on attachment 133328 [details]
Patch

Found a typo, new patch incoming.

[Michal Mocny]

Created attachment 133331 [details]
Patch

[Nat Duca]

Comment on attachment 133331 [details]
Patch

LGTM ty

[Michal Mocny]

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.

[Michal Mocny]

Created attachment 133341 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

Comment on attachment 133341 [details]
Patch

Looks great! R=me

[Michal Mocny]

Created attachment 133348 [details]
Patch

[Adrienne Walker]

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.

[James Robinson]

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

[WebKit Review Bot]

Comment on attachment 133348 [details]
Patch

Clearing flags on attachment: 133348

Committed r111777: <http://trac.webkit.org/changeset/111777>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

(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.

[Michal Mocny]

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.

[Nat Duca]

Patch incoming

[Michal Mocny]

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

[Dana Jansens]

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

[Adrienne Walker]

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