Disable auto_resize_output_surface for LFF devices on Android [chromium/src : main]
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
Related details
- Alex Moshchuk
- Ben Chin
- Bo Liu
- Kartar Singh
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
- Alex Moshchuk
- Ben Chin
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 1 comment![]( "Open in Gerrit")
Bo Liu+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
oh oops that was from the wrong account..
So viz normally will not draw until viz::Display size and root CompositorFrame size [matches](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/display.cc;drc=c603d4c93deb40de18f249a9a59bf5bcb3aacc66;l=1064).
If browser will keep changing display size and submit new CF, that activate a bit later because it waits for the webcontents, we will never draw a frame, as CF will always be a few frames behind.
Is it fine?
The history here is that we added this behaviour for windows originally [in 2014](https://codereview.chromium.org/767443002). The idea was to avoid stretching (otherwise content of the window was stretching without aspect during resize). Then in [2015](https://codereview.chromium.org/1426453006) we realized that what I mentioned above is possible and we don't draw during resize, because we get behind, so instead we introduced this "auto resize" so we draw gutter instead.
Later we turned it off for android [here](https://chromium-review.googlesource.com/c/chromium/src/+/1031129), because there was no resize really and for rotation we wanted to block showing old content anyway, but [it was never ported to viz](https://chromium-review.googlesource.com/c/chromium/src/+/1055626), so it ended up enabled for years.
Ideally we don't want to mess with the compositor frame in viz display and handle gutter somewhere else, but this is out of the scope here.
Related details
- Alex Moshchuk
- Ben Chin
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 1 comment![]( "Open in Gerrit")
Moving myself to CC, since it seems like boliu@ and vasilyt@ are already reviewing this and have more context than I do. (Let me know if you have specific questions for me, though.)
Related details
- Ben Chin
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Chin (Gerrit)
Ben Chin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ben Chin
| Commit-Queue | +1 |
1 comment
Bo Liu+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
Vasiliy Telezhnikovoh oops that was from the wrong account..
So viz normally will not draw until viz::Display size and root CompositorFrame size [matches](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/display.cc;drc=c603d4c93deb40de18f249a9a59bf5bcb3aacc66;l=1064).
If browser will keep changing display size and submit new CF, that activate a bit later because it waits for the webcontents, we will never draw a frame, as CF will always be a few frames behind.
Is it fine?
The history here is that we added this behaviour for windows originally [in 2014](https://codereview.chromium.org/767443002). The idea was to avoid stretching (otherwise content of the window was stretching without aspect during resize). Then in [2015](https://codereview.chromium.org/1426453006) we realized that what I mentioned above is possible and we don't draw during resize, because we get behind, so instead we introduced this "auto resize" so we draw gutter instead.
Later we turned it off for android [here](https://chromium-review.googlesource.com/c/chromium/src/+/1031129), because there was no resize really and for rotation we wanted to block showing old content anyway, but [it was never ported to viz](https://chromium-review.googlesource.com/c/chromium/src/+/1055626), so it ended up enabled for years.
Ideally we don't want to mess with the compositor frame in viz display and handle gutter somewhere else, but this is out of the scope here.
Thanks for the historical context! That's very helpful.
Regarding the concern that disabling `auto_resize_output_surface` might cause us to "never draw" because the browser is always behind: We have simultaneously optimized the browser-side resize latency (reducing the Java UI thread resize handling from ~25ms to <3ms via pre-measuring layout)
If we keep auto_resize = true, Viz stretches the old content to fill the new window size while waiting for the new frame. This stretching is the visual artifact [gutter](http://screencast/cast/NDk4NjA4NzY4NzU4NTc5MnxjMWFjYmI2OC1kNg) we are trying to fix. By disabling it and enforcing synchronization, we ensure blue butter is [gone](http://screencast/cast/NTM1MjA5OTAyOTE4ODYwOHw0MDY3NDFlNi0zMA) during resizing.
Related details
- Bo Liu
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
1 comment
Bo Liu+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
Vasiliy Telezhnikovoh oops that was from the wrong account..
Ben ChinSo viz normally will not draw until viz::Display size and root CompositorFrame size [matches](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/display.cc;drc=c603d4c93deb40de18f249a9a59bf5bcb3aacc66;l=1064).
If browser will keep changing display size and submit new CF, that activate a bit later because it waits for the webcontents, we will never draw a frame, as CF will always be a few frames behind.
Is it fine?
The history here is that we added this behaviour for windows originally [in 2014](https://codereview.chromium.org/767443002). The idea was to avoid stretching (otherwise content of the window was stretching without aspect during resize). Then in [2015](https://codereview.chromium.org/1426453006) we realized that what I mentioned above is possible and we don't draw during resize, because we get behind, so instead we introduced this "auto resize" so we draw gutter instead.
Later we turned it off for android [here](https://chromium-review.googlesource.com/c/chromium/src/+/1031129), because there was no resize really and for rotation we wanted to block showing old content anyway, but [it was never ported to viz](https://chromium-review.googlesource.com/c/chromium/src/+/1055626), so it ended up enabled for years.
Ideally we don't want to mess with the compositor frame in viz display and handle gutter somewhere else, but this is out of the scope here.
Thanks for the historical context! That's very helpful.
Regarding the concern that disabling `auto_resize_output_surface` might cause us to "never draw" because the browser is always behind: We have simultaneously optimized the browser-side resize latency (reducing the Java UI thread resize handling from ~25ms to <3ms via pre-measuring layout)
If we keep auto_resize = true, Viz stretches the old content to fill the new window size while waiting for the new frame. This stretching is the visual artifact [gutter](http://screencast/cast/NDk4NjA4NzY4NzU4NTc5MnxjMWFjYmI2OC1kNg) we are trying to fix. By disabling it and enforcing synchronization, we ensure blue butter is [gone](http://screencast/cast/NTM1MjA5OTAyOTE4ODYwOHw0MDY3NDFlNi0zMA) during resizing.
In your video without blue gutter the resize flow is visually much less responsive, though.
Because we need to wait for CompositorFrame both from browser and renderer, we still can be behind. The blue gutter exists only in dcheck-enabled builds, though some other visual artifacts could be present in release builds too.
That being said, I don't have conceptual objections to this CL, I'm just thinking if there is better solution for this problem.
Related details
- Ben Chin
- Bo Liu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
Bo Liu+vasilyt
Context, this is for window drag resize on desktop-like UI. Apparently right now viz will just throw up the old frame of the wrong size and gutter, which looks bad.
What I'm interested is why do we even want this for phones. Sounds like a bad behavior to gutter in that case too? Dunno if there's history here why it was added. If no one remembers, might worth doing some code archaeology
Vasiliy Telezhnikovoh oops that was from the wrong account..
Ben ChinSo viz normally will not draw until viz::Display size and root CompositorFrame size [matches](https://source.chromium.org/chromium/chromium/src/+/main:components/viz/service/display/display.cc;drc=c603d4c93deb40de18f249a9a59bf5bcb3aacc66;l=1064).
If browser will keep changing display size and submit new CF, that activate a bit later because it waits for the webcontents, we will never draw a frame, as CF will always be a few frames behind.
Is it fine?
The history here is that we added this behaviour for windows originally [in 2014](https://codereview.chromium.org/767443002). The idea was to avoid stretching (otherwise content of the window was stretching without aspect during resize). Then in [2015](https://codereview.chromium.org/1426453006) we realized that what I mentioned above is possible and we don't draw during resize, because we get behind, so instead we introduced this "auto resize" so we draw gutter instead.
Later we turned it off for android [here](https://chromium-review.googlesource.com/c/chromium/src/+/1031129), because there was no resize really and for rotation we wanted to block showing old content anyway, but [it was never ported to viz](https://chromium-review.googlesource.com/c/chromium/src/+/1055626), so it ended up enabled for years.
Ideally we don't want to mess with the compositor frame in viz display and handle gutter somewhere else, but this is out of the scope here.
Vasiliy TelezhnikovThanks for the historical context! That's very helpful.
Regarding the concern that disabling `auto_resize_output_surface` might cause us to "never draw" because the browser is always behind: We have simultaneously optimized the browser-side resize latency (reducing the Java UI thread resize handling from ~25ms to <3ms via pre-measuring layout)
If we keep auto_resize = true, Viz stretches the old content to fill the new window size while waiting for the new frame. This stretching is the visual artifact [gutter](http://screencast/cast/NDk4NjA4NzY4NzU4NTc5MnxjMWFjYmI2OC1kNg) we are trying to fix. By disabling it and enforcing synchronization, we ensure blue butter is [gone](http://screencast/cast/NTM1MjA5OTAyOTE4ODYwOHw0MDY3NDFlNi0zMA) during resizing.
In your video without blue gutter the resize flow is visually much less responsive, though.
Because we need to wait for CompositorFrame both from browser and renderer, we still can be behind. The blue gutter exists only in dcheck-enabled builds, though some other visual artifacts could be present in release builds too.
That being said, I don't have conceptual objections to this CL, I'm just thinking if there is better solution for this problem.
Regarding the concern that disabling auto_resize_output_surface might cause us to "never draw" because the browser is always behind: We have simultaneously optimized the browser-side resize latency (reducing the Java UI thread resize handling from ~25ms to <3ms via pre-measuring layout)
Actually that probably makes the race worse.. if faster UI thread can handle more size events than viz can keep up with CompositorFrames, then we get to the situation where viz has no frame to draw. The right solution is throttle frequency of resize based on how fast viz can resize frames.
I'm just thinking if there is better solution for this problem.
For desktop's "continuous resize", I'm thinking:
- Make viz wait for CF of right size, ie this CL
- Disable surface sync for resize, ie if browser can put up a resized frame (containing updated tab strip), then just put it up asap without waiting for web content. Web content being slow is is somewhat expected, the "native" UI guttering would feel more janky
- Use surfaceRedrawNeededAsync to block until viz swaps with the right size
- Change slim scheduler to put up frames "asap" (we have the DelayedScheduler optimized for browser control, but I assume that's not needed anymore with BCIV)
- Probably still need some scheduling changes. Ben mentions that allowing two pending frames in slim looks much better. I think what happens is UI slow so in the time it puts up one frame, viz manages to send like 3 begin frames, and the first frame submitted blocks submitting frames in the subsequent ones. Increasing pending frames isn't the right solution, but can do something here
-------
For this CL, I think we can just do this for all of android, not just desktop. But added a kill switch (ie guard it with an enabled-by-default feature)
Related details
- Ben Chin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ben Chin (Gerrit)
Ben Chin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ben Chin
| Commit-Queue | +1 |
I removed the device form factor check, enabled the kFluidResize feature flag by default (acting as the requested "kill switch"), and applied the `auto_resize_output_surface = false` setting to all Android devices when the flag is enabled.
Thanks for your advice, they are quiet useful. In fact, I already came out with POC of `surfaceRedrawNeededAsync` and updated the go/clank-fr. It proves that `surfaceRedrawNeededAsync` is the correct solution here. As for changing scheduler, I will put you in post once I uploaded the perfetto log. Thanks again!
Related details
- Bo Liu
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
BASE_FEATURE(kFluidResize, base::FEATURE_ENABLED_BY_DEFAULT);this controls way more things than just auto_resize_output_surface though
added a new one, in content/common/features.h/cc
Related details
- Ben Chin
- Rakina Zata Amni
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |