Bug 83241 - [chromium] Draw missing layer tile checkerboards as checkerboards
Summary: [chromium] Draw missing layer tile checkerboards as checkerboards
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrienne Walker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-04 18:27 PDT by Adrienne Walker
Modified: 2012-04-11 13:11 PDT (History)
8 users (show)

See Also:


Attachments
Patch (19.03 KB, patch)
2012-04-04 18:32 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Remove unneeded tile size (18.79 KB, patch)
2012-04-04 18:37 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Use CCSettings, use lighter colors (21.63 KB, patch)
2012-04-05 14:36 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Only checkerboard root, rename usedCheckerboard (44.14 KB, patch)
2012-04-05 16:26 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch for landing (44.48 KB, patch)
2012-04-09 15:19 PDT, Adrienne Walker
no flags Details | Formatted Diff | Diff
Patch (12.64 KB, patch)
2012-04-11 12:55 PDT, John Bauman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrienne Walker 2012-04-04 18:27:59 PDT
[chromium] Draw missing layer tile checkerboards as checkerboards
Comment 1 Adrienne Walker 2012-04-04 18:32:07 PDT
Created attachment 135739 [details]
Patch
Comment 2 Adrienne Walker 2012-04-04 18:37:04 PDT
Created attachment 135740 [details]
Remove unneeded tile size
Comment 3 Adrienne Walker 2012-04-04 18:40:35 PDT
Comment on attachment 135740 [details]
Remove unneeded tile size

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

This code was ported from Vangelis's patch here: https://bug-69585-attachments.webkit.org/attachment.cgi?id=110070.

The biggest difference is that I just specified the checkerboard width explicitly and used the tile rect position as an offset to make it line up with other tiles in a global sense.  The checkerboards won't perfectly line up with tile edges within a tile, but they're perfectly square, and I think it looks visually better.

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
> +#if OS(ANDROID)
>                  usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));
> +#else
> +                usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> +#endif

I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.
Comment 4 James Robinson 2012-04-04 19:31:13 PDT
Can we get some UX guidance about if this is the checkerboard we want?  I don't want to have to do a bunch of round-trips if it ends up someone dislikes it.
Comment 5 Dana Jansens 2012-04-04 20:33:15 PDT
Comment on attachment 135740 [details]
Remove unneeded tile size

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

>> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
>> +#endif
> 
> I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.

I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.

So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.
Comment 6 Adrienne Walker 2012-04-05 09:22:24 PDT
(In reply to comment #5)
> (From update of attachment 135740 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135740&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
> >> +#endif
> > 
> > I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.
> 
> I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.

I'm not sure I'm sold on having two quad types that draw solid colors.  I guess I think of quad material as "this is how this should be drawn", so solid colors should be drawn with solid color quads.

I could also just make all platforms use checkerboards for missing tiles and let the Android port sort out what they really want when they upstream.

> So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.

Transparency is still applied, so you get a semi-transparent checkerboard if the drawOpacity() of the layer was not 1.  If the drawOpacity() was 0, then the layer wouldn't draw at all.
Comment 7 Vangelis Kokkevis 2012-04-05 10:18:01 PDT
Comment on attachment 135740 [details]
Remove unneeded tile size

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

>>>> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:186
>>>> +#endif
>>> 
>>> I'm not super happy with this.  I'll take suggestions if somebody sees a more elegant way to push this as a setting down to this function.
>> 
>> I guess you could make a "NotATextureDrawQuad" (insert better name) and make it support modes such as checkerboard and solid color. Not sure that's more elegant or anything.
>> 
>> So this will also do the same for non-NCCH layers? What if the layer was originally transparent? I guess we really need to detect solid color tiles and not use textures for them.
> 
> I'm not sure I'm sold on having two quad types that draw solid colors.  I guess I think of quad material as "this is how this should be drawn", so solid colors should be drawn with solid color quads.
> 
> I could also just make all platforms use checkerboards for missing tiles and let the Android port sort out what they really want when they upstream.

Could we make it a setting in CCSettings and have the CCTiledLayerImpl get the setting value via its layerRenderer?
Comment 8 Adrienne Walker 2012-04-05 14:36:10 PDT
Created attachment 135907 [details]
Use CCSettings, use lighter colors
Comment 9 Dana Jansens 2012-04-05 14:38:09 PDT
Comment on attachment 135907 [details]
Use CCSettings, use lighter colors

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

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:192
> +                if (m_checkerboardMissingTiles)
> +                    usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> +                else
> +                    usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));

suddenly checkerboard is an overloaded term :(
Comment 10 Adrienne Walker 2012-04-05 14:39:56 PDT
(In reply to comment #9)
> (From update of attachment 135907 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135907&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:192
> > +                if (m_checkerboardMissingTiles)
> > +                    usedCheckerboard |= quadList.append(CCCheckerboardDrawQuad::create(sharedQuadState, tileRect));
> > +                else
> > +                    usedCheckerboard |= quadList.append(CCSolidColorDrawQuad::create(sharedQuadState, tileRect, backgroundColor()));
> 
> suddenly checkerboard is an overloaded term :(

I was going to do that in a follow-up patch, but I might as well just shave that yak now.

(In reply to comment #4)
> Can we get some UX guidance about if this is the checkerboard we want?  I don't want to have to do a bunch of round-trips if it ends up someone dislikes it.

alcor and glen argued about this and decided they liked 16 pixel checkerboards and white / 0xf1f1f1 colors.  It's pretty trivial to change both of these things in code.
Comment 11 James Robinson 2012-04-05 14:44:00 PDT
Comment on attachment 135907 [details]
Use CCSettings, use lighter colors

I can't offhand remember how this relates to backgroundCoversViewport.  We're only generating new-style checkerboards on tiles on the root (backgroundCoversViewport) layer, right?  Is that because that's the only case where we have tiles with non-empty tileRects and no texture IDs?
Comment 12 Adrienne Walker 2012-04-05 15:00:23 PDT
(In reply to comment #11)
> (From update of attachment 135907 [details])
> I can't offhand remember how this relates to backgroundCoversViewport.  We're only generating new-style checkerboards on tiles on the root (backgroundCoversViewport) layer, right?  Is that because that's the only case where we have tiles with non-empty tileRects and no texture IDs?

backgroundCoversViewport should really just use the background color.  That was originally intended for the use case where for example you're zooming a page that's too short to fill the entire viewport when its width is 100%.  In that case, it really should be the background color.  There's just no content at all, not missing content.

In all other cases, we are generating new-style checkerboards, whether we are on the root layer or on other layers.  So, if you have some super large non-animating non-root layer and you scroll down and there's content that's not painted yet, you will see new-style checkerboards.
Comment 13 James Robinson 2012-04-05 15:04:34 PDT
Oh, so we could end up with new-style checkerboards on top of other layers or with odd transforms applied (instead of just skipping as we do now)?  That seems like it could be surprising.
Comment 14 Adrienne Walker 2012-04-05 16:26:41 PDT
Created attachment 135931 [details]
Only checkerboard root, rename usedCheckerboard
Comment 15 James Robinson 2012-04-09 14:48:27 PDT
Comment on attachment 135931 [details]
Only checkerboard root, rename usedCheckerboard

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

Looks great. R=me

> Source/WebKit/chromium/tests/LayerChromiumTest.cpp:502
> +    EXECUTE_AND_VERIFY_SET_NEEDS_COMMIT_BEHAVIOR(0, testLayer->setDrawCheckerboardForMissingTiles(!testLayer->drawCheckerboardForMissingTiles()));

Changing this might change how the layer draws, so it probably _should_ trigger needsDisplay / needsCommit (although in practice this won't matter since it's effectively static state)
Comment 16 Adrienne Walker 2012-04-09 15:01:39 PDT
(In reply to comment #15)
> (From update of attachment 135931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135931&action=review
> 
> Looks great. R=me
> 
> > Source/WebKit/chromium/tests/LayerChromiumTest.cpp:502
> > +    EXECUTE_AND_VERIFY_SET_NEEDS_COMMIT_BEHAVIOR(0, testLayer->setDrawCheckerboardForMissingTiles(!testLayer->drawCheckerboardForMissingTiles()));
> 
> Changing this might change how the layer draws, so it probably _should_ trigger needsDisplay / needsCommit (although in practice this won't matter since it's effectively static state)

No, you're right.  I'll change that before landing.
Comment 17 Adrienne Walker 2012-04-09 15:19:45 PDT
Created attachment 136314 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-04-09 19:56:10 PDT
Comment on attachment 136314 [details]
Patch for landing

Rejecting attachment 136314 [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:
rce/WebKit/chromium/src/NonCompositedContentHost.cpp
patching file Source/WebKit/chromium/tests/CCQuadCullerTest.cpp
Hunk #1 succeeded at 86 (offset 4 lines).
patching file Source/WebKit/chromium/tests/CCSolidColorLayerImplTest.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/12371595
Comment 19 Adrienne Walker 2012-04-11 09:21:25 PDT
Committed r113871: <http://trac.webkit.org/changeset/113871>
Comment 20 John Bauman 2012-04-11 12:55:21 PDT
Reopening to attach new patch.
Comment 21 John Bauman 2012-04-11 12:55:23 PDT
Created attachment 136730 [details]
Patch
Comment 22 John Bauman 2012-04-11 12:56:18 PDT
webkit-patch upload did something horrible.