Bug 149060 - [ThreadedCompositor] Scrolling artifacts on accelerated subframes
Summary: [ThreadedCompositor] Scrolling artifacts on accelerated subframes
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2015-09-11 03:38 PDT by Emanuele Aina
Modified: 2016-09-17 01:34 PDT (History)
5 users (show)

See Also:

Accelerated subframe testcase (1.83 KB, text/html)
2015-09-11 03:38 PDT, Emanuele Aina
no flags Details
Patch (2.39 KB, patch)
2015-09-11 03:39 PDT, Emanuele Aina
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2016-09-15 03:35 PDT, Emanuele Aina
cgarcia: review-
cgarcia: commit-queue-
Details | Formatted Diff | Diff
Patch (1.68 KB, patch)
2016-09-16 04:39 PDT, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emanuele Aina 2015-09-11 03:38:18 PDT
Created attachment 261000 [details]
Accelerated subframe testcase

When scrolling an accelerated subframe (eg. an <iframe> containing a transformed element, see the attached scroll.html file) the main frame is scrolled temporarily producing ugly glitches.

This seems to due to excessive scroll notifications where the lower layers notify the upper layers again.

I guess an automated testcase would be appropriate, but I don't know where to start, any suggestion would be appropriate.
Comment 1 Emanuele Aina 2015-09-11 03:39:58 PDT
Created attachment 261001 [details]
Comment 2 Michael Catanzaro 2016-01-27 21:03:58 PST
Yoon, what do you think?
Comment 3 Zan Dobersek 2016-02-16 03:43:08 PST
Comment on attachment 261001 [details]

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

This needs some clarification. Yoon should chip in as well.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:126
>  void ThreadedCoordinatedLayerTreeHost::scrollNonCompositedContents(const WebCore::IntRect& rect)

The parameter is now unused.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:-128
> -    m_compositor->scrollTo(rect.location());

After removing this, ThreadedCompositor::scrollTo() is not called anymore. Do we still need it?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:-242
> -    if (m_lastScrollPosition != roundedIntPoint(rect.location())) {
> -        m_lastScrollPosition = roundedIntPoint(rect.location());
> -
> -        if (!m_webPage->corePage()->mainFrame().view()->useFixedLayout())
> -            m_webPage->corePage()->mainFrame().view()->notifyScrollPositionChanged(m_lastScrollPosition);
> -    }
> -
> -    if (m_lastScaleFactor != scale) {
> -        m_lastScaleFactor = scale;
> -        didScaleFactorChanged(m_lastScaleFactor, m_lastScrollPosition);
> -    }

Same for m_lastScrollPosition and m_lastScaleFactor -- these are unused afterwards.
Comment 4 Carlos Garcia Campos 2016-06-14 04:04:27 PDT
I don't see any differences with the attached test case between threaded compositor enabled and disabled.
Comment 5 Michael Catanzaro 2016-07-20 17:44:10 PDT
Emanuele, any update on this?
Comment 6 Emanuele Aina 2016-09-15 02:18:06 PDT
I still see the page jumping badly when trying to scroll down https://people.collabora.com/~em/wk/tests/scroll.html

My steps (under X11):

* ./MiniBrowser https://people.collabora.com/~em/wk/tests/scroll.html
* move mouse pointer over the top iframe with the 300x300 image placeholder
* scroll down with a double-finger drag on my touchpad
* iframe scrolls correctly
* when the iframe has been scrolled completely, the scroll events propagate to the main frame, scrolling the page correctly
* if I keep scrolling down at some point the page goes back to its zero scrolling position
Comment 7 Emanuele Aina 2016-09-15 02:19:54 PDT
Tested on r205858/git 804075239217.
Comment 8 Emanuele Aina 2016-09-15 02:20:30 PDT
I haven't re-tested the patch.
Comment 9 Emanuele Aina 2016-09-15 03:17:22 PDT
It seems that the jump happens as soon as the mouse pointer enters the second iframe during the scroll gesture.
Comment 10 Emanuele Aina 2016-09-15 03:20:00 PDT
Oh, right, that's what I meant in the original report when I was talking about iframes, the scrollable placeholder on top isn't an iframe... :/

In fact, touch-scrolling in the nested iframe produces the flashes due to the page being temporarily scrolled to 0,0 and then back to the right offset.
Comment 11 Emanuele Aina 2016-09-15 03:35:49 PDT
Created attachment 288948 [details]
Comment 12 Emanuele Aina 2016-09-15 03:37:10 PDT
The updated patch seems to fix the issue on my testcase. I'm not sure if it breaks anything else though. :)
Comment 13 Emanuele Aina 2016-09-15 04:57:22 PDT
For what is worth, the one I removed was the only use of SimpleViewportController::didScroll(), so I tried to drop it as well and things still seem to work.

I now need to check where SimpleViewportController::visibleContentsRect() is called, as it uses the value updated by didScroll().
Comment 14 Carlos Garcia Campos 2016-09-16 04:36:13 PDT
Comment on attachment 288948 [details]

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

> w/Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:82
> -    m_viewportController.didScroll(rect.location());
>      didChangeViewport();

This is not correct, it breaks fixed layout scrolling. Also, if we don't call didScroll, we shouldn't need to call didChangeViewport() either.
Comment 15 Carlos Garcia Campos 2016-09-16 04:39:04 PDT
Created attachment 289055 [details]

This should fix the issue
Comment 16 Carlos Garcia Campos 2016-09-17 01:34:40 PDT
Committed r206066: <http://trac.webkit.org/changeset/206066>