Bug 75308 – [GTK] Add TextureMapperGL implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ChangSeok Oh]

This  bug is for adding initial TextureMapperGL implementation for WebKitGtk+.

[Martin Robinson]

Nayan and I are working on this one.

[ChangSeok Oh]

Oh. Really? I've been doing something on this, too.
So, I just hope to contribute it, if you and Nayan don't mind. :)

[Joone Hur]

(In reply to comment #2)
> Oh. Really? I've been doing something on this, too.
> So, I just hope to contribute it, if you and Nayan don't mind. :)

Hi ChangSeok,

Please take a look at:
http://blog.abandonedwig.info/2011/12/webkitgtk-hackfest-wrapup-accelerated.html

:-)

[Martin Robinson]

Created attachment 124489 [details]
Preliminary patch

[Martin Robinson]

Alex and No'am, would you mind taking a look at my first stab at this? It seems to work. :)

[Noam Rosenthal]

I'm ok with the TextureMapperGL changes!

[Martin Robinson]

(In reply to comment #6)
> I'm ok with the TextureMapperGL changes!

One thing I'm not certain about is the contract with TextureMapperGL about when the appropriate GL context should be active. I found that I needed to make it active when constructing the TextureMapper now, as that's when it creates the shaders.

[Noam Rosenthal]

(In reply to comment #7)
> (In reply to comment #6)
> > I'm ok with the TextureMapperGL changes!
> 
> One thing I'm not certain about is the contract with TextureMapperGL about when the appropriate GL context should be active. I found that I needed to make it active when constructing the TextureMapper now, as that's when it creates the shaders.

Yes, that should be changed, and is part of a patch ostap is working on. The GL context should be valid when running beginPainting(), but that would be true after it's fixed.

[Alejandro G. Castro]

Comment on attachment 124489 [details]
Preliminary patch

Great patch! I tested it and it works well, we can do more extensive testing with
the patch in the repository. I've added some small proposals for your consideration, also
I found compilation does not work if WK2 is activated, I guess we can create a different
patch to fix this compilation issues with texmap now.

I hope this helps.

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

> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:40
> +    RefPtr<cairo_t> cr = adoptRef(cairo_create(m_surface.get()));

Small proposal :), considering we are going to store the cr in the m_context what do you think about using cr directly or storing it initially like:

m_context = adoptPtr(new PlatformContextCairo(cairo_create(m_surface.get())));

Later we could use m_context->cr().

> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:43
> +    cairo_set_operator(cr.get(), CAIRO_OPERATOR_CLEAR);
> +    cairo_paint(cr.get());

Initially the surface create function adds all 0s, is this changing that?

> Source/WebCore/platform/graphics/cairo/TextureMapperGLCairo.cpp:-54
> -    notImplemented();

Would it make sense to call cairo_surface_flush to make sure all the operations are done?

> Source/WebCore/platform/graphics/gtk/WindowGLContextGLX.cpp:33
> +        return nullptr;

Does the style propose to use 0 to represent null pointer?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:55
> +    // GraphicsLayerClient

We have to add a dot at the end of the comment.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:56
> +    virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*, double time);

Shoulswe remove the name of the variable 'time'?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:58
> +    virtual void paintContents(const WebCore::GraphicsLayer*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const WebCore::IntRect& rectToPaint);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:121
> +    if (rect == IntRect())

Could we just do rect.isEmpty()?

> Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:186
> +#endif // USE(ACCELERATED_COMPOSITING)

Nitpick, we have to add && USE(TEXTURE_MAPPER_GL).

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:909
> +    * compositing uses the GPU to render animations on pages smoothly and also allows
> +    * propre rendering of 3D CSS transforms.

propre -> proper

> Source/WebKit/gtk/webkit/webkitwebview.cpp:661
> +        copyRectFromCairoSurfaceToContext(priv->backingStore->cairoSurface(), cr, IntSize(), 

Nitpick :), there is a space at the end of the line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:984
> +    gdk_window_ensure_native(window);

Could we disable AC if the function returns FALSE?

> configure.ac:404
> +if test "$enable_webgl" = "yes" ||  test "$with_accelerated_compositing" != "opengl" ; then

Did you add a ! in the opengl condition?

[Martin Robinson]

Created attachment 125550 [details]
Patch

[Noam Rosenthal]

Comment on attachment 125550 [details]
Patch

Another LGTM for the TextureMapper/LayerTreeHost changes.

[Martin Robinson]

(In reply to comment #9)
> (From update of attachment 124489 [details])
> Great patch! I tested it and it works well, we can do more extensive testing with
> the patch in the repository. I've added some small proposals for your consideration, also
> I found compilation does not work if WK2 is activated, I guess we can create a different
> patch to fix this compilation issues with texmap now.

Thanks for the review. I've fixed the WebKit2 compilation. I also updated the patch to reflect the new TextureMapper design. We'll need a little work in the future to implement TextureMapperImageBuffer and to fall back when necessary.

> > Source/WebCore/platform/graphics/gtk/WindowGLContextGLX.cpp:33
> > +        return nullptr;
> 
> Does the style propose to use 0 to represent null pointer?

nullptr is the new thing from C++0x. I believe that WebKit is switching to it gradually.

> 
> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:55
> > +    // GraphicsLayerClient
> 
> We have to add a dot at the end of the comment.

I think we can avoid it here, since this isn't a complete sentence. There are a few places where you can use one word. For instance, in this case it's just the name of the abstract class I'm implementing.


> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:56
> > +    virtual void notifyAnimationStarted(const WebCore::GraphicsLayer*, double time);
> 
> Shoulswe remove the name of the variable 'time'?

I think it's important to leave it here since double is such a generic type.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContext.h:58
> > +    virtual void paintContents(const WebCore::GraphicsLayer*, WebCore::GraphicsContext&, WebCore::GraphicsLayerPaintingPhase, const WebCore::IntRect& rectToPaint);
> 
> Ditto.

I think it's useful here too. :)

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:121
> > +    if (rect == IntRect())
> 
> Could we just do rect.isEmpty()?

Yep! Fixed.

> > Source/WebKit/gtk/WebCoreSupport/AcceleratedCompositingContextGL.cpp:186
> > +#endif // USE(ACCELERATED_COMPOSITING)
> 
> Nitpick, we have to add && USE(TEXTURE_MAPPER_GL).

Fixed.

> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:909
> > +    * compositing uses the GPU to render animations on pages smoothly and also allows
> > +    * propre rendering of 3D CSS transforms.
> 
> propre -> proper'

Fixed.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:661
> > +        copyRectFromCairoSurfaceToContext(priv->backingStore->cairoSurface(), cr, IntSize(), 
> 
> Nitpick :), there is a space at the end of the line.

Fixed.

> > Source/WebKit/gtk/webkit/webkitwebview.cpp:984
> > +    gdk_window_ensure_native(window);
> 
> Could we disable AC if the function returns FALSE?

Fixed.

> > configure.ac:404
> > +if test "$enable_webgl" = "yes" ||  test "$with_accelerated_compositing" != "opengl" ; then
> 
> Did you add a ! in the opengl condition?

Yeah. :/ Fixed.

[Martin Robinson]

Created attachment 125551 [details]
Patch with WebKit2 ChangeLog

[Martin Robinson]

Created attachment 125591 [details]
Properly handle little endian systems with OpenGL ES

[Alejandro G. Castro]

Comment on attachment 125591 [details]
Properly handle little endian systems with OpenGL ES

LGTM, works great :). I just had to add this code to make it compile. Noam are this ifdefs in DrawingAreaProxyImpl.cpp correct? I did not check much that code.

diff --git a/Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp b/Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
index 4c3447b..ba886a0 100644
--- a/Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
+++ b/Source/WebCore/platform/graphics/cairo/GraphicsContextCairo.cpp
@@ -1139,6 +1139,24 @@ InterpolationQuality GraphicsContext::imageInterpolationQuality() const
     return platformContext()->imageInterpolationQuality();
 }
 
+#if ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER)
+TransformationMatrix GraphicsContext::get3DTransform() const
+{
+    notImplemented();
+    return TransformationMatrix();
+}
+
+void GraphicsContext::concat3DTransform(const TransformationMatrix& transform)
+{
+    notImplemented();
+}
+
+void GraphicsContext::set3DTransform(const TransformationMatrix& transform)
+{
+    notImplemented();
+}
+#endif
+
 } // namespace WebCore
 
 #endif // USE(CAIRO)
diff --git a/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp b/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
index 24a5aed..ffc60cf 100644
--- a/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
+++ b/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
@@ -36,7 +36,7 @@
 #include "WebProcessProxy.h"
 #include <WebCore/Region.h>
 
-#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER)
+#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
 #include "LayerTreeHostProxy.h"
 #endif
 
@@ -58,7 +58,7 @@ DrawingAreaProxyImpl::DrawingAreaProxyImpl(WebPageProxy* webPageProxy)
     , m_isBackingStoreDiscardable(true)
     , m_discardBackingStoreTimer(RunLoop::current(), this, &DrawingAreaProxyImpl::discardBackingStore)
 {
-#if USE(TEXTURE_MAPPER)
+#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
     // Construct the proxy early to allow messages to be sent to the web process while AC is entered there.
     if (webPageProxy->pageGroup()->preferences()->forceCompositingMode())
         m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
@@ -335,7 +335,7 @@ void DrawingAreaProxyImpl::enterAcceleratedCompositingMode(const LayerTreeContex
     m_backingStore = nullptr;
     m_layerTreeContext = layerTreeContext;
     m_webPageProxy->enterAcceleratedCompositingMode(layerTreeContext);
-#if USE(TEXTURE_MAPPER)
+#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
     if (!m_layerTreeHostProxy)
         m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
 #endif

[Gustavo Noronha (kov)]

Comment on attachment 125591 [details]
Properly handle little endian systems with OpenGL ES

Re API: simple and effective, looks great to me.

[Martin Robinson]

(In reply to comment #15)

> diff --git a/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp b/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
> index 24a5aed..ffc60cf 100644
> --- a/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
> +++ b/Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp
> @@ -36,7 +36,7 @@
>  #include "WebProcessProxy.h"
>  #include <WebCore/Region.h>
> 
> -#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER)
> +#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
>  #include "LayerTreeHostProxy.h"
>  #endif
> 
> @@ -58,7 +58,7 @@ DrawingAreaProxyImpl::DrawingAreaProxyImpl(WebPageProxy* webPageProxy)
>      , m_isBackingStoreDiscardable(true)
>      , m_discardBackingStoreTimer(RunLoop::current(), this, &DrawingAreaProxyImpl::discardBackingStore)
>  {
> -#if USE(TEXTURE_MAPPER)
> +#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
>      // Construct the proxy early to allow messages to be sent to the web process while AC is entered there.
>      if (webPageProxy->pageGroup()->preferences()->forceCompositingMode())
>          m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
> @@ -335,7 +335,7 @@ void DrawingAreaProxyImpl::enterAcceleratedCompositingMode(const LayerTreeContex
>      m_backingStore = nullptr;
>      m_layerTreeContext = layerTreeContext;
>      m_webPageProxy->enterAcceleratedCompositingMode(layerTreeContext);
> -#if USE(TEXTURE_MAPPER)
> +#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
>      if (!m_layerTreeHostProxy)
>          m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
>  #endif

I don't seem to need this bit to build. What options are you passing to build-webkit?

[Martin Robinson]

Committed r106901: <http://trac.webkit.org/changeset/106901>

[Alejandro G. Castro]

(In reply to comment #17)
> (In reply to comment #15)
> 
> [...]
>
> > +#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
> >      if (!m_layerTreeHostProxy)
> >          m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
> >  #endif
> 
> I don't seem to need this bit to build. What options are you passing to build-webkit?

Hrm, basically no options: build-webkit --gtk --debug --prefix=/blah. I'll update the repository to check if it works for me.

[Alejandro G. Castro]

(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #15)
> > 
> > [...]
> >
> > > +#if USE(TEXTURE_MAPPER) && USE(TILED_BACKING_STORE)
> > >      if (!m_layerTreeHostProxy)
> > >          m_layerTreeHostProxy = adoptPtr(new LayerTreeHostProxy(this));
> > >  #endif
> > 
> > I don't seem to need this bit to build. What options are you passing to build-webkit?
> 
> Hrm, basically no options: build-webkit --gtk --debug --prefix=/blah. I'll update the repository to check if it works for me.

I've just tested it and it still fails for me in linking stage:

./.libs/libwebkit2gtk-3.0.so: undefined reference to `WebKit::LayerTreeHostProxy::LayerTreeHostProxy(WebKit::DrawingAreaProxy*)'
collect2: ld returned 1 exit status

The compilation line is:

build-webkit --gtk --debug --prefix=/home/alex/install --with-accelerated-compositing=opengl

I guess it makes sense because we are compiling DrawingAreaProxyImpl.cpp and it uses LayerTreeHostProxy that it is not compiled.