Bug 137345 – [WinCairo] Accelerated compositing is not implemented.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

This patch is based on GTK, and the texture mapper graphics layer type.

Created attachment 239116 [details]
Patch

(In reply to comment #1)
> Created an attachment (id=239116) [details]
> Patch

This patch is based on the GTK implementation of accelerated compositing.

Created attachment 239531 [details]
Patch

(In reply to comment #3)
> Created an attachment (id=239531) [details]
> Patch

Removed an unneeded graphics layer member in WebView for WinCairo.

Created attachment 239622 [details]
Patch

(In reply to comment #5)
> Created an attachment (id=239622) [details]
> Patch

Fixed a repaint issue in compositing mode.

Created attachment 240010 [details]
Patch

(In reply to comment #7)
> Created attachment 240010 [details]
> Patch

Removed a couple of empty methods.

[Brent Fulgham]

Comment on attachment 240010 [details]
Patch

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

Really nice work! Are you able to activate any o the 3D CSS effects with this turned on?

I had a few minor comments and things to clean up before landing (so cq-) but I think this is pretty much ready to go!

> Source/WebKit/win/WebView.cpp:1110
>              // FIXME: We need to paint into dc (if provided). <http://webkit.org/b/52578>

I'd move this comment inside the #if USE(CA), since it only really applies to the CoreAnimation builds.

> Source/WebKit/win/WebView.cpp:6802
>      if (m_backingLayer)

Whoops (on us!)! This m_backingLayer test is not necessary (see line 6755/6794 above).

> Source/WebKit/win/WebView.h:51
> +#include "AcceleratedCompositingContext.h"

Could this just be a forward declaration?

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:2
> + * Copyright (C) 2014 Apple, Inc.

Shouldn't this be copyright you? If you based it off an Apple copyrighted file, just put both names.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:61
> +static IntSize getWebViewSize(WebView* webView)

Can WebView ever be null? If so, check for that case. If not, let's make it a reference.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:80
> +    m_rootLayer = GraphicsLayer::create(0, *this);

nullptr please.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:85
> +    m_nonCompositedContentLayer = GraphicsLayer::create(0, *this);

Ditto.

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:164
> +    ::GetClientRect(m_window, &r);

GetClientRect can fail, leaving junk the the passed RECT. You might want to handle this case (though it's very rare).

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:170
> +        glClear(GL_COLOR_BUFFER_BIT);

Watch the tests to see if you run afoul of any cases where GL_SCISSOR or other switches are turned on. You might need to use the "TemporaryOpenGLSetting" class to make sure they are turned off when clearing the context (or else you'll get weird partial clears):

E.g., from GraphicsContext3D::prepareTexture:

    TemporaryOpenGLSetting scopedScissor(GL_SCISSOR_TEST, GL_FALSE);
    TemporaryOpenGLSetting scopedDither(GL_DITHER, GL_FALSE);

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.h:2
> + * Copyright (C) 2014 Apple, Inc.

Ditto re: Copyright by you.

[Brent Fulgham]

Comment on attachment 240010 [details]
Patch

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

> Source/WebKit/win/WebView.cpp:869
> +        m_acceleratedCompositingContext->setNeedsDisplayInRect(dirtyRect);

It's too bad that the stuff in the AcceleratedCompositingContext couldn't be part of m_backingLayer. But looking over the GraphicsContext class it would probably be tough to merge the two. Might be an opportunity for some refactoring later.

Created attachment 240025 [details]
Patch

(In reply to comment #9)
> Comment on attachment 240010 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240010&action=review
> 

Thanks for reviewing!

> Really nice work! Are you able to activate any o the 3D CSS effects with
> this turned on?
> 

Yes, I believe so.
For example, http://www.w3.org/Talks/2012/0416-CSS-WWW2012/Demos/transitions/demo-transitions-3-shadows.html seems to be working now :)

> > Source/WebKit/win/WebView.cpp:6802
> >      if (m_backingLayer)
> 
> Whoops (on us!)! This m_backingLayer test is not necessary (see line
> 6755/6794 above).
> 

This is actually needed, as the previous call might take us out of compositing mode, and set m_backingLayer to null.

> > Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:2
> > + * Copyright (C) 2014 Apple, Inc.
> 
> Shouldn't this be copyright you? If you based it off an Apple copyrighted
> file, just put both names.
> 

If this is not too important, I left it as is; I wouldn't mind :)

Created attachment 240028 [details]
Patch

[Brent Fulgham]

Comment on attachment 240028 [details]
Patch

Looks great!  Thanks.

[WebKit Commit Bot]

Comment on attachment 240028 [details]
Patch

Clearing flags on attachment: 240028

Committed r174830: <http://trac.webkit.org/changeset/174830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

https://www.webkit.org/blog-files/3d-transforms/poster-circle.html works, too!  This is awesome

[Zan Dobersek]

Comment on attachment 240028 [details]
Patch

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

> Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:317
> +    // In case an animation is running, we should flush again soon.
> +    if (startedAnimation(m_rootLayer.get()))
> +        scheduleLayerFlush();

Any reason for not using TextureMapperLayer::descendantsOrSelfHaveRunningAnimations() for this? That's what the GTK port uses to determine whether to additionally schedule flushes for animations.

I this this current approach is susceptible to continuing scheduling flushes even after an animation is already finished.

(In reply to comment #18)
> Comment on attachment 240028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240028&action=review
> 
> > Source/WebKit/win/WebCoreSupport/AcceleratedCompositingContext.cpp:317
> > +    // In case an animation is running, we should flush again soon.
> > +    if (startedAnimation(m_rootLayer.get()))
> > +        scheduleLayerFlush();
> 
> Any reason for not using
> TextureMapperLayer::descendantsOrSelfHaveRunningAnimations() for this?
> That's what the GTK port uses to determine whether to additionally schedule
> flushes for animations.
> 
> I this this current approach is susceptible to continuing scheduling flushes
> even after an animation is already finished.

Thanks for catching this :) I'll have a look, and see if we can use it.