[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]

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!