Bug 75308

Nayan and I are working on this one.

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

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

Created attachment 124489 [details] Preliminary patch

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

I'm ok with the TextureMapperGL changes!

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

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

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?

Created attachment 125550 [details] Patch

Comment on attachment 125550 [details] Patch Another LGTM for the TextureMapper/LayerTreeHost changes.

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

Created attachment 125551 [details] Patch with WebKit2 ChangeLog

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

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

Comment on attachment 125591 [details] Properly handle little endian systems with OpenGL ES Re API: simple and effective, looks great to me.

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

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

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

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