Bug 83351

Summary: [chromium] Viewport is not filled when out of texture memory on mac
Product: WebKit Reporter: Dana Jansens <danakj>
Component: New BugsAssignee: Dana Jansens <danakj>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, asvitkine, backer, cc-bugs, dglazkov, enne, fishd, jamesr, piman, reveman, sievers, tkent, vangelis, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 83217    
Bug Blocks: 83386    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Dana Jansens 2012-04-05 22:38:10 PDT
[chromium] Viewport is not filled when out of texture memory on mac
Comment 1 Dana Jansens 2012-04-05 22:43:52 PDT
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.
Comment 2 Dana Jansens 2012-04-06 07:26:34 PDT
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.
Comment 3 Dana Jansens 2012-04-06 09:15:36 PDT
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
Comment 4 Adrienne Walker 2012-04-06 10:22:22 PDT
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.
Comment 5 Dana Jansens 2012-04-06 10:37:43 PDT
(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.
Comment 6 Dana Jansens 2012-04-06 12:17:36 PDT
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.
Comment 7 Adrienne Walker 2012-04-06 13:12:16 PDT
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.
Comment 8 Dana Jansens 2012-04-06 13:18:39 PDT
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?
Comment 9 Adrienne Walker 2012-04-06 13:27:23 PDT
(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.
Comment 10 Dana Jansens 2012-04-06 16:48:29 PDT
Created attachment 136097 [details]
Patch
Comment 11 Dana Jansens 2012-04-06 16:50:16 PDT
(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.
Comment 12 Adrienne Walker 2012-04-06 17:50:44 PDT
Comment on attachment 136097 [details]
Patch

R=me.  Thanks! I'm super excited to get rid of backgroundCovers"Viewport".
Comment 13 Dana Jansens 2012-04-09 10:29:13 PDT
Created attachment 136261 [details]
Patch

rebased
Comment 14 WebKit Review Bot 2012-04-09 10:31:39 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 15 Adrienne Walker 2012-04-09 10:38:15 PDT
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?
Comment 16 Dana Jansens 2012-04-09 10:39:26 PDT
I'm going to s/WebColor&/WebColor/
Comment 17 Dana Jansens 2012-04-09 10:44:36 PDT
Created attachment 136263 [details]
Patch

no reference for WebColor
Comment 18 James Robinson 2012-04-09 10:45:31 PDT
Comment on attachment 136263 [details]
Patch

Looks fine
Comment 19 Dana Jansens 2012-04-09 11:46:36 PDT
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.
Comment 20 WebKit Review Bot 2012-04-09 18:25:20 PDT
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
Comment 21 Dana Jansens 2012-04-09 19:16:14 PDT
Created attachment 136375 [details]
Patch for landing
Comment 22 WebKit Review Bot 2012-04-09 21:55:11 PDT
Comment on attachment 136375 [details]
Patch for landing

Clearing flags on attachment: 136375

Committed r113677: <http://trac.webkit.org/changeset/113677>
Comment 23 WebKit Review Bot 2012-04-09 21:55:18 PDT
All reviewed patches have been landed.  Closing bug.