[webkit-reviews] review denied: [Bug 56749] Enable skia gpu rendering for content layers : [Attachment 86530] proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 23 00:43:29 PDT 2011


Vangelis Kokkevis <vangelis at chromium.org> has denied  review:
Bug 56749: Enable skia gpu rendering for content layers
https://bugs.webkit.org/show_bug.cgi?id=56749

Attachment 86530: proposed patch
https://bugs.webkit.org/attachment.cgi?id=86530&action=review

------- Additional Comments from Vangelis Kokkevis <vangelis at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86530&action=review

I don't think this is quite ready to check in yet.  I'm changing to r- as I
think there are some issues that need to be addressed even for a prototype
implementation.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:54
> +    if (!gGR) {

This suffers from the same issue that the global GrContext used by Canvas2D
does. There will be one global GrContext per Render process, however there
could be multiple GC3D's used (one per tab).  The easy way to trigger the bug
is to run chrome in --single-process mode and open two composited tabs.  A
quick workaround here would be to have one GrContext per LayerRenderChromium.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:94
> +	   layerRendererContext()->deleteFramebuffer(m_fbo);

need to also set m_fbo to 0

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:215
> +	   context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_fbo);

I'm surprised that this works at all. Doesn't binding the new framebuffer
interfere with the framebuffer the compositor is using?  Same for the
viewport...  This method is currently getting called while the compositor is in
the middle of rendering which means that changes to the bound framebuffer
and/or viewport will be unexpected.

> Source/WebKit/chromium/src/WebViewImpl.cpp:2429
> +   
m_layerRenderer->setAcceleratedDrawingEnabled(m_page->settings()->acceleratedDr
awingEnabled());

m_page->settings()   ->  settings()


More information about the webkit-reviews mailing list