Summary: | [chromium] Viewport is not filled when out of texture memory on mac | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dana Jansens <danakj> | ||||||||||||||
Component: | New Bugs | Assignee: | 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
Dana Jansens
2012-04-05 22:38:10 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.
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. 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 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. (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. 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 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 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? (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. Created attachment 136097 [details]
Patch
(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 on attachment 136097 [details]
Patch
R=me. Thanks! I'm super excited to get rid of backgroundCovers"Viewport".
Created attachment 136261 [details]
Patch
rebased
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 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?
I'm going to s/WebColor&/WebColor/ Created attachment 136263 [details]
Patch
no reference for WebColor
Comment on attachment 136263 [details]
Patch
Looks fine
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 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 Created attachment 136375 [details]
Patch for landing
Comment on attachment 136375 [details] Patch for landing Clearing flags on attachment: 136375 Committed r113677: <http://trac.webkit.org/changeset/113677> All reviewed patches have been landed. Closing bug. |