Bug 80173 – [chromium] Use opaque paints in CCOcclusionTracker

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

Make sure this unit test is covered http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests/TiledLayerChromiumTest.cpp?rev=109584#L647

[Dana Jansens]

Created attachment 130650 [details]
Patch

[Dana Jansens]

Bugs filed for fixmes:
http://code.google.com/p/chromium/issues/detail?id=117203
http://code.google.com/p/chromium/issues/detail?id=117165

[Dana Jansens]

Created attachment 130848 [details]
Patch

Missing a virtual. And rebased for paint-culling CL landing.

[Adrienne Walker]

Comment on attachment 130848 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:120
> +    // FIXME: Don't lose pixels with tile boundaries inside them.

This is because of the enclosedIntRect, yeah? Can you just transform post-unite rather than pre-unite?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:127
> +            if (tile) {

style nit: "if (!tile) continue;" (early outs and less indenting is better)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
> +    occlusionTracker.setUsePaintTracking(false); // FIXME: Remove this to turn on paint tracking for paint culling

Is there some larger picture I'm missing as to why you want to land this with paint culling turned off? (I understand leaving a switch to make it easily disabled if need be, just curious about the initial state of that switch.)

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.h:59
> +    virtual Region opaqueContentsRegion(const TransformationMatrix& contentSpaceTransform) const;

I find it a little bit awkward that this function takes a transform.  I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker.  However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up?

Can you also leave some comments about what spaces this function is dealing with?

[Dana Jansens]

Comment on attachment 130848 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:120
>> +    // FIXME: Don't lose pixels with tile boundaries inside them.
> 
> This is because of the enclosedIntRect, yeah? Can you just transform post-unite rather than pre-unite?

Yes, I could stick them all in a Region, then transform each rect in the region. That is probably what I'd like to do, but I'd like to in a follow up CL?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:127
>> +            if (tile) {
> 
> style nit: "if (!tile) continue;" (early outs and less indenting is better)

k!

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:543
>> +    occlusionTracker.setUsePaintTracking(false); // FIXME: Remove this to turn on paint tracking for paint culling
> 
> Is there some larger picture I'm missing as to why you want to land this with paint culling turned off? (I understand leaving a switch to make it easily disabled if need be, just curious about the initial state of that switch.)

Mostly for bisect. I'd like to watch paint culling not cause bugs for a little bit on canaries, then turn it on, and we can bisect to the switch flip for problems.

>> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.h:59
>> +    virtual Region opaqueContentsRegion(const TransformationMatrix& contentSpaceTransform) const;
> 
> I find it a little bit awkward that this function takes a transform.  I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker.  However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up?
> 
> Can you also leave some comments about what spaces this function is dealing with?

I'll add a comment. The reason is the function can take both a contentToTargetSpace and a contentToScreenSpace transform. Taking a transform as parameter lets one function do both.

I could pass a bool and compute the transform inside, but that feels less intuitive to me, wdyt?

[Dana Jansens]

(In reply to comment #5)
> > I find it a little bit awkward that this function takes a transform.  I'm guessing you're trying to save recalculating the transform since you have it in the occlusion tracker.  However, isn't this value always calculated from values on the layer itself, and you'll never pass anything but that same value to a given layer? If so, is there some way to clean this up?
> > 
> > Can you also leave some comments about what spaces this function is dealing with?
> 
> I'll add a comment. The reason is the function can take both a contentToTargetSpace and a contentToScreenSpace transform. Taking a transform as parameter lets one function do both.
> 
> I could pass a bool and compute the transform inside, but that feels less intuitive to me, wdyt?

Further.. The reason I pass in a transform at all is because I wanted to do this transform before constructing a Region to prevent having to use an intermediate Region in the content space.

If we're going to stick them all together in content space to avoid losing pixels, then I could just return that and transform it elsewhere. Better?

[Dana Jansens]

Created attachment 131051 [details]
Patch

[Dana Jansens]

- Removed transform.
- Removed FIXME for missing pixels.

[Adrienne Walker]

Comment on attachment 131051 [details]
Patch

Thanks for those changes! That looks much simpler to my eyes.

I'm still a little confused about the flag switch, since this patch seems like a no-op to me without it turned on.  What bugs are you hoping to catch?

[Dana Jansens]

You're right about no-op, but I'm hoping to land draw side using this soonish. And wanted to do so without flipping paint side.

I'm not sure.. I don't really expect any bugs, but one never does.. Just trying to be a little conservative with it.

[Adrienne Walker]

(In reply to comment #10)
> You're right about no-op, but I'm hoping to land draw side using this soonish. And wanted to do so without flipping paint side.
> 
> I'm not sure.. I don't really expect any bugs, but one never does.. Just trying to be a little conservative with it.

Ok! I just wanted to make sure that I was clear on the larger picture.  :)

[WebKit Review Bot]

Comment on attachment 131051 [details]
Patch

Clearing flags on attachment: 131051

Committed r110317: <http://trac.webkit.org/changeset/110317>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.