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

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alejandro G. Castro]

This bug follows the initial implementation of TextureMapperCairo that adds the initial glue for the accelerated compositing support in GTK+.

[Alejandro G. Castro]

Created attachment 117643 [details]
Proposed patch

This is the initial proposal, I still have to solve issues with the transformation matrix, the perspective does not work and some other issues. But if you have time to check it and send feedback (thanks in advance Noam and others ;) that would be great.

[Alejandro G. Castro]

Created attachment 117660 [details]
Proposed patch

Added proper defines

[Noam Rosenthal]

> This is the initial proposal, I still have to solve issues with the transformation matrix, the perspective does not work and some other issues. But if you have time to check it and send feedback (thanks in advance Noam and others ;) that would be great.

Hi Alejandro
I'd be happy to help out, but most of this code seems to be very Cairo specific, which I'm not very well versed with. In general you seem to be tackling the right pieces in a good way.

[Alejandro G. Castro]

Created attachment 117787 [details]
Proposed patch

New version of the patch, it is limited in how we get the transformation matrix because we are using cairo affine transformations, I'll try to check if we can someway make a projection of the other parts of the matrix instead of just use some of them, like we do in:

    const cairo_matrix_t cairoMatrix = cairo_matrix_t(matrix);

[Alejandro G. Castro]

Also the timer for the animation is just the straighforward solution we can get way better result than with the timeouts.

[WebKit Review Bot]

Comment on attachment 117787 [details]
Proposed patch

Attachment 117787 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10731445

New failing tests:
svg/custom/linking-uri-01-b.svg

[Joone Hur]

Comment on attachment 117787 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117787&action=review

> Source/WebKit/gtk/webkit/webkitwebviewprivate.h:38
> +#include "TextureMapperNode.h"

I think we need to add USE(TEXTURE_MAPPER_CAIRO) here because it may have a conflict with my patch https://bugs.webkit.org/show_bug.cgi?id=73319

[Philippe Normand]

This patch no longer builds. I guess it's because of some recent changes in the TextureMapper:


In file included from ../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:23:0:
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.h: In static member function ‘static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapperCairo::create()’:
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.h:75:69: error: cannot allocate an object of abstract type ‘WebCore::TextureMapperCairo’
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.h:59:7: note:   because the following virtual functions are pure within ‘WebCore::TextureMapperCairo’:
../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:109:30: note: 	virtual WebCore::TextureMapper::AccelerationMode WebCore::TextureMapper::accelerationMode() const
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp: In member function ‘virtual WTF::PassRefPtr<WebCore::BitmapTexture> WebCore::TextureMapperCairo::createTexture()’:
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:236:44: error: cannot allocate an object of abstract type ‘WebCore::BitmapTextureCairo’
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.h:31:7: note:   because the following virtual functions are pure within ‘WebCore::BitmapTextureCairo’:
../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:60:18: note: 	virtual void WebCore::BitmapTexture::updateContents(WebCore::Image*, const WebCore::IntRect&, const WebCore::IntRect&, WebCore::BitmapTexture::PixelFormat)
../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:61:18: note: 	virtual void WebCore::BitmapTexture::updateContents(const void*, const WebCore::IntRect&)
../../../Source/WebCore/platform/graphics/cairo/TextureMapperCairo.cpp:237:1: warning: control reaches end of non-void function [-Wreturn-type]
make[1]: *** [Source/WebCore/platform/graphics/cairo/libWebCoreGtk_la-TextureMapperCairo.lo] Error 1

[Philippe Normand]

Also, TextureMapperNode was renamed to TextureMapperLayer.

[Noam Rosenthal]

(In reply to comment #9)
> Also, TextureMapperNode was renamed to TextureMapperLayer.

Folks, I'm doing lots of work on TextureMapper. If you guys finish upstreaming this or other changes I'd be happy to make sure I don't break it :)

[Alejandro G. Castro]

Created attachment 143550 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 143550 [details]
Proposed patch

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!

[Alejandro G. Castro]

(In reply to comment #12)
> (From update of attachment 143550 [details])
> 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? 
> 

Yep, we are losing information, namely perspective information. I've added a FIXME in the patch but I'll try to check if we can do it later.

> > Source/WebCore/ChangeLog:3
> > +        [GTK] [GTK] Add TextureMapper ImageBuffer support as a fallback
> 
> Two [GTK]s here. :)
> 

Oops :).

> > 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.
> 

I'll add that part, thanks.

> > 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());
>

Yep.

> > 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.
> 

Ok, I'll do that.

> > 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)?
>

I think so, I'll change it.

> > 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?
>

I'll check if we can do it.

> > 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.
>

Yep, I can replace the interface of the function like you suggested.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:1014
> > +    WebKitWebViewPrivate* priv = WEBKIT_WEB_VIEW(widget)->priv;
> 
> Thanks for cleaning this up!

Your welcome! :)

Thanks for the thorough review!

[Alejandro G. Castro]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 143550 [details] [details])
> > 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? 
> > 
> 
> Yep, we are losing information, namely perspective information. I've added a FIXME in the patch but I'll try to check if we can do it later.

Just checked the only function using the get3DTransform is the beginClip and we are getting a transformation that could be a mix of 2D and 3D transforms converted at that point. So we would have to check all the transformations and try to approximate the perspective some other way if we want to do something smarter. I guess we can do that in next patches if we find a good way.

[Alejandro G. Castro]

Created attachment 144511 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 144511 [details]
Proposed patch

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.

[Alejandro G. Castro]

(In reply to comment #16)
> (From update of attachment 144511 [details])
> 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.
> 

Thanks for the review, I've checked all the comments and modified a little bit the implementation, so I'll upload a new patch to review.

[Alejandro G. Castro]

Created attachment 146650 [details]
Proposed patch

[Gustavo Noronha (kov)]

Comment on attachment 146650 [details]
Proposed patch

Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12909736

[Gustavo Noronha (kov)]

Comment on attachment 146650 [details]
Proposed patch

Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12910746

[Alejandro G. Castro]

Created attachment 146829 [details]
Proposed patch

[Martin Robinson]

Comment on attachment 146829 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146829&action=review

Okay. This is a good start. Now we just need to unify the GL and ImageBuffer implementations.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:99
> +        // OpenGL keeps the pixels stored bottom up, so we need to flip the image here.
> +        context->translate(0, height);
> +        context->scale(FloatSize(1, -1));

I think you want to translate and scale before applying the compositing matrix, but I'm not certain.

> Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.h:43
> +    void paintToCanvasRect(const unsigned char* imagePixels, int imageWidth, int imageHeight, const FloatRect& canvasRect, PlatformContextCairo*);

I guess this was added accidentally? I don't see the implementation anywhere.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:544
> +

This newline seems to be added accidentally as well.

[Alejandro G. Castro]

(In reply to comment #22)
> (From update of attachment 146829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146829&action=review
> 
> Okay. This is a good start. Now we just need to unify the GL and ImageBuffer implementations.
> 

Great, thanks for the review! :)

> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.cpp:99
> > +        // OpenGL keeps the pixels stored bottom up, so we need to flip the image here.
> > +        context->translate(0, height);
> > +        context->scale(FloatSize(1, -1));
> 
> I think you want to translate and scale before applying the compositing matrix, but I'm not certain.
>

I agree, makes more sense, I'll change the order.

> > Source/WebCore/platform/graphics/cairo/GraphicsContext3DPrivate.h:43
> > +    void paintToCanvasRect(const unsigned char* imagePixels, int imageWidth, int imageHeight, const FloatRect& canvasRect, PlatformContextCairo*);
> 
> I guess this was added accidentally? I don't see the implementation anywhere.
>

Yep, fixed.

> > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:544
> > +
> 
> This newline seems to be added accidentally as well.

Fixed.

Thanks again for the review.

[Alejandro G. Castro]

Landed! http://trac.webkit.org/changeset/120359

Thanks