Bug 82251 – [chromium] Layers should should know their visibility outside their content bounds

• Component: CSS
• Status: RESOLVED WONTFIX

[Eric Penner]

Currently we clip the visible rect to layers content bounds here:

CCLayerTreeHostCommon.cpp:
...
CCLayerTreeHostCommon::calculateVisibleRect
...
layerRect.intersect(layerBoundRect);
...

This prevents us from doing pre-painting when a layer is isn't visible yet. We should change this rect to be unclipped if we can, and modify usage of this rect to clip it if unnecessary. Alternatively we could store another unclipped rect along-side the clipped rect.

[Eric Penner]

(In reply to comment #0)
> Currently we clip the visible rect to layers content bounds here:
> 
> CCLayerTreeHostCommon.cpp:
> ...
> CCLayerTreeHostCommon::calculateVisibleRect
> ...
> layerRect.intersect(layerBoundRect);
> ...
> 
> This prevents us from doing pre-painting when a layer is isn't visible yet. We should change this rect to be unclipped if we can, and modify usage of this rect to clip it if unnecessary. Alternatively we could store another unclipped rect along-side the clipped rect.

This should help with 82117, so we can pre-paint animated layers the same way we pre-paint other layers (or even more aggressively, but still based on distance from view).

[Dana Jansens]

We probably want a second unclippedLayerRect? visibleLayerRect is used a lot and the implicit clip is depended on all over the place. It's a bit of a special case to care about the unclipped value.

[Eric Penner]

(In reply to comment #2)
> We probably want a second unclippedLayerRect? visibleLayerRect is used a lot and the implicit clip is depended on all over the place. It's a bit of a special case to care about the unclipped value.

Yeah, that probably makes sense then.

[Adrienne Walker]

Isn't this just the target surface's clip rect transformed into the space of the layer contents? It doesn't seem like you need another variable for that.

[Dana Jansens]

(In reply to comment #4)
> Isn't this just the target surface's clip rect transformed into the space of the layer contents? It doesn't seem like you need another variable for that.

Urg, thanks Enne I think I missed the ball on this. I think what you're asking for Eric us just IntRect(IntPoint(), contentBounds())? visibleLayerRect is in the content space of the layer, so that is the whole content space.

Are you thinking something in screen space or target surface space? We can just transform the above rect to find that with the layer's screenSpaceTransform or originTransform.

[Eric Penner]

(In reply to comment #5)
> (In reply to comment #4)
> > Isn't this just the target surface's clip rect transformed into the space of the layer contents? It doesn't seem like you need another variable for that.
> 
> Urg, thanks Enne I think I missed the ball on this. I think what you're asking for Eric us just IntRect(IntPoint(), contentBounds())? visibleLayerRect is in the content space of the layer, so that is the whole content space.
> 
> Are you thinking something in screen space or target surface space? We can just transform the above rect to find that with the layer's screenSpaceTransform or originTransform.

Sorry, I was just tracing back to when the visible rect got clipped, as that is what we are currently using and causing the limitation. I wasn't aware we can just use the targetRenderSurface(), grab it's clip rect, and transform that into the layers space. 

So, it seems like we should use that as a replacement for visibleRect then no? It would also have the advantage of not having the axis-aligned bounding box applied, in case we wanted the exact transformed rect.

[Eric Penner]

(In reply to comment #5)
> (In reply to comment #4)
> > Isn't this just the target surface's clip rect transformed into the space of the layer contents? It doesn't seem like you need another variable for that.
> 
> Urg, thanks Enne I think I missed the ball on this. I think what you're asking for Eric us just IntRect(IntPoint(), contentBounds())? visibleLayerRect is in the content space of the layer, so that is the whole content space.
> 
> Are you thinking something in screen space or target surface space? We can just transform the above rect to find that with the layer's screenSpaceTransform or originTransform.

I think Enne's suggestion is different than this (which is effectively the same as the unclipped visible rect, right Enne?).. Just without the need to compute/store it when we compute the visible rect.

[Eric Penner]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Isn't this just the target surface's clip rect transformed into the space of the layer contents? It doesn't seem like you need another variable for that.
> > 
> > Urg, thanks Enne I think I missed the ball on this. I think what you're asking for Eric us just IntRect(IntPoint(), contentBounds())? visibleLayerRect is in the content space of the layer, so that is the whole content space.
> > 
> > Are you thinking something in screen space or target surface space? We can just transform the above rect to find that with the layer's screenSpaceTransform or originTransform.
> 
> I think Enne's suggestion is different than this (which is effectively the same as the unclipped visible rect, right Enne?).. Just without the need to compute/store it when we compute the visible rect.

Sorry, one last question.. Are render surfaces always clipped to the viewport? Ideally what we actually want is the viewport transformed into content space, so we can expand that, which will expand into the content space even if the viewport was initially outside of it.

[Dana Jansens]

> I think Enne's suggestion is different than this (which is effectively the same as the unclipped visible rect, right Enne?).. Just without the need to compute/store it when we compute the visible rect.

intersection(what Enne suggested, what I suggested) == visibleLayerRect. :)

> Sorry, one last question.. Are render surfaces always clipped to the viewport? Ideally what we actually want is the viewport transformed into content space, so we can expand that, which will expand into the content space even if the viewport was initially outside of it.

Yes and no, the root layer clips everything to itself. This may be equal to the viewport or may be smaller than it (aspect ratio etc).

I think what we want is the target surface though, not the viewport. And then there should be some recursive behaviour. If a layer is just outside of its target surface then it is close to being visible, iff its surface is visible/close to being visible at that same point.

Simplified idea: visibility(Tile T) = distance outside target surface * visibility(closest tile in target surface)

[Adrienne Walker]

(In reply to comment #6)

> So, it seems like we should use that as a replacement for visibleRect then no? It would also have the advantage of not having the axis-aligned bounding box applied, in case we wanted the exact transformed rect.

I would need to be convinced that this could be a replacement for visibleRect.  It seems to me that we'd need to add a bunch of intersect(contentBounds()) calls elsewhere.

(In reply to comment #7)

> I think Enne's suggestion is different than this (which is effectively the same as the unclipped visible rect, right Enne?).. Just without the need to compute/store it when we compute the visible rect.

Yes (and yes).

(In reply to comment #8)

> Sorry, one last question.. Are render surfaces always clipped to the viewport? Ideally what we actually want is the viewport transformed into content space, so we can expand that, which will expand into the content space even if the viewport was initially outside of it.

I'm not sure that you really want the viewport.  What if a surface masks to bounds and something is animating inside of that? That's why I was thinking clip rect and not viewport.

(In reply to comment #9)

> Simplified idea: visibility(Tile T) = distance outside target surface * visibility(closest tile in target surface)

That's along the lines of what I was thinking too.  If you project the target surface's clip rect into a FloatQuad in content space, you can make a prioritization function based on distance from tile rect to that projected clip rect, with the assumption that tiles that are closer to the clip rect will be visible first.  You might additionally be able to add in animation velocity as a second check and do a separating axes test to make sure that the tile rect will ever intersect the clip rect.

[Dana Jansens]

(In reply to comment #10)
> (In reply to comment #6)
> 
> > So, it seems like we should use that as a replacement for visibleRect then no? It would also have the advantage of not having the axis-aligned bounding box applied, in case we wanted the exact transformed rect.
> 
> I would need to be convinced that this could be a replacement for visibleRect.  It seems to me that we'd need to add a bunch of intersect(contentBounds()) calls elsewhere.

FWIW offtopictangent I know Shawn was thinking about trying to do all computation in target space instead of layer/content space when possible. This would align with that. Maybe we wouldn't need to intersect contentBounds() much if done right..? I haven't really thought about it much myself.

[Eric Penner]

(In reply to comment #10)

> I would need to be convinced that this could be a replacement for visibleRect.  It seems to me that we'd need to add a bunch of intersect(contentBounds()) calls elsewhere.

Well we still have the clipped visible rect already if it suffices for those cases. But otherwise we can just store both if we are repeating the calculation a lot... Each usage would need to pick the appropriate rect.

> I'm not sure that you really want the viewport.  What if a surface masks to bounds and something is animating inside of that? That's why I was thinking clip rect and not viewport.

Yeah, I thought of that right after my comment. Although primary concern still being that the target surface is at least clipped to the viewport (if not clipped further).

> > Simplified idea: visibility(Tile T) = distance outside target surface * visibility(closest tile in target surface)
> 
> That's along the lines of what I was thinking too.  If you project the target surface's clip rect into a FloatQuad in content space, you can make a prioritization function based on distance from tile rect to that projected clip rect, with the assumption that tiles that are closer to the clip rect will be visible first.  You might additionally be able to add in animation velocity as a second check and do a separating axes test to make sure that the tile rect will ever intersect the clip rect.

This was my thinking as well.

[Eric Penner]

Created attachment 152655 [details]
Patch

[Eric Penner]

Food for thought. I used basically the same idea (although implemented inside TiledLayerChromium) on Android to paint off-screen animated layers. I put this up very early so we can decide if this is even the right approach first. I think we'll need something like this though so let's discuss. 

We can further refine tile distacnes further by projecting each tile contained within this rect into it's target surface, and then measuring the distance-from-view there.

[Eric Penner]

Comment on attachment 152655 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:53
>      // the surface rect where the layer could be visible. This avoids trying to

I was worried about removing this for perspective projections, but I think it's valid now
since we now use projectClippedRect below, which will clip when points are behind the
projection point.

[Dana Jansens]

Comment on attachment 152655 [details]
Patch

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

To be clear, what do you mean when you say "unclipped" here? There are many things that clip.. The target, the viewport, the layer's bounds?

I see the last one being the case in the code? Is that right and what about the others?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:64
> -    IntRect layerRect = enclosingIntRect(CCMathUtil::projectClippedRect(surfaceToLayer, FloatRect(minimalSurfaceRect)));
> +    IntRect layerRect = enclosingIntRect(CCMathUtil::projectClippedRect(surfaceToLayer, FloatRect(targetSurfaceRect)));

That looks wrong since you change minimalSurfaceRect conditionally above?

[Eric Penner]

Comment on attachment 152655 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:64
>> +    IntRect layerRect = enclosingIntRect(CCMathUtil::projectClippedRect(surfaceToLayer, FloatRect(targetSurfaceRect)));
> 
> That looks wrong since you change minimalSurfaceRect conditionally above?

Whoops, that was from a previous revision where I removed it entirely. It should be minimalSurfaceRect still.

[Eric Penner]

Created attachment 152661 [details]
Patch

[Eric Penner]

> To be clear, what do you mean when you say "unclipped" here? There are many things that clip.. The target, the viewport, the layer's bounds?
> 
> I see the last one being the case in the code? Is that right and what about the others?
> 

I think a better name is in order but I couldn't think of one. It should mean "not clipped by content bounds", although I'm still trying to fully understand the uses of the layer's clip rect since I've also removed the clipRect(). It was previously only for animated layers but when used for all layers it's important that it doesn't loosen the visible bound.

[Eric Penner]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #6)
> > 
> > > So, it seems like we should use that as a replacement for visibleRect then no? It would also have the advantage of not having the axis-aligned bounding box applied, in case we wanted the exact transformed rect.
> > 
> > I would need to be convinced that this could be a replacement for visibleRect.  It seems to me that we'd need to add a bunch of intersect(contentBounds()) calls elsewhere.
> 
> FWIW offtopictangent I know Shawn was thinking about trying to do all computation in target space instead of layer/content space when possible. This would align with that. Maybe we wouldn't need to intersect contentBounds() much if done right..? I haven't really thought about it much myself.

Sorry what does tangent space mean here? Tangent space makes me think of bump-mapping ;)

[Adrienne Walker]

Comment on attachment 152661 [details]
Patch

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

I think unclippedVisibleContentRect is a reasonable name if there's a comment with it in LayerChromium.h that says something like it's the projection of the visible rect into content space, but that it's not clipped to the layer's content bounds.

This seriously needs some tests.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:57
> +    if (clip == ClipVisibleRect)
> +        minimalSurfaceRect.intersect(layerInSurfaceSpace);

Can you convince me that this does anything? I would have figured that the shift to projectClippedRect would make this particular intersection a no-op and, if anything, this only increases the accuracy of the resulting visible rect, unclipped or not.

Do you also not need to avoid the other intersection with the layerBoundRect below on line 65? Wouldn't that make the unclipped visible rect empty for anything that didn't intersect with the content rect? I thought that's what the whole point of this was.

[Eric Penner]

Comment on attachment 152661 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:57
>> +        minimalSurfaceRect.intersect(layerInSurfaceSpace);
> 
> Can you convince me that this does anything? I would have figured that the shift to projectClippedRect would make this particular intersection a no-op and, if anything, this only increases the accuracy of the resulting visible rect, unclipped or not.
> 
> Do you also not need to avoid the other intersection with the layerBoundRect below on line 65? Wouldn't that make the unclipped visible rect empty for anything that didn't intersect with the content rect? I thought that's what the whole point of this was.

For the first point, it definitely looks like this was written before projectClippedRect so the comment doesn't seem relevant anymore (and this can probably be safely removed). However extra clip could possibly make the AABB around the projected visible area tighter in some cases... I'm inclined to not worry about that, or to improve it elsewhere (like projecting each tile in to the render surface to compute final tile visibility) instead.

About the second point, yes!! Bad port from my downstream change. That should be conditional too. I'll obviously need some tests for this :)

[Eric Penner]

Created attachment 154124 [details]
reboot

[Eric Penner]

This was way more complicated than I initially thought after digging into it.

However, the new solution is more elegant I think, and doesn't need the extra "unclipped visible rect" and extra code-path associated with that. So it can now be tested together with the many other tests that test the correctness of the visibleContentRect().

Unfortunately I was side-tracked last week and now I'm sheriff for the next two days, so I'm putting this up for early review of the general concept.

[Adrienne Walker]

Comment on attachment 154124 [details]
reboot

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

I'm a little dubious of using IntRects with a valid location but an empty bounds, as empty IntRects are sometimes handled specially.

Also, I found the previous approach a lot more understandable.  I thought that seemed pretty close to being done.  Why reboot?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:94
> +IntRect CCLayerTreeHostCommon::calculateVisibleRect(const IntRect& targetSurfaceRect, const IntRect& layerBoundRect, const WebTransformationMatrix& transform, int& accumulatedDistance)

Is accumulatedDistance just used for test code?

[Eric Penner]

(In reply to comment #25)
> (From update of attachment 154124 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154124&action=review
> 
> I'm a little dubious of using IntRects with a valid location but an empty bounds, as empty IntRects are sometimes handled specially.
> 

I was thinking the same about using special empty rects as well, except I thought it was still worth it given:
- The line/point that is represented by the empty rect is actually required to calculate correct distances when there is multiple clipping of the visible rect in the layer hierarchy.
- We only change those rects in CCLayerTreeHostCommon so we can insure the special empty rects don't get corrupted.
- The empty rect cases are still the same, so code that doesn't care about the location of the empty rect will behave the same.
- We use the same code path that is heavily tested in the real world (instead of heavily testing visibleRect() but then using unclippedVisibleRect() in practice).

> Also, I found the previous approach a lot more understandable.  I thought that seemed pretty close to being done.  Why reboot?

The problem is that it was very naive and only worked with one layer/target effectively. It could only calculate the distance to the target render surface. If you look in the verifyVisibleRectOutsideContentBounds test, layer "3" is in the middle of the render surface, but it is very far from being visible, since layer "2" has maskToBounds enabled. It would be the same if layer "2" was a render surface. The old way, these cases would just result in an empty unclippedVisibleRect(), which defeats it's purpose. We could not apply layer clipping, but that loosens our bounds on what is visible.

[Eric Penner]

Comment on attachment 154124 [details]
reboot

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:94
>> +IntRect CCLayerTreeHostCommon::calculateVisibleRect(const IntRect& targetSurfaceRect, const IntRect& layerBoundRect, const WebTransformationMatrix& transform, int& accumulatedDistance)
> 
> Is accumulatedDistance just used for test code?

This is actually needed as an output from this function. If the rects don't intersect, it adds the minimum distance between the two rects and returns that in addition to the empty rect that is clamped to layerBoundRect. With those to values we can calculate distances to the visible rect from anywhere inside content bounds.

In addition, the process can be repeated (by several clips from ancestors), which will further add to the accumulated distance.

[Eric Penner]

Hmm, as one possibility to hide the use of custom empty rects, maybe I could add a type like "VisibilityTrackingRect" or something. That could encapsulate both accumulating distances from parent clip rects, and hide the implementation so that only valid IntRect operations can be used.  Let me know if you think that would help.

[Adrienne Walker]

Comment on attachment 154124 [details]
reboot

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:686
> +            layer->setAccumulatedDistanceToVisible(layer->parent()->accumulatedDistanceToVisible());

This calculation seems wrong.  What if there's a transform that doesn't preserve linear distance between the parent and this layer?

Also, where does animation fit into here? It seems like this would be wrong for animated layers on the main thread where positions aren't known.

Also, given that only animations and scrolling can change layer positions, there are some cases where it seems like distance to visible should be essentially infinite.

[Eric Penner]

Comment on attachment 154124 [details]
reboot

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:686
>> +            layer->setAccumulatedDistanceToVisible(layer->parent()->accumulatedDistanceToVisible());
> 
> This calculation seems wrong.  What if there's a transform that doesn't preserve linear distance between the parent and this layer?
> 
> Also, where does animation fit into here? It seems like this would be wrong for animated layers on the main thread where positions aren't known.
> 
> Also, given that only animations and scrolling can change layer positions, there are some cases where it seems like distance to visible should be essentially infinite.

Thanks, yes you are right. I think this assumes that a parent layer will always clip to its bounds. If it doesn't then a parent can have a different accumulated value for it's own distance, unrelated to the child. So the accumulated distance should come from the previous clip rect rather than the parent layer.

About animations, I believe we should have valid locations for animated layers since we keep animations up to date on the main thread such that we can stream in the textures for large layers as the animation occurs.

The last issue I'm uncertain about. The only case I planned to call 'infinite' was when the layer disappears behind the projection point. It seems like all other layers could become visible via layer movements javascript/animation/scrolling. If the layer can't become visible then should we even keep it in the tree?

[Adrienne Walker]

(In reply to comment #30)

> About animations, I believe we should have valid locations for animated layers since we keep animations up to date on the main thread such that we can stream in the textures for large layers as the animation occurs.

I don't think this is true.  The animated values of layers are not known on the main thread while a layer is animating.  If you animate a layer's position, then its position on the main thread is the end point of the animation.

> The last issue I'm uncertain about. The only case I planned to call 'infinite' was when the layer disappears behind the projection point. It seems like all other layers could become visible via layer movements javascript/animation/scrolling. If the layer can't become visible then should we even keep it in the tree?

The question I'm getting at is if we should treat regions of layers that are scrollable and more likely to become visible as being weighted differently than hidden regions of layers that are not scrollable.

[Eric Penner]

(In reply to comment #31)
> (In reply to comment #30)
> 
> > About animations, I believe we should have valid locations for animated layers since we keep animations up to date on the main thread such that we can stream in the textures for large layers as the animation occurs.
> 
> I don't think this is true.  The animated values of layers are not known on the main thread while a layer is animating.  If you animate a layer's position, then its position on the main thread is the end point of the animation.
> 

That was the case initially, but Ian changed how this works since there was no way to paint large animated layers if the position jumped immediately to the end. I've definitely seen that we now get the start position on the first webkit paint, for example.

> 
> The question I'm getting at is if we should treat regions of layers that are scrollable and more likely to become visible as being weighted differently than hidden regions of layers that are not scrollable.

That sounds good to me. Next up will be using scroll direction to weight priorities, so that could also identify scrollable content in general.

[Shawn Singh]

Eric, if it's relevant to you when you rebase for the next patch, there has been some changes in semantics to these rects that will probably affect you.

In particular, clipRect on layers no longer exists.  It still exists in the recursion, and you might be able to use it from there, but the exact values are likely to be different.
  (1) clipRect was a bit "looser" in its bounds before, related to the ambiguous messy drawableContentRect
  (2) drawableContentRect used to be the drawable area of the layer's entire subtree.   Now it actually means "the correctly clipped bounds of the layer".   as before, it's still expressed in the space of the targetSurface.

sometime soon, drawableContentRect might be renamed to "clippedRectInTargetSurfaceSpace", to correctly reflect its new meaning.

I'm happy to discuss more offline, and I'll try to take a closer look at the next patch whenever it comes up =)

[Eric Penner]

(In reply to comment #33)
> Eric, if it's relevant to you when you rebase for the next patch, there has been some changes in semantics to these rects that will probably affect you.
> 
> In particular, clipRect on layers no longer exists.  It still exists in the recursion, and you might be able to use it from there, but the exact values are likely to be different.
>   (1) clipRect was a bit "looser" in its bounds before, related to the ambiguous messy drawableContentRect
>   (2) drawableContentRect used to be the drawable area of the layer's entire subtree.   Now it actually means "the correctly clipped bounds of the layer".   as before, it's still expressed in the space of the targetSurface.
> 
> sometime soon, drawableContentRect might be renamed to "clippedRectInTargetSurfaceSpace", to correctly reflect its new meaning.
> 
> I'm happy to discuss more offline, and I'll try to take a closer look at the next patch whenever it comes up =)

Thanks Shawn, yes I noticed the changes and have rebased but I'm still fixing things up. It should be better/more clear this way if my understanding is correct. I might ping you if I run into any trouble.

[Eric Penner]

Created attachment 156138 [details]
Patch

[Eric Penner]

Created attachment 156147 [details]
tweaks

[Eric Penner]

I have worked out supporting render surfaces but the that was becoming a bit unwieldy, so I think should wait instead for these things as separate CLs:
- Move the visible rect calculation out of the custom pass (Dana mentioned she was going to do this shortly)
- Propagate clip visibility into render surfaces.
- Remove call that removes empty surface subtrees from the tree entirely.

Even without supporting distances to off-screen render surfaces, this still has most of the benefits for painting etc.

[Eric Penner]

I'm not sure who is best to review this, but this CL is ready for review. Shawn, do you think you'll have a chance to look at this soon?

[Shawn Singh]

Sure - thanks for the bump, I'll do it first thing tomorrow morning.  Hopefully I won't be missing too much context surrounding this patch, looks like the discussion is all here.

[Shawn Singh]

Comment on attachment 156147 [details]
tweaks


Very cool.... It looks pretty elegant to me, almost as good as it can be.  And it looks like you bent over backwards to keep the similar style that I was personally pushing for, so thanks for that.

I'm sure you have pondered other options, but I'd like to hear those options you've considered, just in case we can find a solution that won't make me (us?) so uncomfortable about the modifications happening here.  In particular, relying on empty rects having a valid position screams warnings of terrifying debugging tasks in the future.  One possibility - would it work to compute this in a separate additional walk through layers, after visibleLayerRects are computed (even if it has to be recursive)?  I don't know how Enne feels about that, but personally I think it's less error prone and more flexible.  I'm also not convinced that we'll be able to cleanly put visibleLayerRect calculations back into calcDrawTransforms recursion (I used to think so, but now I'm not so sure)

So yeah, Enne's the real reviewer around these parts.  Let's see what they say.

Some more detailed comments below:

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

> Source/WebCore/ChangeLog:9
> +        Add support for computing the distance-to-visible for an arbitrary
> +        point on a layer. This is more difficult when ancestor layers can

It might be nice to quickly hint at the purpose of doing this in the changelog.  I assume the purpose is for allowing pre-painting more intelligently, right?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:59
> +    distanceUnknown |= completelyClipped;

Is there a reason to allow distanceUnknown to remain true even if completelyClipped is false?   My reasoning against this: Seems like you were probably thinking that "if distance is already unknown, let it remain unknown" ?   But, the way this boolean is used in the code, it always starts by initializing the boolean to false before giving it to this function.    I have a bad feeling that we may have some very nasty errors with these semantics later on... someone will input a boolean initialized to true, thinking "it starts unknown, but becomes known after calling this function", and has a frustrating time debugging why it remains unknown after calling.   Unless there's a reason I'm missing, personally think its better to override this value no matter what its original value was, i.e. distanceUnknown = completelyClipped.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-56
> -    IntRect minimalSurfaceRect = targetSurfaceRect;
> -    minimalSurfaceRect.intersect(layerInSurfaceSpace);

Why are we removing these lines?  I think they are quite important for having a tighter bound for visibleContentRect on layers within surfaces.  For example, an div that is contained within a renderSurface that clips to its bounds.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:69
> +    distanceUnknown |= completelyClipped;

We should probably create a bug assigned to me that requires me to rename this clipping concept.  It is not the same as clipRects and might lead to confusing problems.  It looks like you already understand the meaning of this version of "clipped", right?  i.e. the w < 0 issue, nothing to do with 2d clipRects or masksToBounds.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:73
> +    // A non axis-aligned projection could project an empty target surface rect
> +    // (a line) into a non-empty rect in content space.

Should this comment perhaps explain more about why we need this few lines of code?  Specifically, as I understand it, we need to carefully set the visibleRect so that its location is meaningful even though it's size is empty, right?  Otherwise, it feels like there's a slight disconnect between the comment and the code.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:765
> +        drawableContentRectOfLayer = clampRectToBounds(clipRectForSubtree, drawableContentRectOfLayer, accumulatedDistanceToVisible);

I think we need to add the appropriate unit tests for drawableContentRectOfLayer if these are the semantics we really need.   (the alternative would be if we can find a less fragile solution?)

The problem is that most people will blindly assume that we can do normal rect operations on these rects, and many of us will undoubtedly make that mistake while hacking away on this code.   Even if we see this patch, we're going to accidentally forget, and cause another debugging nightmare =)

> Source/WebCore/platform/graphics/chromium/cc/CCVisibleRectUtil.h:64
> +    if (accumulatedDistance != -1)
> +        accumulatedDistance += manhattanDistance(rect, bounds);

I wonder if it would be cleaner to separate accumulatedDistance computation into a separate helper function?   I don't feel too strongly about it, but it does feel a bit unclean to have a third wheel attached to clampRectToBounds() that otherwise is clearly self-documented.

[Eric Penner]

Comment on attachment 156147 [details]
tweaks

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

>> Source/WebCore/ChangeLog:9
>> +        point on a layer. This is more difficult when ancestor layers can
> 
> It might be nice to quickly hint at the purpose of doing this in the changelog.  I assume the purpose is for allowing pre-painting more intelligently, right?

Good point, I'll describe the motivation here.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:59
>> +    distanceUnknown |= completelyClipped;
> 
> Is there a reason to allow distanceUnknown to remain true even if completelyClipped is false?   My reasoning against this: Seems like you were probably thinking that "if distance is already unknown, let it remain unknown" ?   But, the way this boolean is used in the code, it always starts by initializing the boolean to false before giving it to this function.    I have a bad feeling that we may have some very nasty errors with these semantics later on... someone will input a boolean initialized to true, thinking "it starts unknown, but becomes known after calling this function", and has a frustrating time debugging why it remains unknown after calling.   Unless there's a reason I'm missing, personally think its better to override this value no matter what its original value was, i.e. distanceUnknown = completelyClipped.

I agree, in this function it makes more sense as a straight output (rather than input/output).

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-56
>> -    minimalSurfaceRect.intersect(layerInSurfaceSpace);
> 
> Why are we removing these lines?  I think they are quite important for having a tighter bound for visibleContentRect on layers within surfaces.  For example, an div that is contained within a renderSurface that clips to its bounds.

It looks like the issue in the comment was solved by using projectClippedRect so this is why I removed it.

The one advantage that remained in my mind was for a non-axis aligned projection, since we take the bounding box of the projected surface below (so anything smaller would reduce the error in this bound). If this is the advantage you are referring to and you think it's important I'll look into it deeper. The intersect() is what was causing problems but maybe I can work around that in a different way.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:69
>> +    distanceUnknown |= completelyClipped;
> 
> We should probably create a bug assigned to me that requires me to rename this clipping concept.  It is not the same as clipRects and might lead to confusing problems.  It looks like you already understand the meaning of this version of "clipped", right?  i.e. the w < 0 issue, nothing to do with 2d clipRects or masksToBounds.

I think the meaning is clear given the context. It's clipped by a different plane but still clipped. But if there is something more clear I'm all for it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:73
>> +    // (a line) into a non-empty rect in content space.
> 
> Should this comment perhaps explain more about why we need this few lines of code?  Specifically, as I understand it, we need to carefully set the visibleRect so that its location is meaningful even though it's size is empty, right?  Otherwise, it feels like there's a slight disconnect between the comment and the code.

Yeah, since the previously empty rect's position was important, this tries to maintain that location, while insuring nobody confuses this as being visible area. I'll add a comment about that.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:765
>> +        drawableContentRectOfLayer = clampRectToBounds(clipRectForSubtree, drawableContentRectOfLayer, accumulatedDistanceToVisible);
> 
> I think we need to add the appropriate unit tests for drawableContentRectOfLayer if these are the semantics we really need.   (the alternative would be if we can find a less fragile solution?)
> 
> The problem is that most people will blindly assume that we can do normal rect operations on these rects, and many of us will undoubtedly make that mistake while hacking away on this code.   Even if we see this patch, we're going to accidentally forget, and cause another debugging nightmare =)

Yeah good point, the trouble is that this is used later to calculate the visible rect. If we could compute the visible rect directly in here then we wouldn't have any additional constraint on this rect. Changing this would break the visible rect though so I think it is still (indirectly) tested. I can add checks for this rect within the visibleRect test below.

I'm also assuming that the users of these rects (drawableContentRect/visibleContentRect) won't get mixed up by the empty rect having a non zero position. This seems like it should be okay since code always uses isEmpty() etc.

>> Source/WebCore/platform/graphics/chromium/cc/CCVisibleRectUtil.h:64
>> +        accumulatedDistance += manhattanDistance(rect, bounds);
> 
> I wonder if it would be cleaner to separate accumulatedDistance computation into a separate helper function?   I don't feel too strongly about it, but it does feel a bit unclean to have a third wheel attached to clampRectToBounds() that otherwise is clearly self-documented.

Hmm, I pictured these as being the same operation but looks like they could just as efficiently be two functions more clear functions. Maybe I could add accumulateManhattanDistance, which takes the extra parameter and checks for -1.  Then each function only has one return value as well.

[Shawn Singh]

Comment on attachment 156147 [details]
tweaks

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-56
>>> -    minimalSurfaceRect.intersect(layerInSurfaceSpace);
>> 
>> Why are we removing these lines?  I think they are quite important for having a tighter bound for visibleContentRect on layers within surfaces.  For example, an div that is contained within a renderSurface that clips to its bounds.
> 
> It looks like the issue in the comment was solved by using projectClippedRect so this is why I removed it.
> 
> The one advantage that remained in my mind was for a non-axis aligned projection, since we take the bounding box of the projected surface below (so anything smaller would reduce the error in this bound). If this is the advantage you are referring to and you think it's important I'll look into it deeper. The intersect() is what was causing problems but maybe I can work around that in a different way.

Was it a problem that the rect may become empty and lose the desired position information?  That should be workaroundable.  If not, what was the exact problem?

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:765
>>> +        drawableContentRectOfLayer = clampRectToBounds(clipRectForSubtree, drawableContentRectOfLayer, accumulatedDistanceToVisible);
>> 
>> I think we need to add the appropriate unit tests for drawableContentRectOfLayer if these are the semantics we really need.   (the alternative would be if we can find a less fragile solution?)
>> 
>> The problem is that most people will blindly assume that we can do normal rect operations on these rects, and many of us will undoubtedly make that mistake while hacking away on this code.   Even if we see this patch, we're going to accidentally forget, and cause another debugging nightmare =)
> 
> Yeah good point, the trouble is that this is used later to calculate the visible rect. If we could compute the visible rect directly in here then we wouldn't have any additional constraint on this rect. Changing this would break the visible rect though so I think it is still (indirectly) tested. I can add checks for this rect within the visibleRect test below.
> 
> I'm also assuming that the users of these rects (drawableContentRect/visibleContentRect) won't get mixed up by the empty rect having a non zero position. This seems like it should be okay since code always uses isEmpty() etc.

This could be another reason to make this another post-walk instead of doing it in the existing recursion?   I know it might be a bit more code/computation, but it seems overall more intuitive and robust that way.  But I'll defer to Enne's opinion about this.

[Eric Penner]

(In reply to comment #40)
> (From update of attachment 156147 [details])
> 
> Very cool.... It looks pretty elegant to me, almost as good as it can be.  And it looks like you bent over backwards to keep the similar style that I was personally pushing for, so thanks for that.
> 
> I'm sure you have pondered other options, but I'd like to hear those options you've considered, just in case we can find a solution that won't make me (us?) so uncomfortable about the modifications happening here.  In particular, relying on empty rects having a valid position screams warnings of terrifying debugging tasks in the future.  One possibility - would it work to compute this in a separate additional walk through layers, after visibleLayerRects are computed (even if it has to be recursive)?  I don't know how Enne feels about that, but personally I think it's less error prone and more flexible.  I'm also not convinced that we'll be able to cleanly put visibleLayerRect calculations back into calcDrawTransforms recursion (I used to think so, but now I'm not so sure)
> 

I became convinced we can't solve this without the point/lines that are currently stored inside the empty rects. We could maybe put those somewhere else, but in the visible case we do need a rect, so a rect that is sometimes a point/line seems to make the most sense.

One idea for insuring nobody does an accidental intersect() which destroys the position information would be to use a custom type that doesn't even allow normal rect operations. However, that type quickly creeps into a lot of places given the current structure, which I thought would get messy.

The issue with another tree walk is that it seems like it would repeat several operations. However, if the visible rect need to stay in it's own tree walk then maybe all this code could move there (and it would need to become recursive etc.)

Personally, I'm not that averse to having custom empty rects since they are still valid rects.. Users don't need to worry about it if all they are about is non-empty rects.. But but any code that calculates them needs to take the position into account.

[Shawn Singh]

> Personally, I'm not that averse to having custom empty rects since they are still valid rects.. Users don't need to worry about it if all they are about is non-empty rects.. But but any code that calculates them needs to take the position into account.

Brainstorming a few examples of mistakes that can happen:
(1) in the process of GTFO, we migrate to a different rect data type that has subtly different semantics with operations on empty rects affecting their position.
(2) in the process of modifying calcDrawTransforms, perhaps someone creates a temporary variable, and thinks it's OK to re-assign drawableContentRect to the new value, not realizing that position was important
(3) if we have to change the geometric space that something is represented, causing position to go wrong not realizing that we have to translate an empty rect

The more I think about it, the more I feel the probability that someone will modify this incorrectly is very high.

Tests that explicitly check position of empty drawableContentRects under a few scenarios would help greatly, so maybe it's not an issue that would block this patch, though.

Enne, it's definitely your turn to chime in.  Other than this concern, seems OK to me. =)

[Eric Penner]

Comment on attachment 156147 [details]
tweaks

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

>>> Source/WebCore/ChangeLog:9
>>> +        point on a layer. This is more difficult when ancestor layers can
>> 
>> It might be nice to quickly hint at the purpose of doing this in the changelog.  I assume the purpose is for allowing pre-painting more intelligently, right?
> 
> Good point, I'll describe the motivation here.

Done.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:59
>>> +    distanceUnknown |= completelyClipped;
>> 
>> Is there a reason to allow distanceUnknown to remain true even if completelyClipped is false?   My reasoning against this: Seems like you were probably thinking that "if distance is already unknown, let it remain unknown" ?   But, the way this boolean is used in the code, it always starts by initializing the boolean to false before giving it to this function.    I have a bad feeling that we may have some very nasty errors with these semantics later on... someone will input a boolean initialized to true, thinking "it starts unknown, but becomes known after calling this function", and has a frustrating time debugging why it remains unknown after calling.   Unless there's a reason I'm missing, personally think its better to override this value no matter what its original value was, i.e. distanceUnknown = completelyClipped.
> 
> I agree, in this function it makes more sense as a straight output (rather than input/output).

Done.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:-56
>>>> -    minimalSurfaceRect.intersect(layerInSurfaceSpace);
>>> 
>>> Why are we removing these lines?  I think they are quite important for having a tighter bound for visibleContentRect on layers within surfaces.  For example, an div that is contained within a renderSurface that clips to its bounds.
>> 
>> It looks like the issue in the comment was solved by using projectClippedRect so this is why I removed it.
>> 
>> The one advantage that remained in my mind was for a non-axis aligned projection, since we take the bounding box of the projected surface below (so anything smaller would reduce the error in this bound). If this is the advantage you are referring to and you think it's important I'll look into it deeper. The intersect() is what was causing problems but maybe I can work around that in a different way.
> 
> Was it a problem that the rect may become empty and lose the desired position information?  That should be workaroundable.  If not, what was the exact problem?

Done.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:73
>>> +    // (a line) into a non-empty rect in content space.
>> 
>> Should this comment perhaps explain more about why we need this few lines of code?  Specifically, as I understand it, we need to carefully set the visibleRect so that its location is meaningful even though it's size is empty, right?  Otherwise, it feels like there's a slight disconnect between the comment and the code.
> 
> Yeah, since the previously empty rect's position was important, this tries to maintain that location, while insuring nobody confuses this as being visible area. I'll add a comment about that.

Done.

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:765
>>>> +        drawableContentRectOfLayer = clampRectToBounds(clipRectForSubtree, drawableContentRectOfLayer, accumulatedDistanceToVisible);
>>> 
>>> I think we need to add the appropriate unit tests for drawableContentRectOfLayer if these are the semantics we really need.   (the alternative would be if we can find a less fragile solution?)
>>> 
>>> The problem is that most people will blindly assume that we can do normal rect operations on these rects, and many of us will undoubtedly make that mistake while hacking away on this code.   Even if we see this patch, we're going to accidentally forget, and cause another debugging nightmare =)
>> 
>> Yeah good point, the trouble is that this is used later to calculate the visible rect. If we could compute the visible rect directly in here then we wouldn't have any additional constraint on this rect. Changing this would break the visible rect though so I think it is still (indirectly) tested. I can add checks for this rect within the visibleRect test below.
>> 
>> I'm also assuming that the users of these rects (drawableContentRect/visibleContentRect) won't get mixed up by the empty rect having a non zero position. This seems like it should be okay since code always uses isEmpty() etc.
> 
> This could be another reason to make this another post-walk instead of doing it in the existing recursion?   I know it might be a bit more code/computation, but it seems overall more intuitive and robust that way.  But I'll defer to Enne's opinion about this.

Rather than using drawableContentRect, I used the visibleRect to store the value which is then transformed in the later pass. This way the drawableContentRect is not changed.

Regarding another pass, I'm a bit concerned that it would be a lot of copied code from this pass, and it would have to be recursive like this one, rather than a simple walk like the current visibleRect pass. I'm still also confused about why the visible rect can't be calculated in this pass. It looks like it only needs one value from the render surface, which should already be available, and the drawableContentRect. Can you elaborate on why it was done in a separate pass and reasons why it needs to stay there?

>>> Source/WebCore/platform/graphics/chromium/cc/CCVisibleRectUtil.h:64
>>> +        accumulatedDistance += manhattanDistance(rect, bounds);
>> 
>> I wonder if it would be cleaner to separate accumulatedDistance computation into a separate helper function?   I don't feel too strongly about it, but it does feel a bit unclean to have a third wheel attached to clampRectToBounds() that otherwise is clearly self-documented.
> 
> Hmm, I pictured these as being the same operation but looks like they could just as efficiently be two functions more clear functions. Maybe I could add accumulateManhattanDistance, which takes the extra parameter and checks for -1.  Then each function only has one return value as well.

Done.

[Eric Penner]

Created attachment 158730 [details]
Patch

[Eric Penner]

Comment on attachment 158730 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [Chromium] IndexedDB: Enable compression of leveldb blocks

I'll fix the title on next patch.

[Shawn Singh]

(In reply to comment #45)
> 
> Regarding another pass, I'm a bit concerned that it would be a lot of copied code from this pass, and it would have to be recursive like this one, rather than a simple walk like the current visibleRect pass. I'm still also confused about why the visible rect can't be calculated in this pass. It looks like it only needs one value from the render surface, which should already be available, and the drawableContentRect. Can you elaborate on why it was done in a separate pass and reasons why it needs to stay there?
> 

Originally it was done in a separate pass for getting things done at the time.  For a while, we were thinking that it could technically be computed in the main recursion.

However, recently when I think about what information is actually needed to compute a decent visibleRect, it seems like we _cannot_ do it in the recursion.  Specifically:  the visibleRect should be the intersection of the surface's contentRect and the layer's bounds (after transforming things into the same space so they can be intersected correctly).  That's the bottleneck - knowing the surface's contentRect.   We don't know that contentRect until we've bubbled up the recursion and we know what the bounds of the entire subtree drawable area is.  If the layer we are looking at is several layers below in the hierarchy, we would have long since left the recursion of that layer before we know how to compute the surface's contentRect.

So, either we complexify the recursion drastically by having an additional recursion once we know the surface's contentRect, or we do it as a separate walk.  I'm totally against complexifying this recursion, because it won't necessarily be more efficient or readable, but it will definitely be more error-prone than the post-walk.

Enne, what do you think?  Are you still thinking there was a clean way we could compute the visibleLayerRect within the recursion, without changing the semantics of the visibleLayerRect?

[Dana Jansens]

*confused*. Surfaces don't have a visibleLayerRect.

[Shawn Singh]

(In reply to comment #49)
> *confused*. Surfaces don't have a visibleLayerRect.

I'm referring to computing the visibleLayerRect for layers;   computing this requires knowing the layer's target surface's contentRect.   that contentRect is not known until we've bubbled up the recursion to the surface, by that time, the layer is long gone from the recursion. 

Or did I completely miss something ? =)

[Dana Jansens]

Oh okay, I see!

[Eric Penner]

(In reply to comment #50)
> (In reply to comment #49)
> > *confused*. Surfaces don't have a visibleLayerRect.
> 
> I'm referring to computing the visibleLayerRect for layers;   computing this requires knowing the layer's target surface's contentRect.   that contentRect is not known until we've bubbled up the recursion to the surface, by that time, the layer is long gone from the recursion. 
> 
> Or did I completely miss something ? =)

Taking a step back (and excuse my ignorance on this! =) ) It seems like a visible rect should be the intersection of all things that reduced visibility in the tree, and that and all those things should be parents in the tree?

I get that we are in the midst of trying to figure out the render surface's size etc. while we traverse it's children, but would the visibility of surface's childA be effected in any way by it's sibling childB, when all is said and done?  If so I agree that a pass is needed, but I think I'd need someone to draw a picture for me to understand it! ;)

[Shawn Singh]

Comment on attachment 158730 [details]
Patch


I have to say, after looking at this new patch, I realize how impressive it is that you have a working solution to your tricky complicated pre-painting problem =)


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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:229
> +    void setVisibleContentProxyRect(const IntRect& rect, int distance = -1) { m_visibleContentProxyRect = rect; m_visibleContentProxyDistance = distance;}
> +    const IntRect& visibleContentProxyRect() const { return m_visibleContentProxyRect; }
> +    const int visibleContentProxyDistance() const { return m_visibleContentProxyDistance; }
> +    IntRect visibleContentRect() const { return m_visibleContentProxyRect.isEmpty() ? IntRect() : m_visibleContentProxyRect; }

I'm guessing the idea was to make it clearer that position may be important even when size is empty, right?

I do think it helps, but maybe this change isn't 100% worth the new code it requires... I'm still worried it doesn't address any of the brainstormed risks of what-may-go-wrong in comment #44.   In fact, we did have a recent regression of exactly case (3) from that comment, which makes me feel that the risks are very real.  It's the internal modifications of calcDrawTransforms that are the most risky, and I'm not sure that making the layer interface different will help.

Another example of what could go wrong, by the way - (4) if we further modify the code and use intersect() instead of the new clampRectToBounds().

I know it sounds like i'm trying to block this patch, but I don't mean to do that.  If we need to move forward with it, I'm OK with it, and we can instead create a bug that says we need to redesign this to make it less error-prone.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:145
> +static void transformVisibleContentRectForLayer(LayerType* layer)

I'm not quite understanding why this new name is more appropriate?    the "ForLayer" suffix seems nice, but the "transform" part is what I don't understand.  Seems to me the clamping/intersecting is more important than the transforming, the transforming is just done as a detail so that rects in different spaces can be converted to the same space for correct intersection/clamping, right?

[Shawn Singh]

> "I do think it helps, but ... I'm not sure that making the layer interface different will help."

hah.  excuse my contradiction =)

[Adrienne Walker]

(In reply to comment #50)
> (In reply to comment #49)
> > *confused*. Surfaces don't have a visibleLayerRect.
> 
> I'm referring to computing the visibleLayerRect for layers;   computing this requires knowing the layer's target surface's contentRect.   that contentRect is not known until we've bubbled up the recursion to the surface, by that time, the layer is long gone from the recursion. 

I feel fairly confident visibleRect can be done in the same pass.

The contentRect *is* known in the cases where you need to clip with it.  If the contentRect is not known, this is because the surface doesn't clip its subtree.

[Shawn Singh]

(In reply to comment #55)
> (In reply to comment #50)
> > (In reply to comment #49)
> > > *confused*. Surfaces don't have a visibleLayerRect.
> > 
> > I'm referring to computing the visibleLayerRect for layers;   computing this requires knowing the layer's target surface's contentRect.   that contentRect is not known until we've bubbled up the recursion to the surface, by that time, the layer is long gone from the recursion. 
> 
> I feel fairly confident visibleRect can be done in the same pass.
> 
> The contentRect *is* known in the cases where you need to clip with it.  If the contentRect is not known, this is because the surface doesn't clip its subtree.

OK, I see what you and Eric are saying...

Doing this requires projecting the clipRect from ancestor surface space to target surface space.  That clipRect then either becomes (1) a generalized polygon due to inverse-projection, or (2) an inaccurate clipRect, which means we can't directly use it for propagating clipping.

This is why we're not propagating the clipRect past renderSurfaces right now.  I guess, though, we are implicitly doing approach (2) in calculateVisibleLayerRect already, so this could work.

Would this refactor make this patch much easier?  Should I try to do this refactor sooner than later?

[Eric Penner]

Comment on attachment 158730 [details]
Patch

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

Taking a big step back since there are still concerns here, it's possible we could get an approximation to visible distance by projecting the entire viewport into the layer using the inverse screen-space transform. This would result in wasted memory (for example a big document in an overflow-scroll that is nowhere near being visible), but maybe we could live with it.   Computing that rect could be done along with the visible rect I think (can anyone think of any problems with that?)... Then pre-painting would just do visible, followed by this 'viewport visible', and expand from there.  Just brainstorming... That would also work for all layers including surface content assuming we stop culling non-visible surfaces from the tree.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:229
>> +    IntRect visibleContentRect() const { return m_visibleContentProxyRect.isEmpty() ? IntRect() : m_visibleContentProxyRect; }
> 
> I'm guessing the idea was to make it clearer that position may be important even when size is empty, right?
> 
> I do think it helps, but maybe this change isn't 100% worth the new code it requires... I'm still worried it doesn't address any of the brainstormed risks of what-may-go-wrong in comment #44.   In fact, we did have a recent regression of exactly case (3) from that comment, which makes me feel that the risks are very real.  It's the internal modifications of calcDrawTransforms that are the most risky, and I'm not sure that making the layer interface different will help.
> 
> Another example of what could go wrong, by the way - (4) if we further modify the code and use intersect() instead of the new clampRectToBounds().
> 
> I know it sounds like i'm trying to block this patch, but I don't mean to do that.  If we need to move forward with it, I'm OK with it, and we can instead create a bug that says we need to redesign this to make it less error-prone.

Yes the idea was to contain the extra rect constraints (needing to still transform empty rects and not intersect() them etc.) in one or two places.

Other than creating another tree recursion to isolate this calculation, is there anything else I could do to address those concerns? I admit that this doesn't fully remove the risk of such bugs, but the solution of another recursion also seems problematic since it would need to do lots of the same calculations, which could then get out of sync with each other and cause similar bugs.

In any case I'll investigate what it would take, since the last remaining bit here is calculating this for render surface content also.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:145
>> +static void transformVisibleContentRectForLayer(LayerType* layer)
> 
> I'm not quite understanding why this new name is more appropriate?    the "ForLayer" suffix seems nice, but the "transform" part is what I don't understand.  Seems to me the clamping/intersecting is more important than the transforming, the transforming is just done as a detail so that rects in different spaces can be converted to the same space for correct intersection/clamping, right?

I added it since there is now an intermediate value in the visibleContentRect() that get's transformed, but maybe the name should stay as calculate.

And now that I think of it, maybe it's worth biting the bullet and storing a totally separate intermediate rect rather than 're-using' the visibleContentRect. Since some layers don't end up in the iterator, those layers get bogus values since this calculation wasn't finished... Although maybe that's fine since those layers are culled from all further calculations anyway.

[Eric Penner]

(In reply to comment #56)
> (In reply to comment #55)
> > (In reply to comment #50)
> > > (In reply to comment #49)
> > > > *confused*. Surfaces don't have a visibleLayerRect.
> > > 
> > > I'm referring to computing the visibleLayerRect for layers;   computing this requires knowing the layer's target surface's contentRect.   that contentRect is not known until we've bubbled up the recursion to the surface, by that time, the layer is long gone from the recursion. 
> > 
> > I feel fairly confident visibleRect can be done in the same pass.
> > 
> > The contentRect *is* known in the cases where you need to clip with it.  If the contentRect is not known, this is because the surface doesn't clip its subtree.
> 
> OK, I see what you and Eric are saying...
> 
> Doing this requires projecting the clipRect from ancestor surface space to target surface space.  That clipRect then either becomes (1) a generalized polygon due to inverse-projection, or (2) an inaccurate clipRect, which means we can't directly use it for propagating clipping.
> 
> This is why we're not propagating the clipRect past renderSurfaces right now.  I guess, though, we are implicitly doing approach (2) in calculateVisibleLayerRect already, so this could work.
> 
> Would this refactor make this patch much easier?  Should I try to do this refactor sooner than later?

This would solve more than one problem for this patch, since then we could also calculate distances for render surface content! :) Which this patch still doesn't do, and that would 'complete' the goal for the PrioritizedTextureManager which is to prioritize *all* textures, so they never flicker when they first appear.

Given my new understanding, I think we should do any of these:
a.) proceed with the visible optimization, followed by this patch.
b.) Land this and improve with the visible optimization.
c.) Use the 'viewport visible' solution as an interim for pre-painting such that all textures can be prioritized by at least some known distance.

[Adrienne Walker]

Comment on attachment 158730 [details]
Patch

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

It looks like your math is just accumulating Manhattan distance, yeah? I have no idea how that is supposed to be correct down a layer hierarchy across transforms.  There aren't any unit tests for layers with scales, or contentBounds() != bounds(), or deviceScale != 1, or weird projections, and that lack makes me more skeptical.

I agree with all of Shawn's concerns that an empty IntRect with a valid position and an empty size with potentially only one non-zero component is a serious foot gun.  I have no idea whether WebRect or SkRect would continue to function as IntRect does if we switch to them. 

After looking at this patch in more detail, I don't think that this approach (that you changed to in comment #24) is more elegant or simple.  I continue to prefer the previous approach of using an unclipped "visible" content rect.  I totally agree that case #3 in verifyVisibleRectOutsideContentBounds that you point out in comment #26 is not solved by this approach, but I am ok with that.  I would rather have an elegant 90% solution than a brittle 100% one.  I can't think of a use case with scrolling or animation that would use multiple clip rects like this.  Also, as I see it, layer #3 is *infinite* distance away to visible--there is no distance that it can move such that it will be visible.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:145
>>> +static void transformVisibleContentRectForLayer(LayerType* layer)
>> 
>> I'm not quite understanding why this new name is more appropriate?    the "ForLayer" suffix seems nice, but the "transform" part is what I don't understand.  Seems to me the clamping/intersecting is more important than the transforming, the transforming is just done as a detail so that rects in different spaces can be converted to the same space for correct intersection/clamping, right?
> 
> I added it since there is now an intermediate value in the visibleContentRect() that get's transformed, but maybe the name should stay as calculate.
> 
> And now that I think of it, maybe it's worth biting the bullet and storing a totally separate intermediate rect rather than 're-using' the visibleContentRect. Since some layers don't end up in the iterator, those layers get bogus values since this calculation wasn't finished... Although maybe that's fine since those layers are culled from all further calculations anyway.

By "don't end up in the iterator", you're talking about layers (or subtrees) that don't end up in a render surface layer list due to layerShouldBeSkipped? I think that's probably ok.  The only case in that function that affects this distance calculation is backfacing layers and if they're animating, we consider them visible already.  Those layers won't be considered for painting or texture prioritization at all and so all their textures (if they have any) will get reset to the lowest priority, which seems like the right thing to do anyway.  This is a long-winded way to say that I agree with you.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:732
> +    layer->setVisibleContentProxyRect(visibleContentProxyRectOfLayer, accumulatedDistanceToVisible);

visibleContentProxyRectOfLayer is in the layer's content space.  Here, you are setting it to rectInTargetSpace, which is in target space.  What's going on with that?

[Eric Penner]

Comment on attachment 158730 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:732
>> +    layer->setVisibleContentProxyRect(visibleContentProxyRectOfLayer, accumulatedDistanceToVisible);
> 
> visibleContentProxyRectOfLayer is in the layer's content space.  Here, you are setting it to rectInTargetSpace, which is in target space.  What's going on with that?

Previously I was just using drawableContentRect, but doing that requires that it also needs clampRectToBounds etc. This doesn't add assumptions to drawableContentRect, but calculates setVisibleContentProxyRect in two stages, one here and one in the second pass.

I agree it's not clear so I was suggesting to use a separate variable or doing the full calculation in this recursion.

[Shawn Singh]

(In reply to comment #60)
> (From update of attachment 158730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158730&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:732
> >> +    layer->setVisibleContentProxyRect(visibleContentProxyRectOfLayer, accumulatedDistanceToVisible);
> > 
> > visibleContentProxyRectOfLayer is in the layer's content space.  Here, you are setting it to rectInTargetSpace, which is in target space.  What's going on with that?
> 
> Previously I was just using drawableContentRect, but doing that requires that it also needs clampRectToBounds etc. This doesn't add assumptions to drawableContentRect, but calculates setVisibleContentProxyRect in two stages, one here and one in the second pass.
> 
> I agree it's not clear so I was suggesting to use a separate variable or doing the full calculation in this recursion.

Just throwing a wild thought out there ... would it be useful to decompose drawableContentRect into drawableContentOrigin and drawableContentSize, reconstructing it only where necessary, and putting a comment in the appropriate place explaining that origin may be meaningful even when size is empty, so we avoid treating them as a rect?  Then your clamp helper function could receive a point and a size instead of a rect?

[Eric Penner]

(In reply to comment #59)
> (From update of attachment 158730 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158730&action=review
> 
> It looks like your math is just accumulating Manhattan distance, yeah? I have no idea how that is supposed to be correct down a layer hierarchy across transforms.  There aren't any unit tests for layers with scales, or contentBounds() != bounds(), or deviceScale != 1, or weird projections, and that lack makes me more skeptical.
> 

Fair enough if you are skeptical, but minimally I'd like to convince you that this is a valid solution. It accumulates Manhattan distance when a disjoint clip is applied and keeps a proxy, where otherwise this results in an empty rect at zero, from which we can no longer calculate distances. Transformations aren't an issue if we accept the currently used approximation of a bounding-box of the visible rect any time it is transformed.

> I agree with all of Shawn's concerns that an empty IntRect with a valid position and an empty size with potentially only one non-zero component is a serious foot gun.  I have no idea whether WebRect or SkRect would continue to function as IntRect does if we switch to them. 
> 

It seems like this is the main recurring issue here. I'm sure it would be fine with those other rect types, but it would always require slightly special treatment such that the empty proxy rects aren't reset to empty-at-zero rects.

> After looking at this patch in more detail, I don't think that this approach (that you changed to in comment #24) is more elegant or simple.  I continue to prefer the previous approach of using an unclipped "visible" content rect.  I totally agree that case #3 in verifyVisibleRectOutsideContentBounds that you point out in comment #26 is not solved by this approach, but I am ok with that.  I would rather have an elegant 90% solution than a brittle 100% one.  I can't think of a use case with scrolling or animation that would use multiple clip rects like this.  Also, as I see it, layer #3 is *infinite* distance away to visible--there is no distance that it can move such that it will be visible.
> 

I don't think this is inherently brittle and I'll add more tests if we keep this approach. However using a looser bound is fine, that I'll discuss below. I'm confused how you see layer #3 being at an infinite distance. Layer #3 can move right to be visible in layer #2, and then layer #2 can move to the left to make #3 visible.

If you think multiple clips like this can't happen, mobile Gmail provides a nice use case that has two disjoint clips. There is the root clip plus the overflow-scroll clip that starts off completely off-screen (the entire scrollable email is animated in from the right).

Another example is a 'big document in a document'. Without this, the only meaningfulness rect we have left to measure distance from is the viewport. If there are multiple 'documents inside a document', they will pile up in viewport space and need to be allocated and painted before any real pre-painting can happen. 


> By "don't end up in the iterator", you're talking about layers (or subtrees) that don't end up in a render surface layer list due to layerShouldBeSkipped? I think that's probably ok.  The only case in that function that affects this distance calculation is backfacing layers and if they're animating, we consider them visible already.  Those layers won't be considered for painting or texture prioritization at all and so all their textures (if they have any) will get reset to the lowest priority, which seems like the right thing to do anyway.  This is a long-winded way to say that I agree with you.

This is some additional culling of the render surface layer list in addition to layerShouldBeSkipped. I believe it was due to the code below, but possibly another piece I'm not finding now. This is reproducible in the unit test by making layer 2 mask to bounds and non-opaque.

CCLayerTreeHostCommon.cpp:
        if (clippedContentRect.isEmpty())
            renderSurface->clearLayerList();

So, the other option I'm aware of is to transform the viewport into layer space. I think I can poke holes in any tighter approximation than that. I'm totally fine with that myself and it would work for mobile gmail which is the worst case example I'm aware of.

[Eric Penner]

> So, the other option I'm aware of is to transform the viewport into layer space. I think I can poke holes in any tighter approximation than that. I'm totally fine with that myself and it would work for mobile gmail which is the worst case example I'm aware of.

Okay one last brainstorm for a more simple solution:

I jumped to a general solution to multiple clips because I thought it was elegant, but using just the first and last clip would get us 99% of the way there (this would effectively be the 'viewport visible rect' and the 'unclipped visible rect' both of which are not clipped by *anything*).

That would require two extra rects in the layer, unless we made visibleContentRect return the intersection of those two, which would be an approximation of the current visibleContentRect.

[Eric Penner]

(In reply to comment #63)
> > So, the other option I'm aware of is to transform the viewport into layer space. I think I can poke holes in any tighter approximation than that. I'm totally fine with that myself and it would work for mobile gmail which is the worst case example I'm aware of.
> 
> Okay one last brainstorm for a more simple solution:
> 
> I jumped to a general solution to multiple clips because I thought it was elegant, but using just the first and last clip would get us 99% of the way there (this would effectively be the 'viewport visible rect' and the 'unclipped visible rect' both of which are not clipped by *anything*).
> 
> That would require two extra rects in the layer, unless we made visibleContentRect return the intersection of those two, which would be an approximation of the current visibleContentRect.

Quick follow up, when using two rects as I described above, the 'visible distance' would be the maximum distance to either of the two rects.

Of the options I've mentioned, let me know which you like the best, or if you have an alternative I'm not seeing. I can think of these three options now:
a.) Accumulated distance with 'empty rects'.
b.) Store unclipped viewport in layer space and accept any issues.
c.) Store viewport + last clip in layer space (both unclipped).

[Eric Penner]

> Just throwing a wild thought out there ... would it be useful to decompose drawableContentRect into drawableContentOrigin and drawableContentSize, reconstructing it only where necessary, and putting a comment in the appropriate place explaining that origin may be meaningful even when size is empty, so we avoid treating them as a rect?  Then your clamp helper function could receive a point and a size instead of a rect?

Sorry I missed this. I really don't like my new solution of setting an intermediate calculation in visibleContentRect. In terms of not messing with drawableContentRect, that issue will go away if we calculate the visible rect in the main recursion. As a general approach to avoid 'weird empty rects' I like it.  However the size could still be empty but contain a width or height. Not sure if that is considered equally 'weird' or if that would be acceptable.

[Dana Jansens]

(In reply to comment #65)
> However the size could still be empty but contain a width or height. Not sure if that is considered equally 'weird' or if that would be acceptable.

Just as a FWIW, sizes coming from webkit to GraphicsLayers can be empty but have a width/height in order to produce correct layout of children (we saw some bugs when we assumed .isEmpty() meant we could use 0,0 instead).

[Eric Penner]

> Just as a FWIW, sizes coming from webkit to GraphicsLayers can be empty but have a width/height in order to produce correct layout of children (we saw some bugs when we assumed .isEmpty() meant we could use 0,0 instead).

Thanks Dana! It's a good example. I'm sure there is use of empty rects out there too. An empty size isn't that different from an empty rect. IMO this empty rect argument is like having an 'zero int' argument. Why risk dividing by zero after all? Zero should not be allowed!  ;)

As an update on testing. Non axis aligned transforms can't be tested since this doesn't support render-surfaces yet (visible calculation needs to be merged first). I could add scale tests, if that would help.

[Adrienne Walker]

(In reply to comment #67)
> > Just as a FWIW, sizes coming from webkit to GraphicsLayers can be empty but have a width/height in order to produce correct layout of children (we saw some bugs when we assumed .isEmpty() meant we could use 0,0 instead).
> 
> Thanks Dana! It's a good example. I'm sure there is use of empty rects out there too. An empty size isn't that different from an empty rect. IMO this empty rect argument is like having an 'zero int' argument. Why risk dividing by zero after all? Zero should not be allowed!  ;)

Being glib is not a convincing argument here.  An empty size *is* very different from an empty rect.

As Shawn has pointed out, this is risky because a future change that collapses your empty-but-non-zero rect to an empty-but-zero one will be non-obvious when reading that code.  There are plenty of places where there is a check if a rect isEmpty() and then it returns an IntRect() because it assumes that all empty rects are equivalent.

(In reply to comment #62)
> (In reply to comment #59)
> > (From update of attachment 158730 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158730&action=review
> > 
> > It looks like your math is just accumulating Manhattan distance, yeah? I have no idea how that is supposed to be correct down a layer hierarchy across transforms.  There aren't any unit tests for layers with scales, or contentBounds() != bounds(), or deviceScale != 1, or weird projections, and that lack makes me more skeptical.
> > 
> 
> Fair enough if you are skeptical, but minimally I'd like to convince you that this is a valid solution. It accumulates Manhattan distance when a disjoint clip is applied and keeps a proxy, where otherwise this results in an empty rect at zero, from which we can no longer calculate distances. Transformations aren't an issue if we accept the currently used approximation of a bounding-box of the visible rect any time it is transformed.

You'll need to be more convincing.

What space is this distance in?  It looks like you're accumulating distances in target space (which then is stored as a "content space" proxy rect)? The question about scales also relates to this.  If some subtree has a scale applied to it, should the distances be likewise scaled in terms of how far away the children in that subtree are from some clip rect?

> If you think multiple clips like this can't happen, mobile Gmail provides a nice use case that has two disjoint clips. There is the root clip plus the overflow-scroll clip that starts off completely off-screen (the entire scrollable email is animated in from the right).

If it's animated, then don't we already special case prioritizing the entire layer?

[Adrienne Walker]

> Being glib is not a convincing argument here.  An empty size *is* very different from an empty rect.

Er, I mean "empty rect" is very different than a "zero int."  Oops.

[Eric Penner]

> Being glib is not a convincing argument here.  An empty size *is* very different from an empty rect.

It was a joke but nonetheless a valid comparison I think. I didn't mean to trivialize very valid concerns, but just argue that there is cases where unusual arguments are tolerated. A custom type could be used to be very strict instead, but not sure that would really be better.

> As Shawn has pointed out, this is risky because a future change that collapses your empty-but-non-zero rect to an empty-but-zero one will be non-obvious when reading that code.  There are plenty of places where there is a check if a rect isEmpty() and then it returns an IntRect() because it assumes that all empty rects are equivalent.

If this waits for the visible rect calculation merge, then there is only two places that need to deal with funny rects:
- Calculation in the recursion
- Use of the rect for pre-painting
All other code can continue to not know about distances and collapse the rect all they want. I don't think any other code will accidentally take over calculating visible rects and setting them. We could make the recursion a friend function and the setter protected if that is a concern.

> You'll need to be more convincing.

> It looks like you're accumulating distances in target space (which then is stored as a "content space" proxy rect)?

> What space is this distance in? 

I think the best trade-off is to always accumulate distance in pixels. So we can accumulate distance in any space as long as it is in pixel units. From my understanding that's what it is doing. Setting an intermediate target space value in the visibleContentRect is very bad and can go away. It was an attempt to not enforce the empty rect constraints on drawableContentRect, but I agree it was a very bad idea. I agree that scaling would need to be tested (and unfortunately that's all that can be tested right now until render surfaces are supported).

> If it's animated, then don't we already special case prioritizing the entire layer?

Unfortunately not. That works for jquery mobile and most all transitions I could find out there, but the scrollable emails inside gmail-mobile are arbitrarily large. To get that to work well needed both a direction assumption and some form of unclipped rect. An unclipped viewport rect would be fine if we assume that nearby animated layers are moving towards the viewport (or we can further add animation direction to the mix).

But the important case is just simple offscreen layers, like Darin was toying with here:
http://www.corp.google.com/~darin/test/viewport/test.html?burn=25

Picture two cases: that demo, and that demo running inside a small overflow-scroll div that is 100 pages away in the document. If you use the viewport visibility only then you need to paint lots of hidden content when it is in view, and if you use only the last clip (the last mask-to-bounds rect), then you would think it's close to being visible when it's actually 100 pages away. Intersecting the two rects makes them useless, and intersecting conditionally would result in flip-flopping distance estimates and paint thrashing. Storing both would be good, but you will need to store three rects rather than one.

Overall the viewport is probably fine, especially on mobile where over-flow scrolls are close to the viewport's size anyway.

[Dana Jansens]

(In reply to comment #70)
> I agree that scaling would need to be tested (and unfortunately that's all that can be tested right now until render surfaces are supported).

I'm not sure what you mean when you say supporting render surfaces here. They are not like layers, they won't exist at all if they are clipped out, and are by definition basically entirely visible, because clipped/non-visible stuff just isn't part of a render surface. It's size is defined by what is visible in its subtree. Are you meaning to change that or is there something else going on here or?

[Eric Penner]

(In reply to comment #71)
> (In reply to comment #70)
> > I agree that scaling would need to be tested (and unfortunately that's all that can be tested right now until render surfaces are supported).
> 
> I'm not sure what you mean when you say supporting render surfaces here. They are not like layers, they won't exist at all if they are clipped out, and are by definition basically entirely visible, because clipped/non-visible stuff just isn't part of a render surface. It's size is defined by what is visible in its subtree. Are you meaning to change that or is there something else going on here or?

Sorry, I meant render surface content. Painted content that contributes to render surfaces. Currently they are culled entirely from the render surface layer list in the unit test when layer 2 is not visible. And if they weren't culled we still couldn't calculate the distance without propagating visibility down the tree (which Shawn has also indicated is required for calculating visible rects within the main recursion)

[Dana Jansens]

I see, thanks for the explanation. So it's keeping layers in the RSLL that aren't actually going to draw basically.

[Eric Penner]

(In reply to comment #73)
> I see, thanks for the explanation. So it's keeping layers in the RSLL that aren't actually going to draw basically.

You might have meant this, but to be sure: it's culling layers in the RSLL that aren't going to draw, yes. But those layers should still be given a chance to paint.

[Eric Penner]

To clarify a bit more why I don't think transformations are a big issue, here's an example: We want do '1024 pixels' of pre-painting, on two layers that are 512 pixels away from view in target surface space, but scaled differently. In that case each layer's distance would be 512 pixels, so they would each do 512 pixels of pre-painting starting at the point closest to being visible. That means the amount pre-painted amount in target surface space is different!

However, I think it's completely okay to do that and not bias all our memory into scaled down layers (this is already how our current pre-painting ends up working too). In addition, perspective would non-uniformly scale layers making it a much more difficult problem to solve. I'm not sure if that is one of the transformation concerns, but that's my take on that.

[Stephen Chenney]

https://code.google.com/p/chromium/issues/detail?id=231303