Bug 66969 – Chromium: Add a layer for rubber-band overhang painting to the hardware path.

• Component: New Bugs
• Status: RESOLVED FIXED

Chromium: Add a layer for rubber-band overhang painting to the hardware path.

Created attachment 105229 [details]
Patch

[Adrienne Walker]

Comment on attachment 105229 [details]
Patch

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

Despite being titled as such, this doesn't look like a Chromium-specific patch.  I think you are going to either need to add some sort of setting for this or otherwise make it conditional.

Does rubber-banding work out of the box on other hardware accelerated ports?

> Source/WebCore/platform/ScrollView.cpp:1037
> +#if USE(ACCELERATED_COMPOSITING)
> +    if (!layerForScrollCorner())
> +        calculateAndPaintOverhangAreas(context, rect);

Why do you need the scroll corner layer check here?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:960
> +        m_layerForOverhangAreas->setSize(frameView->frameRect().size());

Maybe I misunderstand how these layers are supposed to work, but I would expect overhang layers to be related to the size and position of horizontalOverhangRect and verticalOverhangRect.

(In reply to comment #2)
> (From update of attachment 105229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=105229&action=review
> 
> Despite being titled as such, this doesn't look like a Chromium-specific patch.  I think you are going to either need to add some sort of setting for this or otherwise make it conditional.
> 
> Does rubber-banding work out of the box on other hardware accelerated ports?
> 
> > Source/WebCore/platform/ScrollView.cpp:1037
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    if (!layerForScrollCorner())
> > +        calculateAndPaintOverhangAreas(context, rect);
> 
> Why do you need the scroll corner layer check here?

That was meant to be:

> > +    if (!layerForOverhangAreas())
> > +        calculateAndPaintOverhangAreas(context, rect);

So that it's only called in the software path.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:960
> > +        m_layerForOverhangAreas->setSize(frameView->frameRect().size());
> 
> Maybe I misunderstand how these layers are supposed to work, but I would expect overhang layers to be related to the size and position of horizontalOverhangRect and verticalOverhangRect.

If I did it that way, then two layers would be required, which would be more boilerplate. Instead, I decided to go with one layer that covers the size of web content area.

The other advantage of having a single layer is that we can do it with a single paintOverhangAreas() like the existing software path, rather than calling it twice (clipped to each rect) or refactoring the code to have paintHorizontalOverhang(), paintVerticalOverhang() and paintOverhangCorner().

[Adrienne Walker]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 105229 [details] [details])
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:960
> > > +        m_layerForOverhangAreas->setSize(frameView->frameRect().size());
> > 
> > Maybe I misunderstand how these layers are supposed to work, but I would expect overhang layers to be related to the size and position of horizontalOverhangRect and verticalOverhangRect.
> 
> If I did it that way, then two layers would be required, which would be more boilerplate. Instead, I decided to go with one layer that covers the size of web content area.

So, that means that you have to invalidate and repaint the entire contents of the overhang area on every change in scroll position?

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 105229 [details] [details] [details])
> > > 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:960
> > > > +        m_layerForOverhangAreas->setSize(frameView->frameRect().size());
> > > 
> > > Maybe I misunderstand how these layers are supposed to work, but I would expect overhang layers to be related to the size and position of horizontalOverhangRect and verticalOverhangRect.
> > 
> > If I did it that way, then two layers would be required, which would be more boilerplate. Instead, I decided to go with one layer that covers the size of web content area.
> 
> So, that means that you have to invalidate and repaint the entire contents of the overhang area on every change in scroll position?

Yes, the patch currently does:

overhangLayer->setNeedsDisplay();

in ScrollView::scrollContents().

This shouldn't invalidate other layers below the overhang layer, right?

> > So, that means that you have to invalidate and repaint the entire contents of the overhang area on every change in scroll position?

Sorry, just to clarify:

It won't invalidate the overhang layer unless the ScrollView has overhangs. If there is no overhang, the code calls overhangLayer->setDrawsContent(false). Again, see ScrollView::scrollContents().

Created attachment 105628 [details]
Patch

> > Source/WebCore/platform/ScrollView.cpp:1037
> > +#if USE(ACCELERATED_COMPOSITING)
> > +    if (!layerForScrollCorner())
> > +        calculateAndPaintOverhangAreas(context, rect);

I've corrected the check to use layerForOverhangAreas() as was intended in the first place.

[James Robinson]

Comment on attachment 105628 [details]
Patch

We talked about how to add tests for this, which it definitely needs.  I actually don't really know what 'overhang' is, could you explain or provide some reference?

(In reply to comment #9)
> (From update of attachment 105628 [details])
> I actually don't really know what 'overhang' is, could you explain or provide some reference?

See: http://code.google.com/p/chromium/issues/detail?id=88353

[Nico Weber]

(In reply to comment #9)
> (From update of attachment 105628 [details])
> We talked about how to add tests for this, which it definitely needs.

dethbakin tells me there's currently no infrastructure to test rubberband scrolling ("it would be great to add some, but i can't think of how it would work")

[James Robinson]

Comment on attachment 105628 [details]
Patch

Given that Safari has this behavior without any WebCore changes, I'm beginning to suspect this is the wrong approach.  Another issue is that this seems to create layers for scrollable subframes, although that doesn't seem at all desirable.

Is the idea here that you just want to see a textured background "behind" the root layer when scrolling it past the viewport bounds? If so I think that should be handled at the compositor level, not in ScrollView.

(In reply to comment #12)
> (From update of attachment 105628 [details])
> Given that Safari has this behavior without any WebCore changes, I'm beginning to suspect this is the wrong approach. 

Adrienne Walker suggested to use this approach.

> Another issue is that this seems to create layers for scrollable subframes, although that doesn't seem at all desirable.

The new layer lives in RenderLayerCompositor, not in ScrollView. ScrollView just uses it if there is one.

> Is the idea here that you just want to see a textured background "behind" the root layer when scrolling it past the viewport bounds?

Yes, with some custom shadows over the texture.

Created attachment 106031 [details]
Patch

(In reply to comment #14)
> Created an attachment (id=106031) [details]
> Patch

The updated patch is a work in progress that adds some layout tests.

[James Robinson]

(In reply to comment #13)
> > Is the idea here that you just want to see a textured background "behind" the root layer when scrolling it past the viewport bounds?
> 
> Yes, with some custom shadows over the texture.

Where are the shadows supposed to be rendered?  Can you show some screenshots of what the expected behavior is for a rubber-banded main frame and a rubber-banded iframe?

Created attachment 106124 [details]
Example screenshot

> Where are the shadows supposed to be rendered?  Can you show some screenshots of what the expected behavior is for a rubber-banded main frame and a rubber-banded iframe?

Here's an example screenshot for a rubber-banded main frame (iframes do not get rubber banding behaviour). The overhang is at the top left in this example.

Created attachment 106132 [details]
Patch

[WebKit Review Bot]

Comment on attachment 106132 [details]
Patch

Attachment 106132 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9579803

New failing tests:
platform/chromium-mac/rubberbanding/transform-overhang-n.html
platform/chromium-mac/rubberbanding/overhang-e.html
platform/chromium-mac/rubberbanding/transform-overhang-e.html
media/media-blocked-by-willsendrequest.html
platform/chromium-mac/rubberbanding/overhang-se.html
platform/chromium-mac/rubberbanding/overhang-sw.html
platform/chromium-mac/rubberbanding/overhang-ne.html
platform/chromium-mac/rubberbanding/overhang-s.html
platform/chromium-mac/rubberbanding/overhang-nw.html
platform/chromium-mac/rubberbanding/overhang-w.html
platform/chromium-mac/rubberbanding/overhang-n.html
fast/forms/implicit-submission.html

[Nico Weber]

+bdakin since this adds infrastructure for overhang scrolling tests

[James Robinson]

May I suggest moving the testing stuff to a different patch, independent from the chromium compositor changes?  I think that the testing logic is of value by itself and it's better not to mix cross-platform changes with platform-specific changes whenever possible.

(In reply to comment #21)
> May I suggest moving the testing stuff to a different patch, independent from the chromium compositor changes?  I think that the testing logic is of value by itself and it's better not to mix cross-platform changes with platform-specific changes whenever possible.

Sounds good to me, I'll do that.

Created https://bugs.webkit.org/show_bug.cgi?id=67511 which adds the test infrastructure with tests that already pass (the non-gpu case).

Created attachment 106417 [details]
Patch

Created attachment 106638 [details]
Patch

[Adrienne Walker]

(In reply to comment #25)
> Created an attachment (id=106638) [details]
> Patch

I talked with James a little bit offline (who is pretty busy).  I think part of the problem with this patch is what I mentioned in comment 2.  This continues to not really look like a Chromium-specific patch, even though this overhang logic is only needed for the Chromium port.  So, I think you're going to need some #ifdefs or something.  Also, your patch as written applies to iframes as well as the main frame, so you're going to need some additional logic there.

I worry a little bit about having a full viewport graphics layer.  I still don't like that you have to paint the overhang layers every frame because paintOverhangLayers intermingles paints of both a drop shadow that moves with the layer and a texture background that's pinned to the viewport.  And, even if you only paint part of this layer, you still potentially have to upload the entire viewport's worth of that buffer due to the way invalidation rect unioning happens in TiledLayerChromium.

Can you create a GPU trace of how this overhang painting performs? In particular, on a large monitor, full screen Chrome, open up a page that you can scroll on, open chrome://tracing in another tab, hit record, then go back to your original page.  Do some normal scrolling for a few seconds, then do some scrolling where you hit both the vertical and the horizontal overhang.  Hit the stop tracing button on the tracing tab.  You can then save that trace and send it to me or just attach it here.

Created attachment 106736 [details]
Trace requested in comment 26.

Trace of scrolling normally to the bottom of http://www.webkit.org/blog/386/3d-transforms/, then doing rubber-band scrolling a few times to trigger the overhang drawing.

Note: That page uses 3D CSS (necessary to trigger this path).

(In reply to comment #26)
> (In reply to comment #25)
> > Created an attachment (id=106638) [details] [details]
> > Patch
> 
> Also, your patch as written applies to iframes as well as the main frame, so you're going to need some additional logic there.

How can I check if this is an iframe from RenderLayerCompositor?

[Adrienne Walker]

(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > Created an attachment (id=106638) [details] [details] [details]
> > > Patch
> > 
> > Also, your patch as written applies to iframes as well as the main frame, so you're going to need some additional logic there.
> 
> How can I check if this is an iframe from RenderLayerCompositor?

m_renderView->document()->ownerElement() is true if it's an iframe.

Created attachment 106764 [details]
Patch

(In reply to comment #30)
> Created an attachment (id=106764) [details]
> Patch

Updated patch doesn't create the layer for iframes and has the layer creation is ifdef'd only for Chromium + Mac.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium-gpu/composit..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:3736 Path does not exist. fast/block/positioning/positioned-float-layout-after-image-load.html
LayoutTests/platform/chromium/test_expectations.txt:3736:  Path does not exist. fast/block/positioning/positioned-float-layout-after-image-load.html  [test/expectations] [2]
Total errors found: 1 in 25 files


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

(In reply to comment #27)
> Created an attachment (id=106736) [details]
> Trace requested in comment 26.
> 
> Trace of scrolling normally to the bottom of http://www.webkit.org/blog/386/3d-transforms/, then doing rubber-band scrolling a few times to trigger the overhang drawing.
> 
> Note: That page uses 3D CSS (necessary to trigger this path).

Adrienne, any insight from this trace?

[Adrienne Walker]

I haven't forgotten you.  Just trying to figure out what to suggest.

(In reply to comment #27)
> Created an attachment (id=106736) [details]
> Trace requested in comment 26.
> 
> Trace of scrolling normally to the bottom of http://www.webkit.org/blog/386/3d-transforms/, then doing rubber-band scrolling a few times to trigger the overhang drawing.
> 
> Note: That page uses 3D CSS (necessary to trigger this path).

Thanks! From that trace, it looks like at a resolution around 1900x1200, paint and upload combined take a whopping 5ms per 16ms frame when drawing the overhang.  As I feared, it's doing an entire viewport's worth of painting and texture uploads when you have both overhangs.  I'm not comfortable with that.

Part of the problem is the naive way that invalidations are being unioned together in TiledLayerChromium (so L-shapes become squares), and this should be fixed in a separate patch.  It's on my radar as bug 67854, but I've been busy.

I'll mention that there's some design discomfort here with how this is being done, that's likely causing some reluctance to review this patch.  For some background, we're working towards having Chromium's compositor be able to scroll in a separate thread without needing WebKit paints, so having to repaint every scroll defeats that purpose, even if it's just in this edge case.  So, although I agree with you that it's nice to be correct first and fast second, it's hard not to look at this patch as introducing technical debt that somebody from the GPU team will have to pay in the future.

In an ideal world, I'd want these gradient borders and repeating texture backgrounds as a feature on LayerRendererChromium, which could draw and upload both more efficiently and more generally.  That seems like the right place to put this logic. Unfortunately, that approach is a good bit more complex than this patch.

I think what you have is probably the simplest correct solution, even if I'm not that happy with the #ifdef warts.  However, the performance makes me really uncomfortable.  I'm also not a reviewer, so you may want to work on convincing somebody else who is.

Can a reviewer chime in, here?

What would it take to get this landed?

I can change the code to have two overhang layers - one for the vertical overhang and another one for the horizontal overhang - which would avoid the need for a full-window layer in the overhang case - just the size of the overhangs. This would add slightly more boilerplate to the patch but would likely improve performance.

But would this be enough for a reviewer to accept this? Or is the only way to get this landed is to rewrite the whole thing in another way?

Someone who is willing to review this, please advise. Thanks.

[James Robinson]

What sort of hardware was that trace recorded on?  The performance is really bad.

This was on Mac Pro running Lion:

Processor  2 x 2.66 GHz 6-Core Intel Xeon
Graphics  ATI Radeon HD 5770 1024 MB

Note: It probably wasn't a very good performance test - I was running other stuff in the background. (Though I don't believe I was doing a build at the time.)

I can re-run in more controlled circumstances if you want.

[James Robinson]

I have two main concerns at this point with this patch:

1.) The performance is bad.  Based on that trace, it seems this costs 6ms/frame on a fairly top end Mac Pro.  I imagine on an air or similar it'll be even worse.
2.) Related to (1), the design isn't ideal here - it touches RenderLayerCompositor when it really shouldn't, it puts the gradient and linen texture on the same layer, and it adds extra layers even when there is no overhang.

Unfortunately for you fixing (2) is a little tricky now since our compositor infrastructure is very much a work in progress and in flux.

My feeling is that it's necessary to fix at least separate the gradient from the linen layers in order to get reasonable performance, so maybe you could do that and then we can look at how much work the remaining code will take to fix?

> My feeling is that it's necessary to fix at least separate the gradient from the linen layers in order to get reasonable performance

Wouldn't the shadow layer invalidation alone have the same performance impact as the current patch (where the layer would need to be uploaded at every scroll offset change)? At least, assuming the shadow isn't dissected into many separate layers.

For example, consider both overhangs being present - one at the top and one on the left. Then there's an L shaped shadow around the corner of the web content area and another drop-shadow from the top. Either this layer would need a full content size layer or very complex logic to manipulate a lot of little layers.

Or am I missing something that would make this more efficient?

[Adrienne Walker]

(In reply to comment #38)

> My feeling is that it's necessary to fix at least separate the gradient from the linen layers in order to get reasonable performance, so maybe you could do that and then we can look at how much work the remaining code will take to fix?

If it's in a separate layer, there will still be a full viewport paint and upload of the whole linen texture, at least for the first frame.  At least it wouldn't be a per-frame cost, at that point.

[James Robinson]

Right, there would be one paint+upload for the linen texture and one for the shadow, but in the common case of rubber banding into only the X or Y direction (and not both) you wouldn't need to repaint at all after the initial upload.

(In reply to comment #41)
> Right, there would be one paint+upload for the linen texture and one for the shadow, but in the common case of rubber banding into only the X or Y direction (and not both) you wouldn't need to repaint at all after the initial upload.

So the common case is a single overhang in the Y direction (up/down).

When the overhang area is above, there's two shadows - one from the toolbar and one from the web content. This would still need to be redrawn on each resize unless we further split it into two.

Created attachment 107172 [details]
Trace of regular scrolling

Created attachment 107173 [details]
Trace of rubber-band scrolling.

I was concerned about making conclusions from my previous trace, which was not in controlled circumstances. In particular, I don't remember whether that was on a Debug or Release build (oops).

I took two more traces with the existing patch on the same page. One with regular scrolling and the other one triggering rubber-banding.

> I took two more traces with the existing patch on the same page. One with regular scrolling and the other one triggering rubber-banding.

(The new traces are from a Release build.)

Created attachment 107182 [details]
Trace of rubber-banding with split horizontal / vertical overhang layers.

Here's a trace of rubber-banding on the same page in a Release build with a patch that has separate layers for the vertical and horizontal overhang layers.

James or Adrienne, could you take a glance at the trace and see what the performance of that is like?

I understand the desire to avoid any redrawing while scrolling, but I fear to do that for the overhang stuff would result in a lot of code that will be different between the compositor / non-compositor path that would be a maintenance burden if we ever decide to change how the overhang looks. (For instance, the fact that we currently draw a drop-shadow from the top of the window would affect the design of the layers used under the compositor if we wish to avoid re-drawing things.)

So I'm hoping just splitting horizontal / vertical overhang layers would provide good enough performance so that we wouldn't need to write/maintain a lot of compositor specific code to draw the overhang.

[James Robinson]

(In reply to comment #47)
> I understand the desire to avoid any redrawing while scrolling, but I fear to do that for the overhang stuff would result in a lot of code that will be different between the compositor / non-compositor path that would be a maintenance burden if we ever decide to change how the overhang looks. (For instance, the fact that we currently draw a drop-shadow from the top of the window would affect the design of the layers used under the compositor if we wish to avoid re-drawing things.)
> 
> So I'm hoping just splitting horizontal / vertical overhang layers would provide good enough performance so that we wouldn't need to write/maintain a lot of compositor specific code to draw the overhang.

That's the thing, the two codepaths *are* very different and so the compositor path by definition has to have a lot of compositor specific code.  We'll have to rewrite this patch for the compositor path within a few weeks, if it lands as-is.  I'd really like to not be blocking your progress as much as possible, but this is a lot of complexity.

Anyway, the performance looks OK in release, and I suppose we (the compositor team) can pick up the rewrite since there's some blocking work that we need to take care of.

[James Robinson]

Comment on attachment 106764 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:960
> +        // Keep the overhang layer's size synchronized with the frame size.

This comment is useless, remove it

> Source/WebCore/rendering/RenderLayerCompositor.h:316
> +    OwnPtr<GraphicsLayer> m_layerForOverhangAreas;

shouldn't this be guarded by #if PLATFORM(CHROMIUM) along with the other logic? There's no need to add overhead for other ports that will never use this.

> LayoutTests/platform/chromium-gpu/compositing/rubberbanding/transform-overhang-e-expected.txt:1
> +layer at (0,0) size 800x600

you shouldn't need any of these since the render tree is not useful.  Please add the line "layoutTestController.dumpAsText(true)" to your tests

> Anyway, the performance looks OK in release, and I suppose we (the compositor team) can pick up the rewrite since there's some blocking work that we need to take care of.

That's great to hear!

Is there any significant improvement with the trace in comment 47 vs. the one in comment 44?

Created attachment 107208 [details]
Patch

Updated patch addresses issues in comment 49.

However, it seems some recent change has made this no longer pass its layout tests. I'm investigating.

Created attachment 107344 [details]
Patch

(In reply to comment #53)
> Created an attachment (id=107344) [details]
> Patch

The updated patch passes now passes its own layout tests again.

James, can you take another look?

[James Robinson]

Comment on attachment 107344 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 107344 [details]
Patch

Clearing flags on attachment: 107344

Committed r95158: <http://trac.webkit.org/changeset/95158>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.