Bug 82569 – [chromium] Ensure framebuffer exists at the start of beginDrawingFrame.

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

[Michal Mocny]

[chromium] Ensure framebuffer exists at the start of beginDrawingFrame.

[Michal Mocny]

Created attachment 134491 [details]
Patch

[Michal Mocny]

Comment on attachment 134491 [details]
Patch

If we ensure that a framebuffer exists before beginning to draw a frame, we need not have clients explicitly manage it, and we can be sure it is done on the right thread.

This is also how it will work once it recreates itself of first use, so it is good practice.

We need not discard a recreated framebuffer, because GpuMemoryManager will send another allocation suggesting to discard it.

[Nat Duca]

Comment on attachment 134491 [details]
Patch

LGTM

[Michal Mocny]

Comment on attachment 134491 [details]
Patch

one sec, patch to remove public methods as they are no longer needed inc

[Michal Mocny]

Created attachment 134493 [details]
Patch

[Nat Duca]

Comment on attachment 134493 [details]
Patch

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

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64
> +class FakeDrawableCCLayerImpl: public CCLayerImpl {

Not sure what these root layer changes are for...?

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196
> +    m_layerRendererChromium.beginDrawingFrame();

Where's the expectation that we called ensure?

[Michal Mocny]

Comment on attachment 134493 [details]
Patch

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

>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64
>> +class FakeDrawableCCLayerImpl: public CCLayerImpl {
> 
> Not sure what these root layer changes are for...?

We assert that we do not have a null root layer in beginDrawingFrame.

>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196
>> +    m_layerRendererChromium.beginDrawingFrame();
> 
> Where's the expectation that we called ensure?

Right below, before this line we EXPECT that isFramebufferDiscarded returns true, after this line it returns false.

[Nat Duca]

Comment on attachment 134493 [details]
Patch

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

Got it. Makes sense. Still lgtm.

>>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:196
>>> +    m_layerRendererChromium.beginDrawingFrame();
>> 
>> Where's the expectation that we called ensure?
> 
> Right below, before this line we EXPECT that isFramebufferDiscarded returns true, after this line it returns false.

So you might want to put before this line expect no framebuffer

[James Robinson]

Comment on attachment 134493 [details]
Patch

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

R=me. Could use some small cleanup before landing.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:168
> +    friend class LayerRendererGpuMemoryAllocationChangedCallbackAdapter;

this adapter confuses me. It adapts the call onSwapBuffersComplete() to onSwapBuffersComplete(). Why doesn't LRC just implement Extensions3DChromium::SwapBuffersCompleteCallbackCHROMIUM ?

>>> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:64
>>> +class FakeDrawableCCLayerImpl: public CCLayerImpl {
>> 
>> Not sure what these root layer changes are for...?
> 
> We assert that we do not have a null root layer in beginDrawingFrame.

This class appears to do nothing but expose the constructor (which is protected). However there's a ::create() function that does exactly what you want. Just make your test client have an OwnPtr<CCLayerImpl> and you can test the real class instead of this somewhat silly fake

> Source/WebKit/chromium/tests/LayerRendererChromiumTest.cpp:71
> +    FakeLayerRendererChromiumClient() : m_setFullRootLayerDamageCount(0), m_rootLayer(1) { }

one statement per line

[Michal Mocny]

Comment on attachment 134493 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:168
>> +    friend class LayerRendererGpuMemoryAllocationChangedCallbackAdapter;
> 
> this adapter confuses me. It adapts the call onSwapBuffersComplete() to onSwapBuffersComplete(). Why doesn't LRC just implement Extensions3DChromium::SwapBuffersCompleteCallbackCHROMIUM ?

I assume you meant to type onMemoryAllocationChanged, and the main reason was to not need to expose extensions3dchromium in the header, and was copying the style of onSwap...
I can make the change if that is preferred.

[James Robinson]

Right, the other one. I think it's fine for LayerRendererChromium to know about Extensions, the point of LRC is to implement GraphicsContext3D-based functionality.

[Nat Duca]

I think also that the E3D callback model takes ownership of the callback ptr. We dont want G3CD deleting the lrc :)

[Michal Mocny]

Created attachment 134639 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134639 [details]
Patch

Clearing flags on attachment: 134639

Committed r112568: <http://trac.webkit.org/changeset/112568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.