Bug 73634

Summary: [GTK] Add TextureMapper ImageBuffer support as a fallback from the hardware accelerated path
Product: WebKit Reporter: Alejandro G. Castro <alex@igalia.com>
Component: WebKit GtkAssignee: Nobody <webkit-unassigned@lists.webkit.org>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov@chromium.org, eoin.shanaghy@gmail.com, gns@gnome.org, kevin.cs.oh@gmail.com, mrobinson@webkit.org, nayankk@motorola.com, noam@webkit.org, pnormand@igalia.com, webkit.review.bot@gmail.com, xan.lopez@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
Bug Depends on: 73655, 73713    
Bug Blocks: 74087    
Attachments:
Description Flags
Proposed patch
none
Proposed patch
none
Proposed patch
webkit.review.bot: commit‑queue-
Proposed patch
mrobinson: review-
Proposed patch
none
Proposed patch
gns: commit‑queue-
Proposed patch none

Description From 2011-12-02 01:44:16 PST
This bug follows the initial implementation of TextureMapperCairo that adds the initial glue for the accelerated compositing support in GTK+.
------- Comment #1 From 2011-12-02 10:03:49 PST -------
Created an attachment (id=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.
------- Comment #2 From 2011-12-02 11:32:43 PST -------
Created an attachment (id=117660) [details]
Proposed patch

Added proper defines
------- Comment #3 From 2011-12-02 15:14:43 PST -------
> 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.
------- Comment #4 From 2011-12-04 02:07:21 PST -------
Created an attachment (id=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);
------- Comment #5 From 2011-12-04 02:08:17 PST -------
Also the timer for the animation is just the straighforward solution we can get way better result than with the timeouts.
------- Comment #6 From 2011-12-04 02:53:02 PST -------
(From update of attachment 117787 [details])
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
------- Comment #7 From 2011-12-04 03:38:55 PST -------
(From update of attachment 117787 [details])
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
------- Comment #8 From 2012-02-17 07:11:03 PST -------
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
------- Comment #9 From 2012-02-17 07:11:55 PST -------
Also, TextureMapperNode was renamed to TextureMapperLayer.
------- Comment #10 From 2012-02-17 07:14:49 PST -------
(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 :)
------- Comment #11 From 2012-05-23 05:55:58 PST -------
Created an attachment (id=143550) [details]
Proposed patch
------- Comment #12 From 2012-05-23 13:24:31 PST -------
(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? 

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!
------- Comment #13 From 2012-05-28 10:28:07 PST -------
(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.

> > 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!
------- Comment #14 From 2012-05-29 04:12:26 PST -------
(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 143550 [details] [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.
------- Comment #15 From 2012-05-29 04:18:35 PST -------
Created an attachment (id=144511) [details]
Proposed patch
------- Comment #16 From 2012-05-29 08:23:39 PST -------
(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.

> 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.
------- Comment #17 From 2012-06-08 14:16:24 PST -------
(In reply to comment #16)
> (From update of attachment 144511 [details] [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.
------- Comment #18 From 2012-06-08 14:32:23 PST -------
Created an attachment (id=146650) [details]
Proposed patch
------- Comment #19 From 2012-06-08 15:44:58 PST -------
(From update of attachment 146650 [details])
Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12909736
------- Comment #20 From 2012-06-08 15:59:40 PST -------
(From update of attachment 146650 [details])
Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12910746
------- Comment #21 From 2012-06-11 04:27:36 PST -------
Created an attachment (id=146829) [details]
Proposed patch
------- Comment #22 From 2012-06-12 12:52:59 PST -------
(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.

> 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.
------- Comment #23 From 2012-06-14 12:47:53 PST -------
(In reply to comment #22)
> (From update of attachment 146829 [details] [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.
------- Comment #24 From 2012-06-14 13:38:08 PST -------
Landed! http://trac.webkit.org/changeset/120359

Thanks