Bug 43992 – [chromium] scrolling issues when accelerated compositor is enabled

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alexey Marinichev]

Run chrome with --enable-accelerated-compositing and load http://webkit.org/blog/386/3d-transforms/.  Scroll down couple pages and reload.  Further scrolling results in most of the page not being redrawn.

Also, when running with --enable-accelerated-compositing, transformation matrix for scrolling is calculated incorrectly, which results in incorrect rendering.

[Alexey Marinichev]

Created attachment 64521 [details]
proposed fix

[WebKit Review Bot]

Attachment 64521 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:412:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Marinichev]

Created attachment 64524 [details]
check-webkit-style happy is happy now

[Vangelis Kokkevis]

Comment on attachment 64524 [details]
check-webkit-style happy is happy now


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 163f6c5..ef0d9a6 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,32 @@
> [phone]  Alexey Marinichev  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] scrolling issues when accelerated compositor is enabled
> +        https://bugs.webkit.org/show_bug.cgi?id=43992
> +
> +        No new tests. (OOPS!)
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::drawLayers):
> +
> [phone]  Alexey Marinichev  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] scrolling issues when accelerated compositor is enabled
> +        https://bugs.webkit.org/show_bug.cgi?id=43992
> +
> +        Corrected scroll position not being updated, and an off-by-half error.
> +
> +        To test the former, follow instructions in the bug.  To test the
> +        latter, change GL_NEAREST to GL_LINEAR in LayerRendererChromium.cpp.
> +        Scrolling should work properly for all window sizes, without blurring
> +        images and shifting them to the left.
> +
> +        * platform/graphics/chromium/LayerRendererChromium.cpp:
> +        (WebCore::LayerRendererChromium::drawLayers):
> +
>  2010-08-16  Paul Sawaya  <[e-mail]>
>  
>          Reviewed by Chris Marrin.
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 2f70efa..abaa274 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -390,8 +390,8 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>  #error "Need to implement for your platform."
>  #endif
>  
> -        scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width()) - scrollDelta.x(),
> -            (int)floorf(0.5 * visibleRect.height()) + scaleFactor * scrollDelta.y(), 0);
> +        scrolledLayerMatrix.translate3d((0.5 * visibleRect.width()) - scrollDelta.x(),
> +            (0.5 * visibleRect.height()) + scaleFactor * scrollDelta.y(), 0);
>          scrolledLayerMatrix.scale3d(1, -1, 1);
>  
>          // Switch shaders to avoid RGB swizzling.
> @@ -406,7 +406,10 @@ void LayerRendererChromium::drawLayers(const IntRect& updateRect, const IntRect&
>  
>          checkGLError();
>          m_scrollPosition = scrollPosition;
> -    }
> +    } else if (abs(scrollDelta.y()) > contentRect.height() || abs(scrollDelta.x()) > contentRect.width())
> +        // Scrolling larger than the contentRect size does not preserve any of the pixels, so there is
> +        // no need to copy framebuffer pixels back into the texture.
> +        m_scrollPosition = scrollPosition;
>  
>      // FIXME: The following check should go away when the compositor renders independently from its own thread.
>      // Ignore a 1x1 update rect at (0, 0) as that's used a way to kick off a redraw for the compositor.

WebCore/ChangeLog:11
 +          (WebCore::LayerRendererChromium::drawLayers):
There are two entries in the ChangeLog for this change.

WebCore/platform/graphics/chromium/LayerRendererChromium.cpp: 
 +          scrolledLayerMatrix.translate3d((int)floorf(0.5 * visibleRect.width()) - scrollDelta.x(),
Maybe remove the parenthesis around the 0.5* .. expressions as they're not needed anymore?

[Alexey Marinichev]

Created attachment 64544 [details]
got rid of a redundant entry in the Changelog; removed unnecessary parentheses

[David Levin]

Comment on attachment 64544 [details]
got rid of a redundant entry in the Changelog; removed unnecessary parentheses

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 163f6c5..cbaecba 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,20 @@
> [phone]  Alexey Marinichev  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] scrolling issues when accelerated compositor is enabled
> +        https://bugs.webkit.org/show_bug.cgi?id=43992

It would be nice to have some explanation of why the int cast/floorf call is being removed. (Is that the off-by-half bug?)
> +

Is it possible to create a layout test for this (that verifies the fix when accelerated compositing is on)?


> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +    } else if (abs(scrollDelta.y()) > contentRect.height() || abs(scrollDelta.x()) > contentRect.width())
> +        // Scrolling larger than the contentRect size does not preserve any of the pixels, so there is
> +        // no need to copy framebuffer pixels back into the texture.
> +        m_scrollPosition = scrollPosition;

This should have {} around it since the else clause is more than one logical line.

[Alexey Marinichev]

Created attachment 64613 [details]
added braces

Layout test for the m_scrollPosition update is possible.  I'd rather do it in a separate commit, it will take me some time to figure out how layout tests are written.

Layout test for off by half error is not easy.  It's trivial if GL_NEAREST is changed to GL_LINEAR; otherwise it might require multiple scrolls.  Again, I'd rather do it in a separate commit.

[David Levin]

Comment on attachment 64613 [details]
added braces

I'm sorry, but there really should be a layout test submitted with the fix.

[Vangelis Kokkevis]

(In reply to comment #8)
> (From update of attachment 64613 [details])
> I'm sorry, but there really should be a layout test submitted with the fix.

In all fairness, it's hard to make layout tests for these fixes without resorting to grabbing snapshots from the browser. Unfortunately, we don't yet have the compositor running in chromium's testshell which makes it impossible at the moment to do any automated testing on it.

[Darin Fisher (:fishd, Google)]

Since this is a Chromium only change, and Chromium isn't able to run the compositor layout tests yet, I don't think we should hold up this patch for a layout test.

We should make sure there is a bug filed to add a layout test for this patch, and make it be blocked by the bug to enable the compositor layout tests for Chromium.

[David Levin]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 64613 [details] [details])
> > I'm sorry, but there really should be a layout test submitted with the fix.
> 
> In all fairness, it's hard to make layout tests for these fixes without resorting to grabbing snapshots from the browser. Unfortunately, we don't yet have the compositor running in chromium's testshell which makes it impossible at the moment to do any automated testing on it.

It seems simple. There could be a pixel test that produces the bad result. When the compositor is turned on, this pixel test would catch regressions.

As of now, there is no such pixel test which is really unfortunate.

[WebKit Commit Bot]

Comment on attachment 64613 [details]
added braces

Clearing flags on attachment: 64613

Committed r65651: <http://trac.webkit.org/changeset/65651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.