Bug 82336 - Make it possible to use accelerated compositing for page overlay fading
Summary: Make it possible to use accelerated compositing for page overlay fading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Lars Knudsen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-27 07:20 PDT by Lars Knudsen
Modified: 2013-08-01 15:00 PDT (History)
11 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Lars Knudsen 2012-03-27 07:20:25 PDT
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 Lars Knudsen 2012-03-27 07:48:22 PDT
Created attachment 134061 [details]
Patch
Comment 2 Build Bot 2012-03-27 08:01:48 PDT
Comment on attachment 134061 [details]
Patch

Attachment 134061 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12146499
Comment 3 Build Bot 2012-03-27 08:13:37 PDT
Comment on attachment 134061 [details]
Patch

Attachment 134061 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12142546
Comment 4 Kenneth Rohde Christiansen 2012-03-27 08:18:25 PDT
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?
Comment 5 Kenneth Rohde Christiansen 2012-03-27 08:21:31 PDT
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.
Comment 6 Lars Knudsen 2012-03-27 08:37:23 PDT
Created attachment 134074 [details]
Patch
Comment 7 Jocelyn Turcotte 2012-03-27 09:02:26 PDT
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.
Comment 8 Lars Knudsen 2012-03-27 09:48:37 PDT
(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.
Comment 9 Noam Rosenthal 2012-03-27 10:12:14 PDT
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.
Comment 10 Lars Knudsen 2012-03-28 02:36:53 PDT
Created attachment 134244 [details]
Patch
Comment 11 Noam Rosenthal 2012-03-28 05:41:14 PDT
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.
Comment 12 Lars Knudsen 2012-03-28 07:57:04 PDT
Created attachment 134296 [details]
Patch
Comment 13 Lars Knudsen 2012-03-28 07:58:01 PDT
(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 :-)
Comment 14 Lars Knudsen 2012-03-28 08:04:50 PDT
Created attachment 134297 [details]
Patch
Comment 15 Noam Rosenthal 2012-03-28 08:30:36 PDT
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.
Comment 16 Simon Fraser (smfr) 2012-03-28 08:59:31 PDT
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.
Comment 17 Lars Knudsen 2012-03-28 09:06:45 PDT
Created attachment 134309 [details]
Patch
Comment 18 Lars Knudsen 2012-03-29 04:48:30 PDT
Created attachment 134549 [details]
Patch
Comment 19 Noam Rosenthal 2012-03-29 07:05:56 PDT
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.
Comment 20 Lars Knudsen 2012-03-29 07:22:44 PDT
(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.
Comment 21 Lars Knudsen 2012-03-29 07:32:09 PDT
(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.
Comment 22 Noam Rosenthal 2012-03-29 07:45:06 PDT
(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 Lars Knudsen 2012-04-11 05:30:58 PDT
Created attachment 136661 [details]
Patch
Comment 24 Noam Rosenthal 2012-04-11 05:39:57 PDT
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.
Comment 25 Lars Knudsen 2012-04-11 06:00:22 PDT
Created attachment 136663 [details]
Patch
Comment 26 Noam Rosenthal 2012-04-13 10:38:26 PDT
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 Simon Hausmann 2012-04-17 09:20:25 PDT
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? :)
Comment 28 Noam Rosenthal 2012-04-17 15:23:35 PDT
(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 :)
Comment 29 Noam Rosenthal 2012-04-25 06:19:06 PDT
Comment on attachment 136663 [details]
Patch

Please fix const comment from Simon before committing.
Comment 30 Lars Knudsen 2012-04-26 05:49:15 PDT
Created attachment 138983 [details]
Patch
Comment 31 WebKit Review Bot 2012-04-26 07:05:58 PDT
Comment on attachment 138983 [details]
Patch

Clearing flags on attachment: 138983

Committed r115310: <http://trac.webkit.org/changeset/115310>
Comment 32 WebKit Review Bot 2012-04-26 07:06:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Tim Horton 2013-08-01 15:00:12 PDT
(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