Reland unobstructed platform views #17336
Conversation
| joined_rect.intersect(platform_view_rect); | ||
| // Subpixels in the platform may not align with the canvas subpixels. | ||
| // To workaround it, round the rect values. | ||
| joined_rect.setLTRB(std::round(joined_rect.left()), std::round(joined_rect.top()), |
There was a problem hiding this comment.
Are you sure rounding is what we want, instead of cutting off the subpixels? Could we end up with a row or column of pixels whose content is empty?
There was a problem hiding this comment.
It nows rounds the left and top bounds to the nearest int, whereas the right and bottom bounds are ceiled. This ensures that the rect is slightly larger than the original one.
|
|
||
| std::unique_ptr<SurfaceFrame> frame = | ||
| layer->surface->AcquireFrame(SkISize::Make(rect.width(), rect.height())); | ||
| auto overlay_view = layer->overlay_view.get(); |
There was a problem hiding this comment.
Doesn't matter what @chinmaygarde likes, the google style guide is clear. It's the same constitution we all adhere to including him.
There was a problem hiding this comment.
In this case in particular, overlay_view refers to a view. However, I acknowledge the style guide has left this to the author's judgment who may be biased. For example, I think someone can infer that this is a UIView because of the word view in the property name, and the fact that the properties being set are very commonly used in the context of a UIView.
I don't have strong preference. Generally speaking, I like type inference. Although, I'm happy to change it if it helps a new contributor.
There was a problem hiding this comment.
I'd personally don't feel strongly about it either, but the style guide is clear. I learned years ago, everyone is happier at Google if we just follow the guide. There is too much overhead to debating this stuff all the time or trying to figure out what the unwritten style is =)
There was a problem hiding this comment.
I heard what you are saying. My rule of thumb is that if the property name carries the meaning of it and type, there is no need to specify the type. Someone can do layer->overlay_view.get().frame = CGMakeRect(...) instead of first setting layer->overlay_view.get() as local variable with an explicit type.
There was a problem hiding this comment.
Use type deduction only if it makes the code clearer to readers who aren't familiar with the project, or if it makes the code safer. Do not use it merely to avoid the inconvenience of writing an explicit type.
I would say this doesn't make the code clearer because someone who isn't familiar with the code doesn't know the type of layer->overlay_view and it doesn't make it any safer.
if the property name carries the meaning of it and type, there is no need to specify the type
The property doesn't specify the type, it says view but that's not the same thing as a UIView.
|
Do you have the results of platform_views_scroll_perf_ios__timeline_summary before and after the change specifically? Is that what is in the description? |
|
@gaaclarke I updated the description. PTAL :) |
| joined_rect.setLTRB(std::nearbyint(joined_rect.left()), std::nearbyint(joined_rect.top()), | ||
| std::ceil(joined_rect.right()), std::ceil(joined_rect.bottom())); |
There was a problem hiding this comment.
Assuming that the origin is top left I believe this is what you want:
joined_rect.setLTRB(ceil(joined_rect.left()), ceil(joined_rect.top()),
floor(joined_rect.right()), floor(joined_rect.bottom()));
You don't want to round because it's non-deterministic. We need to make a decision, should we expand the area, or shrink it?
The code i wrote is guaranteeing that we are trimming fractional pixels to create a rect with the smallest surface area. The reason I believe we want that is that way we can guarantee that we have content that will be contained by it. One way rounding could be bad is if a mask and a view go to the right edge of 10.5, if the masks right edge is rounded to 11 it is exceeding the bounds of his view which may be benign or erroneous depending on the implementation of the mask.
You have a better understanding of the code, i'm not sure if you want to shrink the surface area, but I'm pretty sure you don't want rounding, you want ceil or floor.
There was a problem hiding this comment.
The code i wrote is guaranteeing that we are trimming fractional pixels to create a rect with the smallest surface area.
The rect should cover a bigger surface, not a smaller one. Otherwise, a row or column could end up missing pixels.
There was a problem hiding this comment.
Ok, that sounds good to me. Just remove the rounding to guarantee the surface area gets bigger then, right?
There was a problem hiding this comment.
(std::floor(joined_rect.left()), std::floor(joined_rect.top()), std::ceil(joined_rect.right()), std::ceil(joined_rect.bottom())) should produce a bigger surface area. Is there any concern with floor for top, left and ceil for right, bottom?
3 participants
Relands #17049 with a perf improvement.
Commit 20d0b16 is the workaround for the perf regression filed in flutter/flutter#53266, which consists of adding a wrapper view for each overlay view, such that the wrapper clips the overlay. This ensures that overlay surfaces don't change size between frames.
Benchmark without unobstructed platform views:
{ "average_frame_build_time_millis": 3.1827624999999986, "90th_percentile_frame_build_time_millis": 3.999, "99th_percentile_frame_build_time_millis": 12.668, "worst_frame_build_time_millis": 15.697, "missed_frame_build_budget_count": 0, "average_frame_rasterizer_time_millis": 4.016325, "90th_percentile_frame_rasterizer_time_millis": 5.33, "99th_percentile_frame_rasterizer_time_millis": 6.244, "worst_frame_rasterizer_time_millis": 6.296, "missed_frame_rasterizer_budget_count": 0 }Bechmark with unobstructed platform views (without perf optimization):
Issue: flutter/flutter#53266
{ "average_frame_build_time_millis": 3.888986666666665, "90th_percentile_frame_build_time_millis": 6.07, "99th_percentile_frame_build_time_millis": 11.615, "worst_frame_build_time_millis": 11.685, "missed_frame_build_budget_count": 0, "average_frame_rasterizer_time_millis": 14.192533333333328, "90th_percentile_frame_rasterizer_time_millis": 19.397, "99th_percentile_frame_rasterizer_time_millis": 53.07, "worst_frame_rasterizer_time_millis": 54.267, "missed_frame_rasterizer_budget_count": 23, "frame_count": 75 }Benchmark with unobstructed platform views (with perf optimization in 20d0b16):
{ "average_frame_build_time_millis": 1.5274395604395605, "90th_percentile_frame_build_time_millis": 1.601, "99th_percentile_frame_build_time_millis": 7.545, "worst_frame_build_time_millis": 8.644, "missed_frame_build_budget_count": 0, "average_frame_rasterizer_time_millis": 8.354488888888888, "90th_percentile_frame_rasterizer_time_millis": 9.132, "99th_percentile_frame_rasterizer_time_millis": 11.409, "worst_frame_rasterizer_time_millis": 12.047, "missed_frame_rasterizer_budget_count": 0 }What it's interesting is that the frame build time got better, but the frame rasterizer got worse. I confirmed this is better than the regression seen in flutter/flutter#53266.