Bug 82336 - Make it possible to use accelerated compositing for page overlay fading
: Make it possible to use accelerated compositing for page overlay fading
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-03-27 07:20 PST by
Modified: 2013-08-01 15:00 PST (History)


Attachments
Patch (9.08 KB, patch)
2012-03-27 07:48 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (10.46 KB, patch)
2012-03-27 08:37 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.83 KB, patch)
2012-03-28 02:36 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.55 KB, patch)
2012-03-28 07:57 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.38 KB, patch)
2012-03-28 08:04 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.16 KB, patch)
2012-03-28 09:06 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.14 KB, patch)
2012-03-29 04:48 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.18 KB, patch)
2012-04-11 05:30 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.54 KB, patch)
2012-04-11 06:00 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.53 KB, patch)
2012-04-26 05:49 PST, Lars Knudsen
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-03-27 07:20:25 PST
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.
------- Comment #1 From 2012-03-27 07:48:22 PST -------
Created an attachment (id=134061) [details]
Patch
------- Comment #2 From 2012-03-27 08:01:48 PST -------
(From update of attachment 134061 [details])
Attachment 134061 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12146499
------- Comment #3 From 2012-03-27 08:13:37 PST -------
(From update of attachment 134061 [details])
Attachment 134061 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12142546
------- Comment #4 From 2012-03-27 08:18:25 PST -------
(From update of attachment 134061 [details])
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?
------- Comment #5 From 2012-03-27 08:21:31 PST -------
(From update of attachment 134061 [details])
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.
------- Comment #6 From 2012-03-27 08:37:23 PST -------
Created an attachment (id=134074) [details]
Patch
------- Comment #7 From 2012-03-27 09:02:26 PST -------
(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.

> 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.
------- Comment #8 From 2012-03-27 09:48:37 PST -------
(In reply to comment #7)
> (From update of attachment 134074 [details] [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.
------- Comment #9 From 2012-03-27 10:12:14 PST -------
(From update of attachment 134074 [details])
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.
------- Comment #10 From 2012-03-28 02:36:53 PST -------
Created an attachment (id=134244) [details]
Patch
------- Comment #11 From 2012-03-28 05:41:14 PST -------
(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.
------- Comment #12 From 2012-03-28 07:57:04 PST -------
Created an attachment (id=134296) [details]
Patch
------- Comment #13 From 2012-03-28 07:58:01 PST -------
(In reply to comment #11)
> (From update of attachment 134244 [details] [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 :-)
------- Comment #14 From 2012-03-28 08:04:50 PST -------
Created an attachment (id=134297) [details]
Patch
------- Comment #15 From 2012-03-28 08:30:36 PST -------
(From update of attachment 134297 [details])
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.
------- Comment #16 From 2012-03-28 08:59:31 PST -------
(From update of attachment 134297 [details])
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.
------- Comment #17 From 2012-03-28 09:06:45 PST -------
Created an attachment (id=134309) [details]
Patch
------- Comment #18 From 2012-03-29 04:48:30 PST -------
Created an attachment (id=134549) [details]
Patch
------- Comment #19 From 2012-03-29 07:05:56 PST -------
(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.

> 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.
------- Comment #20 From 2012-03-29 07:22:44 PST -------
(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.

> 
> you can leave the member initialization intact.
------- Comment #21 From 2012-03-29 07:32:09 PST -------
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 134549 [details] [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.
------- Comment #22 From 2012-03-29 07:45:06 PST -------
(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.
------- Comment #23 From 2012-04-11 05:30:58 PST -------
Created an attachment (id=136661) [details]
Patch
------- Comment #24 From 2012-04-11 05:39:57 PST -------
(From update of attachment 136661 [details])
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.
------- Comment #25 From 2012-04-11 06:00:22 PST -------
Created an attachment (id=136663) [details]
Patch
------- Comment #26 From 2012-04-13 10:38:26 PST -------
andersca, any thoughts/issues with this?
Otherwise I'd rather r+ it and we can fix issues later, it seems good enough to me.
------- Comment #27 From 2012-04-17 09:20:25 PST -------
(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?

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

The getter should probably be const? :)
------- Comment #28 From 2012-04-17 15:23:35 PST -------
(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 :)
------- Comment #29 From 2012-04-25 06:19:06 PST -------
(From update of attachment 136663 [details])
Please fix const comment from Simon before committing.
------- Comment #30 From 2012-04-26 05:49:15 PST -------
Created an attachment (id=138983) [details]
Patch
------- Comment #31 From 2012-04-26 07:05:58 PST -------
(From update of attachment 138983 [details])
Clearing flags on attachment: 138983

Committed r115310: <http://trac.webkit.org/changeset/115310>
------- Comment #32 From 2012-04-26 07:06:06 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #33 From 2013-08-01 15:00:12 PST -------
(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 136663 [details] [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