Bug 83351 – [chromium] Viewport is not filled when out of texture memory on mac

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Viewport is not filled when out of texture memory on mac

[Dana Jansens]

Created attachment 135981 [details]
Patch

Here's a CL to make sure we draw something opaque in every pixel in the viewport without special-casing drawing of the NCCH.

I renamed "backgroundCoversViewport" since that is not what it actually means. I propose the name "rootContentLayer" instead,
to say that the layer is the parent of all content in the tree, and that its background color should be used to fill the
background of the viewport.

We use the occlusion tracker to figure out what pixels we are seeing through the viewport with, and we add "gutter quads" to
cover those pixels up with the NCCH background color. I tested this with acko.net, by setting the memory limits very low, and
it seems well. Am running a mac try job to try and verify that the rubberbanding works.

[Dana Jansens]

The transform-overhang tests are currently marked as IMAGE PASS so it's hard to tell from EWS if this is good. Still attempting to get a good mac try job without them blacklisted in expectations.

[Dana Jansens]

Looks good on mac.

http://build.chromium.org/p/tryserver.chromium/builders/mac_layout_rel/builds/194/steps/webkit_tests/logs/stdio

08:52:17.984 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-e.html passed
08:52:18.232 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-n.html passed
08:52:18.530 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-ne.html passed
08:52:18.799 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-nw.html passed
08:52:19.036 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-s.html passed
08:52:19.296 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-se.html passed
08:52:19.803 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-size-change.html passed
08:52:20.099 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-sw.html passed
08:52:20.352 20323 worker/1 platform/chromium/compositing/rubberbanding/transform-overhang-w.html passed

[Adrienne Walker]

Another use case for consideration is that sometimes we don't have a NCCH.  See bug 82860, for what I'm talking about here.  I totally dig the idea that this should be based on the occlusion tracker and not just on the size of one layer against its clip rect.

However, I'd like to see us not special case any layer and instead set a background color on the layer tree host itself for what color the viewport should be filled with if not covered by opaque content.

[Dana Jansens]

(In reply to comment #4)
> Another use case for consideration is that sometimes we don't have a NCCH.  See bug 82860, for what I'm talking about here.  I totally dig the idea that this should be based on the occlusion tracker and not just on the size of one layer against its clip rect.
> 
> However, I'd like to see us not special case any layer and instead set a background color on the layer tree host itself for what color the viewport should be filled with if not covered by opaque content.

Oh, okay cool. I was not sure how to handle this well.. that sounds good to me.

[Dana Jansens]

Created attachment 136050 [details]
Patch

- Remove backgroundCoversViewport() entirely.
- Set the background color on the LayerTreeHost when painting the NCCH, from WebViewImpl.
- Default the background color on the LTH to white in case we never had a NCCH yet.

[Adrienne Walker]

Comment on attachment 136050 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        In order to do this, we fix a bug in the occlusion tracker, that
> +        allowed opaque() layers with skipsDraw to occlude, by sticking a
> +        virtual skipsDraw() on the layer classes that the tiled layers
> +        can override and return something useful with.

Forgive my change aversion, but I liked when skipsDraw was a property that was localized internal to tiled layers and not public in a way that other parts of the compositor needed to worry about.

Can visibleContentOpaqueRegion() just return the right thing for opaque() layers, handling skipsDraw internally?

> Source/Platform/chromium/public/WebLayerTreeView.h:114
> +    // Sets the background color for the viewport.
> +    WEBKIT_EXPORT void setBackgroundColor(const WebCore::Color&);
> +

Since this is the platform interface, I think this needs to be WebColor.

[Dana Jansens]

Comment on attachment 136050 [details]
Patch

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

>> Source/WebCore/ChangeLog:22
>> +        can override and return something useful with.
> 
> Forgive my change aversion, but I liked when skipsDraw was a property that was localized internal to tiled layers and not public in a way that other parts of the compositor needed to worry about.
> 
> Can visibleContentOpaqueRegion() just return the right thing for opaque() layers, handling skipsDraw internally?

We can move skipsDraw back. But it means making the LayerChromium/CCLayerImpl hold logic about returning their opaque area if opaque() is true. I liked what we have now because that logic is isolated in CCOcclusionTracker.cpp and not duplicated across the two layer types. Is there some compromise?

[Adrienne Walker]

(In reply to comment #8)
> (From update of attachment 136050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136050&action=review
> 
> >> Source/WebCore/ChangeLog:22
> >> +        can override and return something useful with.
> > 
> > Forgive my change aversion, but I liked when skipsDraw was a property that was localized internal to tiled layers and not public in a way that other parts of the compositor needed to worry about.
> > 
> > Can visibleContentOpaqueRegion() just return the right thing for opaque() layers, handling skipsDraw internally?
> 
> We can move skipsDraw back. But it means making the LayerChromium/CCLayerImpl hold logic about returning their opaque area if opaque() is true. I liked what we have now because that logic is isolated in CCOcclusionTracker.cpp and not duplicated across the two layer types. Is there some compromise?

It's true that this results in duplicating that conditional in two functions, but I think that's the right abstraction?  Maybe it just seems to me that CCOcclusionTracker shouldn't know better than the layer itself about what that layer's opaque region is and visibleContentOpaqueRegion() should return what it says on the tin.

[Dana Jansens]

Created attachment 136097 [details]
Patch

[Dana Jansens]

(In reply to comment #7)
> Since this is the platform interface, I think this needs to be WebColor.

done.

(In reply to comment #9)
> It's true that this results in duplicating that conditional in two functions, but I think that's the right abstraction?  Maybe it just seems to me that CCOcclusionTracker shouldn't know better than the layer itself about what that layer's opaque region is and visibleContentOpaqueRegion() should return what it says on the tin.

done. this wasn't as bad as I was thinking it would be for whatever reason.


this is depping on bug #83217 cuz they both change the same code (markOccludedBehindLayer) and that one is for M19 so i'd rather it go in first.

[Adrienne Walker]

Comment on attachment 136097 [details]
Patch

R=me.  Thanks! I'm super excited to get rid of backgroundCovers"Viewport".

[Dana Jansens]

Created attachment 136261 [details]
Patch

rebased

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Adrienne Walker]

Comment on attachment 136261 [details]
Patch

Oops, this never got an API approval.

jamesr, can you ok the API changes and CQ+ this if you're good with it?

[Dana Jansens]

I'm going to s/WebColor&/WebColor/

[Dana Jansens]

Created attachment 136263 [details]
Patch

no reference for WebColor

[James Robinson]

Comment on attachment 136263 [details]
Patch

Looks fine

[Dana Jansens]

Comment on attachment 136263 [details]
Patch

This depends on the culling clipped rects bug which was reverted. Will request CQ again when it lands again.

[WebKit Review Bot]

Comment on attachment 136263 [details]
Patch

Rejecting attachment 136263 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
sts/CCLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
patching file Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp
patching file Source/WebKit/chromium/tests/CCTiledLayerImplTest.cpp
patching file Source/WebKit/chromium/tests/LayerChromiumTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12371534

[Dana Jansens]

Created attachment 136375 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 136375 [details]
Patch for landing

Clearing flags on attachment: 136375

Committed r113677: <http://trac.webkit.org/changeset/113677>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.