Bug 82336 – Make it possible to use accelerated compositing for page overlay fading

• Component: WebKit2
• Status: RESOLVED FIXED

[Lars Knudsen]

Currently, the page overlay fading is done by redrawing each frame in the fade animation.  This can cause some flickering and uneven fade animation flow, especially on CPU limited hardware. 

We would like a solution that utilizes accelerated compositing for the fade animation, where possible, to offload this task to the GPU, while preserving the original fade mechanism (with redraws), where AC is not available.

[Lars Knudsen]

Created attachment 134061 [details]
Patch

[Build Bot]

Comment on attachment 134061 [details]
Patch

Attachment 134061 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12146499

[Build Bot]

Comment on attachment 134061 [details]
Patch

Attachment 134061 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12142546

[Kenneth Rohde Christiansen]

Comment on attachment 134061 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +        Page overlay fading will use AC where possible.  Otherwise, it will 
> +        fall back to the previous method of redrawing each frame in the animation.

That is available on the mac platform for instance, but that doesn't mean that Mac folks wants to use it

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:92
> +    if (m_webPage) {
> +        if (!m_webPage->drawingArea()->setPageOverlayOpacity(m_fractionFadedIn))

huh? why not merge those two?

[Kenneth Rohde Christiansen]

Comment on attachment 134061 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:110
>      GraphicsContextStateSaver stateSaver(graphicsContext);
> -    graphicsContext.beginTransparencyLayer(1);
> +    graphicsContext.beginTransparencyLayer(m_webPage->drawingArea()->pageOverlayDrawOpacity());

If this code replaces the pageOverlay->fractionFadedIn(); code in TapHighlightController, then could it also do so for the FindController? At least a commetn about it in the ChangeLog would be good.

[Lars Knudsen]

Created attachment 134074 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 134074 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:79
> +    virtual float pageOverlayDrawOpacity() const { return m_pageOverlayDrawOpacity; }

This default implementation should return a default value of 1, and the member variable plus its getter should be moved down to DrawingAreaImpl.

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:109
> -    graphicsContext.beginTransparencyLayer(1);
> +    graphicsContext.beginTransparencyLayer(m_webPage->drawingArea()->pageOverlayDrawOpacity());
>      graphicsContext.setCompositeOperation(CompositeCopy);

Make sure that you test this code, hopefully on a Safari build, from what I'm seeing it might not blend correctly since you are still using the CompositeCopy mode.
This kind of code is easy to break and I wouldn't push this until we know we don't break their stuff, both in AC and non-AC mode.

[Lars Knudsen]

(In reply to comment #7)
> (From update of attachment 134074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134074&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:79
> > +    virtual float pageOverlayDrawOpacity() const { return m_pageOverlayDrawOpacity; }
> 
> This default implementation should return a default value of 1, and the member variable plus its getter should be moved down to DrawingAreaImpl.

ok

> 
> > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:109
> > -    graphicsContext.beginTransparencyLayer(1);
> > +    graphicsContext.beginTransparencyLayer(m_webPage->drawingArea()->pageOverlayDrawOpacity());
> >      graphicsContext.setCompositeOperation(CompositeCopy);
> 
> Make sure that you test this code, hopefully on a Safari build, from what I'm seeing it might not blend correctly since you are still using the CompositeCopy mode.
> This kind of code is easy to break and I wouldn't push this until we know we don't break their stuff, both in AC and non-AC mode.

I need to get a mac then - or someone with a mac to try it out.

[Noam Rosenthal]

Comment on attachment 134074 [details]
Patch

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

The Qt parts are good, but this patch would modify behavior of WebKit2 on CA, which I'm not comfortable with.

> Source/WebKit2/WebProcess/WebPage/DrawingArea.cpp:57
> +    , m_pageOverlayDrawOpacity(1.0)

I don't want this variable at all. Use layer opacity when LayerTreeHost returns true, use regular fade in when it doesn't (for now).

>>> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:109
>>>      graphicsContext.setCompositeOperation(CompositeCopy);
>> 
>> Make sure that you test this code, hopefully on a Safari build, from what I'm seeing it might not blend correctly since you are still using the CompositeCopy mode.
>> This kind of code is easy to break and I wouldn't push this until we know we don't break their stuff, both in AC and non-AC mode.
> 
> I need to get a mac then - or someone with a mac to try it out.

Testing would be good, but the code should be written in a way that it's obvious that behavior on CA is not changed. 
The transparency layer should always be (1), and in Qt we use it to change the overlayLayer and return true

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:102
> +        context.setFillColor(m_color, ColorSpaceSRGB);

Please use the regular fadeIn if setOverlayOpacity returned false.

> Source/WebKit2/WebProcess/WebPage/ca/LayerTreeHostCA.cpp:190
> +void LayerTreeHostCA::setPageOverlayOpacity(float value)
> +{
> +    ASSERT(m_pageOverlayLayer);
> +    m_pageOverlayLayer->setOpacity(value);
> +    scheduleLayerFlush();
> +}

This should return false and do nothing by default. This patch should not change the CA implementation.

[Lars Knudsen]

Created attachment 134244 [details]
Patch

[Noam Rosenthal]

Comment on attachment 134244 [details]
Patch

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

This would still affect implementations that use LayerTreeHost and DrawingAreaImpl.
LayerTreeHost::setPageOverlayOpacity is where the difference should occur.

> Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:78
> +    virtual void setPageOverlayOpacity(float) = 0;

This function should return bool, and return false by default.

[Lars Knudsen]

Created attachment 134296 [details]
Patch

[Lars Knudsen]

(In reply to comment #11)
> (From update of attachment 134244 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134244&action=review
> 
> This would still affect implementations that use LayerTreeHost and DrawingAreaImpl.
> LayerTreeHost::setPageOverlayOpacity is where the difference should occur.
> 
> > Source/WebKit2/WebProcess/WebPage/LayerTreeHost.h:78
> > +    virtual void setPageOverlayOpacity(float) = 0;
> 
> This function should return bool, and return false by default.


Seems sane - fixed, please check :-)

[Lars Knudsen]

Created attachment 134297 [details]
Patch

[Noam Rosenthal]

Comment on attachment 134297 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:91
> +    if (m_webPage && !m_webPage->drawingArea()->setPageOverlayOpacity(m_fractionFadedIn))

This test should happen in PageOverlay::fadeAnimationTimerFired, not in setNeedsDisplay. Would make the whole code a lot more readable.
That way, if set...Opacity returns true, we don't even have to change fractionFadedIn at all.

> Source/WebKit2/WebProcess/WebPage/TapHighlightController.cpp:107
> +        if (m_webPage->drawingArea()->layerTreeHost())

This is wrong.

[Simon Fraser (smfr)]

Comment on attachment 134297 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:78
> +    virtual bool setPageOverlayOpacity(float) { return false; }

This needs a comment to say what the return value means.

[Lars Knudsen]

Created attachment 134309 [details]
Patch

[Lars Knudsen]

Created attachment 134549 [details]
Patch

[Noam Rosenthal]

Comment on attachment 134549 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:78
> +    // Return false if accelerated compositing should not be used for overlay opacity.

I'd say "If this function returns false, PageOverlay should apply opacity when painting.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:231
> +    scheduleCompositingLayerSync();

Why is this needed?

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:-127
> -    m_fractionFadedIn = 0.0;

m_fractionFadeIn would have an undefined value before the animation.

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:162
> +    if (!m_webPage->drawingArea()->setPageOverlayOpacity(fractionFadedIn)) {
> +        m_fractionFadedIn = fractionFadedIn;
> +        setNeedsDisplay();
> +    }

if you add this:
} else {
    // Opacity is handled externally, in DrawingArea or LayerTreeHost.
    m_fractionFadeIn = 1;
}

you can leave the member initialization intact.

[Lars Knudsen]

(In reply to comment #19)
> (From update of attachment 134549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134549&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:78
> > +    // Return false if accelerated compositing should not be used for overlay opacity.
> 
> I'd say "If this function returns false, PageOverlay should apply opacity when painting.

ok - sounds fine.

> 
> > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:231
> > +    scheduleCompositingLayerSync();
> 
> Why is this needed?

Might not be here - I'll do some testing without

> 
> > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:-127
> > -    m_fractionFadedIn = 0.0;
> 
> m_fractionFadeIn would have an undefined value before the animation.

It's defined in the constructor.

> 
> > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:162
> > +    if (!m_webPage->drawingArea()->setPageOverlayOpacity(fractionFadedIn)) {
> > +        m_fractionFadedIn = fractionFadedIn;
> > +        setNeedsDisplay();
> > +    }
> 
> if you add this:
> } else {
>     // Opacity is handled externally, in DrawingArea or LayerTreeHost.
>     m_fractionFadeIn = 1;
> }

The problem here is that setNeedsDisplay seems to be called before (actually - 4 times, we need to look into that) the timer event fires.  If I leave the initialization up to startFade[In|Out]Animation - then for 4 repaints, it's set to the wrong value - making the paint do opacity = 0 => nothing visible for AC opacity transform.

> 
> you can leave the member initialization intact.

[Lars Knudsen]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 134549 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134549&action=review
> > 
> > > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:78
> > > +    // Return false if accelerated compositing should not be used for overlay opacity.
> > 
> > I'd say "If this function returns false, PageOverlay should apply opacity when painting.
> 
> ok - sounds fine.
> 
> > 
> > > Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:231
> > > +    scheduleCompositingLayerSync();
> > 
> > Why is this needed?
> 
> Might not be here - I'll do some testing without
> 
> > 
> > > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:-127
> > > -    m_fractionFadedIn = 0.0;
> > 
> > m_fractionFadeIn would have an undefined value before the animation.
> 
> It's defined in the constructor.
> 
> > 
> > > Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:162
> > > +    if (!m_webPage->drawingArea()->setPageOverlayOpacity(fractionFadedIn)) {
> > > +        m_fractionFadedIn = fractionFadedIn;
> > > +        setNeedsDisplay();
> > > +    }
> > 
> > if you add this:
> > } else {
> >     // Opacity is handled externally, in DrawingArea or LayerTreeHost.
> >     m_fractionFadeIn = 1;
> > }
> 
> The problem here is that setNeedsDisplay seems to be called before (actually - 4 times, we need to look into that) the timer event fires.  If I leave the initialization up to startFade[In|Out]Animation - then for 4 repaints, it's set to the wrong value - making the paint do opacity = 0 => nothing visible for AC opacity transform.

Actually - here is a printout of the sequence of 2 "tap-hold-release" cycles (all in PageOverlay.cpp):

Click 1:

PageOverlay
setPage
startFadeInAnimation
startFadeAnimation
setNeedsDisplay
drawRect
drawRect
drawRect
drawRect
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
startFadeOutAnimation
startFadeAnimation
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
setPage

Click 2:

PageOverlay
setPage
startFadeInAnimation
startFadeAnimation
setNeedsDisplay
drawRect
drawRect
drawRect
drawRect
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
startFadeOutAnimation
startFadeAnimation
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
fadeAnimationTimerFired
setPage


> 
> > 
> > you can leave the member initialization intact.

[Noam Rosenthal]

(In reply to comment #21)

OK, so it's not setNeedsDisplay that's called several times, but rather drawRect, which can also be called if the actual contents are re-rendered (which is what's probably happening).

The way we could solve it, is by changing the API a bit;
- In DrawingArea, we can do: bool shouldApplyOverlayFadeWhenPainting();
- in the timer, if the above function returned false, we simply call scheduleLayerFlush instead of setNeedsDisplay.
- In LayerTreeHostQt, we set the layer's opacity to overlay->fractionFadedIn.
- When painting the overlay, we only apply m_fractionFadeIn if shouldApplyOverlayFadeWhenPainting returned true.

[Lars Knudsen]

Created attachment 136661 [details]
Patch

[Noam Rosenthal]

Comment on attachment 136661 [details]
Patch

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

Much better. Minor nitpicks, though I want smfr or andersca to make sure this is fine with them.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.h:64
> +    virtual bool shouldApplyOverlayFadeWhenPainting();

Let's rename to pageOverlayShouldApplyFadeWhenPainting

> Source/WebKit2/WebProcess/WebPage/PageOverlay.cpp:165
> +        m_webPage->drawingArea()->layerTreeHost()->setPageOverlayOpacity(m_fractionFadedIn);

I don't like the dependency on LayerTreeHost, let's add setPageOverlayOpacity to DrawingArea and patch it through there.

[Lars Knudsen]

Created attachment 136663 [details]
Patch

[Noam Rosenthal]

andersca, any thoughts/issues with this?
Otherwise I'd rather r+ it and we can fix issues later, it seems good enough to me.

[Simon Hausmann]

Comment on attachment 136663 [details]
Patch

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

I wonder if we should bother with supporting page overlays without AC. Why not make AC a requirement if you want to use page overlays in your WK2 port?

> Source/WebKit2/WebProcess/WebPage/DrawingArea.h:80
> +    virtual bool pageOverlayShouldApplyFadeWhenPainting() { return true; }

The getter should probably be const? :)

[Noam Rosenthal]

(In reply to comment #27)
> (From update of attachment 136663 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136663&action=review
> 
> I wonder if we should bother with supporting page overlays without AC. Why not make AC a requirement if you want to use page overlays in your WK2 port?
Maybe later... let's get it working before making it a requirement :)

[Noam Rosenthal]

Comment on attachment 136663 [details]
Patch

Please fix const comment from Simon before committing.

[Lars Knudsen]

Created attachment 138983 [details]
Patch

[WebKit Review Bot]

Comment on attachment 138983 [details]
Patch

Clearing flags on attachment: 138983

Committed r115310: <http://trac.webkit.org/changeset/115310>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 136663 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=136663&action=review
> > 
> > I wonder if we should bother with supporting page overlays without AC. Why not make AC a requirement if you want to use page overlays in your WK2 port?
> Maybe later... let's get it working before making it a requirement :)

I'm going to make it a requirement: https://bugs.webkit.org/show_bug.cgi?id=119411