Bug 73634

Summary: [GTK] Add TextureMapper ImageBuffer support as a fallback from the hardware accelerated path
Product: WebKit Reporter: Alejandro G. Castro <alex>
Component: WebKit GtkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eoin.shanaghy, gns, kevin.cs.oh, mrobinson, nayankk, noam, pnormand, webkit.review.bot, xan.lopez
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 Alejandro G. Castro 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 Alejandro G. Castro 2011-12-02 10:03:49 PST
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.
Comment 2 Alejandro G. Castro 2011-12-02 11:32:43 PST
Created attachment 117660 [details]
Proposed patch

Added proper defines
Comment 3 Noam Rosenthal 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 Alejandro G. Castro 2011-12-04 02:07:21 PST
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);
Comment 5 Alejandro G. Castro 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 WebKit Review Bot 2011-12-04 02:53:02 PST
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
Comment 7 Joone Hur 2011-12-04 03:38:55 PST
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
Comment 8 Philippe Normand 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 Philippe Normand 2012-02-17 07:11:55 PST
Also, TextureMapperNode was renamed to TextureMapperLayer.
Comment 10 Noam Rosenthal 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 Alejandro G. Castro 2012-05-23 05:55:58 PDT
Created attachment 143550 [details]
Proposed patch
Comment 12 Martin Robinson 2012-05-23 13:24:31 PDT
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!
Comment 13 Alejandro G. Castro 2012-05-28 10:28:07 PDT
(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!
Comment 14 Alejandro G. Castro 2012-05-29 04:12:26 PDT
(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.
Comment 15 Alejandro G. Castro 2012-05-29 04:18:35 PDT
Created attachment 144511 [details]
Proposed patch
Comment 16 Martin Robinson 2012-05-29 08:23:39 PDT
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.
Comment 17 Alejandro G. Castro 2012-06-08 14:16:24 PDT
(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.
Comment 18 Alejandro G. Castro 2012-06-08 14:32:23 PDT
Created attachment 146650 [details]
Proposed patch
Comment 19 Gustavo Noronha (kov) 2012-06-08 15:44:58 PDT
Comment on attachment 146650 [details]
Proposed patch

Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12909736
Comment 20 Gustavo Noronha (kov) 2012-06-08 15:59:40 PDT
Comment on attachment 146650 [details]
Proposed patch

Attachment 146650 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12910746
Comment 21 Alejandro G. Castro 2012-06-11 04:27:36 PDT
Created attachment 146829 [details]
Proposed patch
Comment 22 Martin Robinson 2012-06-12 12:52:59 PDT
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.
Comment 23 Alejandro G. Castro 2012-06-14 12:47:53 PDT
(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.
Comment 24 Alejandro G. Castro 2012-06-14 13:38:08 PDT
Landed! http://trac.webkit.org/changeset/120359

Thanks