[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





--- Comment #13 from Alejandro G. Castro <alex at igalia.com>  2012-05-28 10:28:07 PST ---
(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!

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