[webkit-reviews] review denied: [Bug 73634] [GTK] Add TextureMapper ImageBuffer support as a fallback from the hardware accelerated path : [Attachment 143550] Proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 23 13:25:27 PDT 2012
Martin Robinson <mrobinson at webkit.org> has denied Alejandro G. Castro
<alex at igalia.com>'s request for review:
Bug 73634: [GTK] Add TextureMapper ImageBuffer support as a fallback from the
hardware accelerated path
https://bugs.webkit.org/show_bug.cgi?id=73634
Attachment 143550: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=143550&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=143550&action=review
Looking pretty good. Just a few small suggestions. Do we lose any information
by immediately converting the transformation matrix to an affine transformation
instead of building up a transformation matrix and then converting it to an
affine transformation when necessary?
Just for you information, I have plans to eliminate the different AC paths and
just have the GL one. When GL fails, we'll just fall back to ImageBuffer.
> Source/WebCore/ChangeLog:3
> + [GTK] [GTK] Add TextureMapper ImageBuffer support as a fallback
Two [GTK]s here. :)
> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:93
> +#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER) &&
USE(TEXTURE_MAPPER_CAIRO)
> +void GraphicsContext3DPrivate::paintToTextureMapper(TextureMapper*
textureMapper, const FloatRect& targetRect, const TransformationMatrix& matrix,
float opacity, BitmapTexture* mask)
> +{
> +}
> +#endif // USE(ACCELERATED_COMPOSITING)
It should be possible to support both TextureMappers at once. Take a look at
the Qt version of paintToTextureMapper.
> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:44
> + IntRect srcRect(sourceOffset, targetRect.size());
> +
m_image->context()->platformContext()->drawSurfaceToContext(surface.get(),
targetRect, srcRect, m_image->context());
You should be able to specify srcRect inline like this
m_image->context()->platformContext()->drawSurfaceToContext(surface.get(),
targetRect, IntRect(sourceOffset, targetRect.size()), m_image->context());
> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:61
> bool enabled();
I think it makes sense to just add the cairo_t* parameter to
renderLayersToWindow and have it unused in the GL version. That would elminate
the ifdef here.
> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:86
> +#elif USE(TEXTURE_MAPPER_CAIRO)
> + WebCore::TextureMapperLayer* m_rootTextureMapperLayer;
> + OwnPtr<WebCore::GraphicsLayer> m_rootGraphicsLayer;
> + OwnPtr<WebCore::TextureMapper> m_textureMapper;
Can you unify the shared variables by putting them under #if
USE(TEXTURE_MAPPER)?
> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:544
> +#if USE(TEXTURE_MAPPER_CAIRO)
> + gtk_widget_queue_draw_area(GTK_WIDGET(m_webView),
> + rect.x(), rect.y(),
> + rect.width(), rect.height());
> +#endif
Is it possible to simply move this to scheduleRootLayerRepaint?
> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:605
> +#if USE(TEXTURE_MAPPER_GL)
>
m_webView->priv->acceleratedCompositingContext->renderLayersToWindow(rect);
> #endif
I think it would be possible to avoid this #ifdef here by simply passing 0 for
the cairo_t * and exiting renderLayersToWindow early.
> Source/WebKit/gtk/webkit/webkitwebview.cpp:1014
> + WebKitWebViewPrivate* priv = WEBKIT_WEB_VIEW(widget)->priv;
Thanks for cleaning this up!
More information about the webkit-reviews
mailing list