Bug 101787 – Zoomed-in scrolling is very slow when deviceScaleFactor > 1

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

If you pinch-to-zoom on a retina Mac and try to scroll, it will be very slow. Profiles show that most of the time is spent in CG rendering glyphs.

<rdar://problem/12178726>

[Beth Dakin]

Created attachment 173374 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
WebKitLibraries/WebKitSystemInterface.h:143:  The parameter name "cgContext" adds no information, so it should be removed.  [readability/parameter_name] [5]
WebKitLibraries/WebKitSystemInterface.h:143:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


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

[Simon Fraser (smfr)]

Comment on attachment 173374 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        quantizing, but we'll turn it off if we're scrolling continent that 

continent

What's import is not that we quantize, but that we quantize to something less than a device pixel.

> Source/WebCore/ChangeLog:13
> +        Disable quantization if scrolling is happening on the main thread or 

subpixel quantization

> Source/WebCore/page/FrameView.cpp:3234
> +    bool didQuantizeFonts = true;

This (and all the other) references to quantize should be subpixelQuantize.

> Source/WebCore/page/FrameView.cpp:3247
> +    // FIXME: The !isRootFrame check should not be necessary once sub-frames can scroll on the scrolling thread.
> +    if (scrollingOnMainThread || !isRootFrame) {
> +        didQuantizeFonts = p->shouldQuantizeFonts();
> +        p->setShouldQuantizeFonts(false);
> +    }

Can we do this up in some Mac-specific code? Seems weird to have it here.

Also, I don't think FrameView::paintContents() gets called at all in tiled scrolling mode.

> Source/WebCore/rendering/RenderBlock.cpp:2940
> +    bool didQuantizeFonts = true;
> +    if (hasOverflowClip()) {
> +        didQuantizeFonts = paintInfo.context->shouldQuantizeFonts();
> +        paintInfo.context->setShouldQuantizeFonts(false);
> +    }

There are already platforms that  composite overflow scrolling contents that would not want this. I think we should move this into RenderLayer somewhere, which knows more about this (usesCompositedScrolling()).

> Source/WebCore/rendering/RenderLayerBacking.cpp:1569
> +    bool didQuantizeFonts = true;
> +    bool scrollingOnMainThread = true;
> +#if ENABLE(THREADED_SCROLLING)
> +    if (Page* page = renderer()->frame()->page()) {
> +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +            scrollingOnMainThread = scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread();
> +    }
> +#endif
> +    if (scrollingOnMainThread) {
> +        didQuantizeFonts = context.shouldQuantizeFonts();
> +        context.setShouldQuantizeFonts(false);

This is going to affect every RenderLayerBacking, not just those for the tiles. I think we only want to do this if m_isMainFrameRenderViewLayer.

[Beth Dakin]

(In reply to comment #3)
> > Source/WebCore/page/FrameView.cpp:3247
> > +    // FIXME: The !isRootFrame check should not be necessary once sub-frames can scroll on the scrolling thread.
> > +    if (scrollingOnMainThread || !isRootFrame) {
> > +        didQuantizeFonts = p->shouldQuantizeFonts();
> > +        p->setShouldQuantizeFonts(false);
> > +    }
> 
> Can we do this up in some Mac-specific code? Seems weird to have it here.
> 

I think this is a useful choke-point for painting sub-frames. 

> Also, I don't think FrameView::paintContents() gets called at all in tiled scrolling mode.
> 

It does seem to. 

> > Source/WebCore/rendering/RenderBlock.cpp:2940
> > +    bool didQuantizeFonts = true;
> > +    if (hasOverflowClip()) {
> > +        didQuantizeFonts = paintInfo.context->shouldQuantizeFonts();
> > +        paintInfo.context->setShouldQuantizeFonts(false);
> > +    }
> 
> There are already platforms that  composite overflow scrolling contents that would not want this. I think we should move this into RenderLayer somewhere, which knows more about this (usesCompositedScrolling()).
> 

I can try to find an appropriate spot there.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:1569
> > +    bool didQuantizeFonts = true;
> > +    bool scrollingOnMainThread = true;
> > +#if ENABLE(THREADED_SCROLLING)
> > +    if (Page* page = renderer()->frame()->page()) {
> > +        if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +            scrollingOnMainThread = scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread();
> > +    }
> > +#endif
> > +    if (scrollingOnMainThread) {
> > +        didQuantizeFonts = context.shouldQuantizeFonts();
> > +        context.setShouldQuantizeFonts(false);
> 
> This is going to affect every RenderLayerBacking, not just those for the tiles. I think we only want to do this if m_isMainFrameRenderViewLayer.

I don't follow that. Why wouldn't we want to do this for all backings?

[Beth Dakin]

Created attachment 173437 [details]
Patch

Okay, here's another go.

[Build Bot]

Comment on attachment 173437 [details]
Patch

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

[Simon Fraser (smfr)]

Comment on attachment 173437 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=101787
> +        Zoomed-in scrolling is very slow when deviceScaleFactor > 1

Please put the description first, as prepareChangelog does. That means that browsing short commit messages shows the bug title, rather than just the bug number.

> Source/WebCore/ChangeLog:9
> +        whether or not fonts should be subpixel quantized. We want to default 

subpixel-quantized.

> Source/WebCore/ChangeLog:10
> +        to quantizing, but we'll turn it off if we're scrolling content that 

to subpixel-quantizing.

> Source/WebCore/ChangeLog:14
> +        State has a new bool shouldQuantizeFonts. It defaults to true since 

shouldSubpixelQuantizeFonts

> Source/WebCore/ChangeLog:18
> +        (WebCore):

Nuke it!

> Source/WebCore/ChangeLog:26
> +        whether or not it should try to quantize the fonts.

subpixel-quantize

> Source/WebCore/ChangeLog:32
> +        Disable quantization for overflow areas, subframes, and content that 

etc.

> Source/WebCore/rendering/RenderLayer.cpp:3185
> +    bool didQuantizeFonts = true;

subpixelQuantize

> Source/WebCore/rendering/RenderLayer.cpp:3199
> +    bool needToAdjustSubpixelQuantization = scrollingOnMainThread || (renderer()->hasOverflowClip() && !usesCompositedScrolling()) || (frame && frame->ownerElement());

We might not want the scrollingOnMainThread here. That could cause a page that toggles some style (background-attachment) to have jiggling text, but it would cause slow-scrolling pages to possibly show waviness when scrolling when zoomed, but it this really an issue?

[Beth Dakin]

Committed http://trac.webkit.org/changeset/134348

I just realized though that I forgot to discuss this issue more with Simon:

(In reply to comment #7)
> > Source/WebCore/rendering/RenderLayer.cpp:3199
> > +    bool needToAdjustSubpixelQuantization = scrollingOnMainThread || (renderer()->hasOverflowClip() && !usesCompositedScrolling()) || (frame && frame->ownerElement());
> 
> We might not want the scrollingOnMainThread here. That could cause a page that toggles some style (background-attachment) to have jiggling text, but it would cause slow-scrolling pages to possibly show waviness when scrolling when zoomed, but it this really an issue?

It does help that these are two less-likely scenarios combining. But it would be best to test. I'll try to cook up a test case.

[Beth Dakin]

Build fix: http://trac.webkit.org/changeset/134349

[Tim Horton]

Comment on attachment 173437 [details]
Patch

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

>> Source/WebCore/rendering/RenderLayer.cpp:3199
>> +    bool needToAdjustSubpixelQuantization = scrollingOnMainThread || (renderer()->hasOverflowClip() && !usesCompositedScrolling()) || (frame && frame->ownerElement());
> 
> We might not want the scrollingOnMainThread here. That could cause a page that toggles some style (background-attachment) to have jiggling text, but it would cause slow-scrolling pages to possibly show waviness when scrolling when zoomed, but it this really an issue?

Hehe, two years later I just noticed exactly this. Going into find-in-page overlay mode (with the holes) throws us into main thread scrolling (for now), and any tiles that repaint after that have subpixel quantization disabled, leading to neat cracks and slight text jumpyness (especially when leaving this mode). Also makes it hard for the find highlight to precisely match the text underneath :)