[webkit-reviews] review granted: [Bug 73634] [GTK] Add TextureMapper ImageBuffer support as a fallback from the hardware accelerated path : [Attachment 144511] Proposed patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted 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 144511: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=144511&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=144511&action=review


Looking good. r+ though there are a few things to fix before landing.

> Source/WebCore/ChangeLog:4
> +	   [GTK] Add TextureMapper ImageBuffer support as a fallback
> +	   from the hardware accelerated path

Careful here, I don't think there should be a newline in the description.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:30
> +#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER) &&
USE(TEXTURE_MAPPER_CAIRO)

Are you sure you want to guard the inclusion of TextureMapperGL.h with
TEXTURE_MAPPER_CAIRO?

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:87
> +	   glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER, m_context->m_fbo);

> +	   glReadPixels(/* x */ 0, /* y */ 0, width, height,
GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, imagePixels);
> +	   glBindFramebuffer(GraphicsContext3D::FRAMEBUFFER,
m_context->m_boundFBO);

You should set the GL_PACK_ROW_LENGTH to the stride of the image. While in
practice this works with many images, I think that this code could break in
some cases if you do not. So for instance you would call
glPixelStorei(GL_PACK_ROW_LENGTH, stride / 4).

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:91
> +	  
context->platformContext()->drawSurfaceToContext(offscreenImage.get(),
targetRect,

This can be one line.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextCairo.cpp:107
> +    gtk_widget_queue_draw_area(GTK_WIDGET(m_webView),
> +				  rect.x(), rect.y(),
> +				  rect.width(), rect.height());

This can be one line.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextCairo.cpp:169
> +   
copyRectFromCairoSurfaceToContext(m_webView->priv->backingStore->cairoSurface()
, cr,
> +					 IntSize(), rectToPaint);

One line again.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextClutter.cpp:56
> +qbool AcceleratedCompositingContext::renderLayersToWindow(cairo_t*, const
IntRect& clipRect)

I think you may have accidentally inserted an extra q here.