-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Should non-root elements be clipped when capturing them? #73
Comments
This could be a good performance optimization but also something we can punt to v2. |
Presumably we have to clip to max texture size for old content, right? I think figuring out how and when to clip is within scope for v1 |
The fact that we have to clip to max texture size is an implementation short-coming that could be fixed. We could cache the content in a tiled image instead of a single texture, right? That's why I was seeing this as a perf optimization. |
I think elements exceeding max texture size may be pretty frequent on the web (don't quote me on that). So, is the suggestion to implement a tiled image solution then? Basically, my concern is that v1 needs to deal with elements that are larger than max texture size, but whether it is enforced clipping, or tiled textures, I don't have a strong opinion |
It's not going to be feasible to do tiled image in v1 for sure. So the fallback will have to be that we clip to max texture size. If that implies that we're not spec compliant (since we are implicitly clipping the image) then yes, we have to deal with this for v1. My proposal would be to clip at max texture size in v1. And add a performance hint that allows clipping further later or think about tiled image if we see legit use-cases which need content larger than max texture size. What's your preferred option? |
My preferred option is that we clip non-root elements to the viewport in the same way we clip root elements to the viewport. We should also have an ability to add a padding to this clip, for both root and non-root content, in v1. I don't see a need to have different behaviors for root and non-root elements here. I think the only reason we have them is that we know that roots will be large and we will have to clip them to max texture size. All I'm proposing is that non-roots can be equally large almost as often. I think that would make for a good API that also naturally precludes elements flying from/to "really far away" which may be outside of the painted region. If the stepping stone to get there is always clip root, never clip non-root, then that's fine. The implementation for clipping non-root elements has to exist in v1 anyway, since clipping to max texture size needs to be done at some offset from element's origin, so I propose we clip centered around the viewport. |
Ok. So what you're proposing is that an element's snapshot bounds is its visible rect in the viewport. I'm assuming that includes the ink overflow from self effects and will also include padding from overflow-clip-margin. What happens if there is a transform (like rotation or skew on it) so its visible rect is not axis aligned with the viewport? |
Btw, this is also related to the sizing of boxes where this content goes (using a replaced element). But let's figure out the desired paint bounds for a shared element first. I filed #99 to discuss box sizing after this. |
It's a good question about what to do if there's a transform. What do we do if there's a transform on body like this In the case of transform, the general idea is that we want to capture the content clipped in such a way that it has the minimal clip which still paints all of the viewport visible contents. I don't know how to describe this operation, but I think given the transform it should be possible to find such a rect. As for what it includes, it includes the same things it would've included otherwise. It's just clipped afterwards. Is the visible rect in Chromium specified relative to the layer itself or to its transform parent? |
body is a content of the root indeed. We don't have this issue for the root snapshot if it's going to be the output of the root stacking context since there can't be any transform on it. I agree with the motivation for the general idea (to spend less memory on snapshots) but trying to think through how to implement/define this makes it sound hard. The problem is that an element's visible quad (let's call it quad because of the non axis aligned transforms) is dependent on multiple properties that come from its hierarchy. There are transforms and clips coming from the ancestor chain (gonna exclude effects like clip-path since that's even more complicated). So the inputs we need for sure are the element's paint rect (which is expanded from the overflow outside its bounding box) and the viewport rect. And the output we want is a clip rect in the paint rect space (or content space in chromium terminology)? What other inputs do we need to define this? "Is the visible rect in Chromium specified relative to the layer itself or to its transform parent?" : Which visible rect? Can you link to the code? |
Actually, there maybe a way to lean on Element.getBoundingClientRect() to define this. Though I don't think that rect includes overflow. |
For the Chromium part, I think that we paint +/- some amount from the viewport for each of the composited layers. So we have an ability to detect the viewport rects in layers post transform (otherwise we'd have to paint the whole thing under any non-translate transform, right?). getBoundingClientRect is definitely a part of this solution, we should know the visible quads that comprise the element, so the union of those into an axis aligned rect should give us the answer here? I don't mean to get hung up on details, since that's always hard to do in an issue like this one. But what's the alternative? We just let the max texture size clip happen somewhere? That is obviously worse to me, so we need to come up with some sort of an answer here. |
The reason I'm prodding on the details here is that while doing a clip in viewport space would be ideal, it's harder to define. I'm happy to consider it if we can sort out the details and maybe they are not as complicated as I think. Would appreciate your help in narrowing them down. The alternate proposal is to size the image based on element() with the exception that the size is expanded to include ink overflow from self effects. Then this image is clipped to max texture size. The case this could miss is if you have a massive element and the part of it in the visible viewport is what gets clipped when we clamp to max texture size. |
I'd start by looking at the interest rect code in painting and see if there are any useful insights. I'm pretty sure we have the ability to take a rect in element's local space and map it to an axis aligned rect in viewport space. So the inverse, taking a viewport rect and mapping it down to element space should also be possible (by we, I mean Chromium here) |
Ok. So let's summarize the 2 options with as much detail as we can : Option 1 Option 2 |
For the second part of the second option, you assumed that a rect in viewport space stays a rect in element space but other than that, something similar to that yeah. I don't know if it's easier to just map the viewport bounds rect into element space and do clipping there. It depends on what space the developer will provide the padding in |
"you assumed that a rect in viewport space stays a rect in element space" : You mean when the painted rect clipped by viewport is being mapped back to element space (or decorated bounding box space)? We'll have to make this a rect somehow... That was a good question : "depends on what space the developer will provide the padding in". I'm not sure what's the right answer. Seems like viewport space is better, if the idea is for the developer to say : "this is how much I want to move this element when it animates". But when we do preserve the hierarchy, it probably makes sense to do all of this in the ancestor shared element's space instead of viewport space? I also realized that this will affect both the sizing and transform on the transition elements which render the snapshot. Right now we're saying that the transform maps the element's border-box to viewport (or ancestor) space. But now it will have to be the transform that maps this clipped captured rect to the corresponding quad...? I also want to summarize the downsides of option 1 for why this can't be punted to v2 :
Is there anything else? I'm asking so that if we have to make a call to punt it we understand what use-cases will be broken. |
@khushalsagar (sorry to break this discussion up with some meta but...) When you share this with folks on Monday and we discuss this on Tuesday, could you make sure to describe both why it should be in v1 and also the options? I think those are both crucial pieces to this convo that others will need for context. |
Hopefully layout/paint experts can correct me if this is wrong, but I think that in this case we can do the following:
In blink, at least, I think TransformationMatrix::MapRect handles some of this. (edit: grammar and s/rect/quad/) |
Hm, I think that'd create weird cases with headers that are slightly scrolled out of view. It creates a bit of a footgun as the developer might not test for that. Would it be sufficient to define two clipping modes:
Agreed. There's crossover with #85 here. Maybe we're overcomplicating things by treating the root as special. |
I like the two modes approach, it keeps the API simple and allows the developer to not use too much memory if they are careful even on large (non-root) elements. A fine-grained control of precise padding amount can then be a possible v2 feature. |
Stepping back, this seems analogous to checkerboarding and feels like a UA implementation detail. That said, I do agree that it makes sense to capture reasonably-sized elements even if they're outside the viewport. For items that are smaller than the maximum size, clearly you can capture the whole element. For items larger than the texture size, taking the-part-closest-to-the-viewport seems like a reasonable heuristic. This is more complicated than what I've suggested above (and I'm not sure it's important to dig into the details here), but seems possible and could work identically for both root and non-root elements. |
For the 2 modes approach, the max-clipped sounds similar to clamping to max texture size except we're using viewport bounds instead of max texture size. Sounds reasonable but it wouldn't address the concern where the visible portion of the element is outside this rect though. And I thought that was the functional issue with clamping to max texture size (other than the memory concerns). The viewport-clipped mode helps ensuring we always capture what's visible and allow padding (maybe a UA default but exposing fine-grained control if that becomes useful). I'm trying to understand how a developer would decide which mode to use. If the element is small enough use max-clipped. If it's big and the visible portion won't get captured with max-clipped then use viewport-clipped? |
@ianvollick are you suggesting we still have the 2 modes but instead of asking developers to choose, the UA should do it internally? If the whole element fits within maximum size (based on viewport bounds or max texture size) then capture the whole element. If not use viewport-clipped. |
If we have these two modes in v1, what is the plan to achieve just one mode in v2? If devs are choosing the mode in v1, will they have to undo anything for v2? Just looking to reduce the amount of work required overall |
I just spoke with Khushal and one point I should clarify is that the approach I'm suggesting seems like it will naturally produce the behavior of the two modes Jake described, but with one difference: if we the element is very large and we use the close-to-the-viewport heuristic, the rectangle we capture won't necessarily be positioned at the origin in element space -- rather, it will be the max-sized rectangle closest to the viewport (determining this rectangle isn't as simple as i described above, but I don't think the details are important here). I.e., we won't need two modes with this approach. |
+1 to Ian's point. I'll summarize the idea to ensure we can all be on the same page :
@vmpstr @jakearchibald @hvanops does this sound reasonable to you? |
Not to miss the point about unnecessary memory use earlier. "The element's captured painted content has to be capped to some bounds" : It makes sense for this heuristic to start with something that captures more than the developer might have intended (if we use max texture size or a padded viewport rect). And we can add the option for the developer to hint to capture less as a memory optimization later. |
Yeah, I think a heuristic approach is fine. We can always change from A couple of things that aren't clear to me: If an element is 300x300, but partially out of the viewport, can it be captured in its entirety? If an element is 20x20, but 20,000 pixels north of the viewport, can it be captured? I'm hoping we can have a ruleset like this:
We need to think about how this will impact the size of the captured elements. I guess this gets less complicated using @flackr's model of detaching the texture size from the replaced element #99, although that comes with complications of its own. |
"If an element is 300x300, but partially out of the viewport, can it be captured in its entirety?" : Absolutely. "If an element is 20x20, but 20,000 pixels north of the viewport, can it be captured?" : Yes to this too. We already have heuristics which check for how close an element is from the viewport to decide whether to paint/raster it. This case likely gets optimized out today but we can force it to paint/raster if it's a shared element. For the ruleset you mentioned, I think that's what is being proposed :
This should make sure all functional use-cases are addressed. There is still room for perf optimizations where a developer can hint that only in-viewport area should be captured. But I'm hoping we can add that later. You're absolutely right that we'll need to think through how this gets reflected in the box sizes. I actually think it gets complicated if we use shared element box size instead of texture size for pseudo elements. But we can discuss the details for it once the painted content size is finalized. |
Yep, we're on the same page here. Sounds good! |
The conclusion for this was to always capture the full element content and allow the UA to limit it to a capped size (max texture size) if needed. The region captured if the content needs to be clipped to a capped size is the area closest to the viewport. |
Related to #72 , for non-root elements: should we be able to clip them since they can be massive as well?
The text was updated successfully, but these errors were encountered: