[Webkit-unassigned] [Bug 73634] [GTK] Add TextureMapper ImageBuffer support as a fallback from the hardware accelerated path

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73634


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #144511|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #16 from Martin Robinson <mrobinson at webkit.org>  2012-05-29 08:23:39 PST ---
(From update of attachment 144511)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.