Fix visual gutters during mixed resizing [chromium/src : main]
ben chin (Gerrit)
ben chin voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- 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 4 comments![]( "Open in Gerrit")
this is a lot of complexity and requires justification for landing, and I don't see that justification yet
1. Holds the child SurfaceView at the maximum dimensions reached during an active drag gesture. Preventing the surface from shrinking mid-drag eliminates empty background gutters when dragging diagonally or smaller.this doesn't make sense to me..
what's causing gutter in this case? Is viz being too fast or too slow? If it's too slow, then is it running into the throttling for pure shrink?
2. Replaces refresh-rate dependent frame counting with a 200ms elapsed time check (FLUID_RESIZE_TIMEOUT_MS). Once 200ms passes without a layout change and Viz is idle, the surface cleanly resets after the next buffer swap. This aligns 1:1 with AOSP WindowManager's 200ms drag-resizing sync timeout and uses ViewUtils.requestLayout() for jank tracing and presubmit compliance.what's this?
gfx::Size display_size = screen->GetPrimaryDisplay().GetSizeInPixel();get display that the window is in, not the primary one
Related details
- ben chin
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. |
ben chin (Gerrit)
ben chin voted and added 3 comments![]( "Open in Gerrit")
Votes added by ben chin
| Commit-Queue | +1 |
3 comments
1. Holds the child SurfaceView at the maximum dimensions reached during an active drag gesture. Preventing the surface from shrinking mid-drag eliminates empty background gutters when dragging diagonally or smaller.this doesn't make sense to me..
what's causing gutter in this case? Is viz being too fast or too slow? If it's too slow, then is it running into the throttling for pure shrink?
Viz is too slow during window expansion (W++), so it's not related to pure shrink throttling. The OS window expands faster than clank, exposing a gutter. Extending this SolidColorLayer covers the new area instantly without waiting for layout.
2. Replaces refresh-rate dependent frame counting with a 200ms elapsed time check (FLUID_RESIZE_TIMEOUT_MS). Once 200ms passes without a layout change and Viz is idle, the surface cleanly resets after the next buffer swap. This aligns 1:1 with AOSP WindowManager's 200ms drag-resizing sync timeout and uses ViewUtils.requestLayout() for jank tracing and presubmit compliance.ben chinwhat's this?
My previous solution is waiting for a fixed number of compositor frames to pass before considering the resize 'finished' and resetting the surface. However, counting frames is inconsistent across different device refresh rates (e.g., 5 frames is ~80ms on 60Hz, but only ~40ms on 120Hz) which might show gutter. I replaced that with a strict 200ms timeout, which matches AOSP WindowManager's own drag-resize sync timeout.
gfx::Size display_size = screen->GetPrimaryDisplay().GetSizeInPixel();get display that the window is in, not the primary one
- 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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- 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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- 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 2 comments![]( "Open in Gerrit")
1. Holds the child SurfaceView at the maximum dimensions reached during an active drag gesture. Preventing the surface from shrinking mid-drag eliminates empty background gutters when dragging diagonally or smaller.ben chinthis doesn't make sense to me..
what's causing gutter in this case? Is viz being too fast or too slow? If it's too slow, then is it running into the throttling for pure shrink?
Viz is too slow during window expansion (W++), so it's not related to pure shrink throttling. The OS window expands faster than clank, exposing a gutter. Extending this SolidColorLayer covers the new area instantly without waiting for layout.
if viz is too slow, then can't you just disable the throttling you added?
2. Replaces refresh-rate dependent frame counting with a 200ms elapsed time check (FLUID_RESIZE_TIMEOUT_MS). Once 200ms passes without a layout change and Viz is idle, the surface cleanly resets after the next buffer swap. This aligns 1:1 with AOSP WindowManager's 200ms drag-resizing sync timeout and uses ViewUtils.requestLayout() for jank tracing and presubmit compliance.ben chinwhat's this?
My previous solution is waiting for a fixed number of compositor frames to pass before considering the resize 'finished' and resetting the surface. However, counting frames is inconsistent across different device refresh rates (e.g., 5 frames is ~80ms on 60Hz, but only ~40ms on 120Hz) which might show gutter. I replaced that with a strict 200ms timeout, which matches AOSP WindowManager's own drag-resize sync timeout.
previous solution that was never submitted? then mentioning that solution will only add confusion
this is still an arbitrary timeout though, is framework going to expose that timeout?
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
1. Holds the child SurfaceView at the maximum dimensions reached during an active drag gesture. Preventing the surface from shrinking mid-drag eliminates empty background gutters when dragging diagonally or smaller.ben chinthis doesn't make sense to me..
what's causing gutter in this case? Is viz being too fast or too slow? If it's too slow, then is it running into the throttling for pure shrink?
Bo LiuViz is too slow during window expansion (W++), so it's not related to pure shrink throttling. The OS window expands faster than clank, exposing a gutter. Extending this SolidColorLayer covers the new area instantly without waiting for layout.
if viz is too slow, then can't you just disable the throttling you added?
also, so me an annotated trace with all the events that's leading up to gutters
I don't think this vague description of words good enough anymore
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 2 comments![]( "Open in Gerrit")
Votes added by ben chin
| Commit-Queue | +1 |
2 comments
1. Holds the child SurfaceView at the maximum dimensions reached during an active drag gesture. Preventing the surface from shrinking mid-drag eliminates empty background gutters when dragging diagonally or smaller.ben chinthis doesn't make sense to me..
what's causing gutter in this case? Is viz being too fast or too slow? If it's too slow, then is it running into the throttling for pure shrink?
Bo LiuViz is too slow during window expansion (W++), so it's not related to pure shrink throttling. The OS window expands faster than clank, exposing a gutter. Extending this SolidColorLayer covers the new area instantly without waiting for layout.
Bo Liuif viz is too slow, then can't you just disable the throttling you added?
also, so me an annotated trace with all the events that's leading up to gutters
I don't think this vague description of words good enough anymore
Hi Bo,
Thank you for the feedback. I have completely removed the `setDrawPaused` pure shrink throttling from `CompositorView.surfaceChanged`.
To answer your question on whether disabling throttling alone fixes the issue and to provide the annotated trace comparison from [Perfetto SQL](https://ui.perfetto.dev/#!/?s=b35e56c6c1cf84113a7645543359f2978734532c) log [slices](https://screenshot.googleplex.com/4fiJ8npvneP6kGq):
#### 1. Throttling Removed: Viz Buffer Swap Latency Dropped by ~90%
• Before (With setDrawPaused throttling): Buffer swaps ( CompositorView.didSwapBuffers ) lagged behind UI layout by 226ms to 1,131ms, with up to 12 consecutive onLayout
expansion events queuing up before a single buffer swap returned.
• After (No throttling, this CL): With throttling removed and layout stabilized, CompositorView.didSwapBuffers now returns cleanly within 5ms to 42ms (~1 to 2 frames).
#### 2. Why We Still Need Layer & Surface Bounding Even With 5ms–42ms Swaps
Even at peak unthrottled speed ( ~5ms–42ms swap latency), when the user actively drags to expand the window ( W++ ):
1. The 1–2 Frame Multi-Process Gap: CompositorView.onLayout expands the OS window synchronously at t = 0 ms . Clank's multi-process rendering pipeline takes 1–2 vsync frames ( ~15ms–42ms ) to produce and swap the newly sized buffer.
2. Covering the Gap with Zero Overhead: During those 15ms–42ms before the new buffer swaps, the outgoing buffer ( W_old ) sits inside the wider OS window ( W_new ).
2. Replaces refresh-rate dependent frame counting with a 200ms elapsed time check (FLUID_RESIZE_TIMEOUT_MS). Once 200ms passes without a layout change and Viz is idle, the surface cleanly resets after the next buffer swap. This aligns 1:1 with AOSP WindowManager's 200ms drag-resizing sync timeout and uses ViewUtils.requestLayout() for jank tracing and presubmit compliance.ben chinwhat's this?
Bo LiuMy previous solution is waiting for a fixed number of compositor frames to pass before considering the resize 'finished' and resetting the surface. However, counting frames is inconsistent across different device refresh rates (e.g., 5 frames is ~80ms on 60Hz, but only ~40ms on 120Hz) which might show gutter. I replaced that with a strict 200ms timeout, which matches AOSP WindowManager's own drag-resize sync timeout.
previous solution that was never submitted? then mentioning that solution will only add confusion
this is still an arbitrary timeout though, is framework going to expose that timeout?
I have removed the mention of the unsubmitted solution from the commit msg.
Regarding whether the framework exposes the timeout: Unfortunately, the Android SDK does not expose this via public APIs or provide a drag-end callback. However, inside [AOSP WindowManagerService](https://source.corp.google.com/h/googleplex-android/platform/superproject/main/+/main:frameworks/base/services/core/java/com/android/server/wm/DragResizingSynchronizer.java;l=43), `SYNC_TIMEOUT = 200` is the private constant used internally to synchronize and release window resize barriers. We use `FLUID_RESIZE_TIMEOUT_MS = 200` here to mirror that exact same OS-level timeout.
Related details
- 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 Commit-Queue+1![]( "Open in Gerrit")
- Bo Liu
- David Trainor
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 4 comments![]( "Open in Gerrit")
private final Choreographer.FrameCallback mResetResizeStateFrameCallback =why use choreographer? and why busy loop it? a simple delayed task should do?
protected void onLayout(boolean changed, int left, int top, int right, int bottom) {is there a more declarative way to just set the size of the view, instead of hacking onlayout or onmeasure?
int targetWidth = Math.max(w, mMaxResizeWidth);how about just get the display size and allocate that size immediately? instead of still doing incremental resizing
CompositorViewJni.get().setDrawPaused(mNativeCompositorView, false);I think pull out the removing of the pause draw code into a separate CL *ahead* of this change
and make sure to remove native side of stuff as well
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin has uploaded the change for review![]( "Open in Gerrit")
Commit message
Fix visual gutters during mixed resizing
Mixed resizing Clank (e.g., W++, H--) can cause Clank's rendering engine
to lag behind the OS window, exposing unrendered background gutters.
This CL fixes this through two improvements:
1. Allocates the full display size to the child SurfaceView immediately
when resizing starts, and uses declarative setTranslationX/Y to keep
the content visually stationary.
2. Uses a Handler-based 200ms timeout (FLUID_RESIZE_TIMEOUT_MS) on the
UI thread to cleanly reset the SurfaceView back to MATCH_PARENT when
resizing stops.
Change diff
Change information
- M chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
Related details
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 Commit-Queue+1![]( "Open in Gerrit")
- 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 added 4 comments![]( "Open in Gerrit")
private final Choreographer.FrameCallback mResetResizeStateFrameCallback =why use choreographer? and why busy loop it? a simple delayed task should do?
Yes, replaced Choreographer with a delayed runnable. We now schedule a single 200ms task when resizing starts. If Viz is still busy when it runs, we retry with a 16ms delay.
protected void onLayout(boolean changed, int left, int top, int right, int bottom) {is there a more declarative way to just set the size of the view, instead of hacking onlayout or onmeasure?
update the SurfaceView's LayoutParams to the display size when resize starts
int targetWidth = Math.max(w, mMaxResizeWidth);how about just get the display size and allocate that size immediately? instead of still doing incremental resizing
sure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
CompositorViewJni.get().setDrawPaused(mNativeCompositorView, false);I think pull out the removing of the pause draw code into a separate CL *ahead* of this change
and make sure to remove native side of stuff as well
sure! created
https://chromium-review.googlesource.com/c/chromium/src/+/8113587
and a new version for all the suggested solution we discussed at
https://chromium-review.googlesource.com/c/chromium/src/+/8113588
Related details
- Bo Liu
- David Trainor
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
int targetWidth = Math.max(w, mMaxResizeWidth);ben chinhow about just get the display size and allocate that size immediately? instead of still doing incremental resizing
sure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
As I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588. If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine). Therefore, the system stops holding the parent window frame and user will see the gutter.
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
int targetWidth = Math.max(w, mMaxResizeWidth);ben chinhow about just get the display size and allocate that size immediately? instead of still doing incremental resizing
ben chinsure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
As I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588. If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine). Therefore, the system stops holding the parent window frame and user will see the gutter.
As I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588.
Why not keep using this CL? Having two similar ones is very confusing. Plus I can't look at incremental diffs.
If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine).
I think it made sense up to here.
Therefore, the system stops holding the parent window frame and user will see the gutter.
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin voted and added 2 comments![]( "Open in Gerrit")
Votes added by ben chin
| Commit-Queue | +1 |
2 comments
int targetWidth = Math.max(w, mMaxResizeWidth);ben chinhow about just get the display size and allocate that size immediately? instead of still doing incremental resizing
ben chinsure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
Bo LiuAs I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588. If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine). Therefore, the system stops holding the parent window frame and user will see the gutter.
As I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588.
Why not keep using this CL? Having two similar ones is very confusing. Plus I can't look at incremental diffs.
If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine).
I think it made sense up to here.
Therefore, the system stops holding the parent window frame and user will see the gutter.
This I don't follow. What happens that's the problem?
I've uploaded the new patchset back here!
Regarding the technical point: Because the `SurfaceView` dimensions are static, the Android OS removes the synchronization lock (BLAST sync) that usually ties the parent window expansion to the child surface redraw. Without that lock, the parent window border expands instantly with the user's finger, but our Viz compositor rendering is fully asynchronous and always lags behind. The gap between the instantly expanded window border and the lagging asynchronous web content is exposed as the visual gutter.
CompositorViewJni.get().setDrawPaused(mNativeCompositorView, false);ben chinI think pull out the removing of the pause draw code into a separate CL *ahead* of this change
and make sure to remove native side of stuff as well
sure! created
https://chromium-review.googlesource.com/c/chromium/src/+/8113587
and a new version for all the suggested solution we discussed at
https://chromium-review.googlesource.com/c/chromium/src/+/8113588
discarded https://chromium-review.googlesource.com/c/chromium/src/+/8113588 and will continue on this CL for after removal logic
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
int targetWidth = Math.max(w, mMaxResizeWidth);ben chinhow about just get the display size and allocate that size immediately? instead of still doing incremental resizing
ben chinsure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
Bo LiuAs I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588. If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine). Therefore, the system stops holding the parent window frame and user will see the gutter.
ben chinAs I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588.
Why not keep using this CL? Having two similar ones is very confusing. Plus I can't look at incremental diffs.
If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine).
I think it made sense up to here.
Therefore, the system stops holding the parent window frame and user will see the gutter.
This I don't follow. What happens that's the problem?
I've uploaded the new patchset back here!
Regarding the technical point: Because the `SurfaceView` dimensions are static, the Android OS removes the synchronization lock (BLAST sync) that usually ties the parent window expansion to the child surface redraw. Without that lock, the parent window border expands instantly with the user's finger, but our Viz compositor rendering is fully asynchronous and always lags behind. The gap between the instantly expanded window border and the lagging asynchronous web content is exposed as the visual gutter.
Two questions wrt to "the synchronization lock"..
sounds like there is *some* synchronization? what does it synchronize exactly? Waiting for sufaceRedrawNeededAsync?
second.. there is no way to predict what the user will do after doing a "diagonal" resize. maybe then user does a pure shrink without lifting the touch or mouse button, and you are going to run into the same problem where surface view doesn't resize?
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin voted and added 1 comment![]( "Open in Gerrit")
Votes added by ben chin
| Commit-Queue | +1 |
1 comment
int targetWidth = Math.max(w, mMaxResizeWidth);ben chinhow about just get the display size and allocate that size immediately? instead of still doing incremental resizing
ben chinsure! grab the max `DisplayMetrics` from the context and lock the bounds to it immediately.
Bo LiuAs I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588. If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine). Therefore, the system stops holding the parent window frame and user will see the gutter.
ben chinAs I experimented at https://chromium-review.git.corp.google.com/c/chromium/src/+/8113588.
Why not keep using this CL? Having two similar ones is very confusing. Plus I can't look at incremental diffs.
If we allocate `mDisplayWidth/mDisplayHeight` upfront on the first drag event, the structural view bounds become static for the rest of the drag. When a SurfaceView's layout dimensions stop changing, Android's `ViewRootImpl` stops triggering its geometry transactions (BLASTSyncEngine).
I think it made sense up to here.
Therefore, the system stops holding the parent window frame and user will see the gutter.
This I don't follow. What happens that's the problem?
Bo LiuI've uploaded the new patchset back here!
Regarding the technical point: Because the `SurfaceView` dimensions are static, the Android OS removes the synchronization lock (BLAST sync) that usually ties the parent window expansion to the child surface redraw. Without that lock, the parent window border expands instantly with the user's finger, but our Viz compositor rendering is fully asynchronous and always lags behind. The gap between the instantly expanded window border and the lagging asynchronous web content is exposed as the visual gutter.
Two questions wrt to "the synchronization lock"..
sounds like there is *some* synchronization? what does it synchronize exactly? Waiting for sufaceRedrawNeededAsync?
second.. there is no way to predict what the user will do after doing a "diagonal" resize. maybe then user does a pure shrink without lifting the touch or mouse button, and you are going to run into the same problem where surface view doesn't resize?
Hi Bo,
To answer your question:
1. When `SurfaceView` layout bounds change during a resize, Android's `ViewRootImpl` detects the geometry change and registers a `surfaceRedrawNeededAsync` callback via `BLASTSyncEngine`. This holds the OS window transaction until `CompositorView` completes its draw callback. When we lock `SurfaceView` layout bounds upfront to static display dimensions, `ViewRootImpl` sees no layout change on the child `SurfaceView` and skips registering `surfaceRedrawNeededAsync` / `BLASTSyncEngine` barriers. The OS window frame resizes uninhibited while Viz renders asynchronously, exposing the lag as a gutter.
2. Using this CL will not have the issue you described. I tested this exact scenario and Web content continues rendering and updating frames throughout the gesture.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
2. Using this CL will not have the issue you described. I tested this exact scenario and Web content continues rendering and updating frames throughout the gesture.
That amounts to "it looks ok", which is not convincing. The initial diagonal resize puts us in "don't shrink surfaceview" mode, then surfaceview shouldn't resize on shrinking resize, and then we run into the problem you described? How is that avoided?
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
Here is the mechanism that avoids frame starvation during mid-drag shrink reversals:
1. [Removal of SetDrawPaused](https://chromium-review.git.corp.google.com/c/chromium/src/+/8113587): Previously, shrink resizes triggered `SetDrawPaused(true)`, which explicitly paused the cc::slim compositor until SurfaceView received a shrink callback. Because `SetDrawPaused` is gone, CompositorImpl and Viz continue producing and swapping frames
2. When shrinking, the parent window frame clips the oversized (W_max, H_max) surface. setTranslationX/Y keeps the content anchored while Viz renders at the active window viewport size (W_current, H_current). Once drag stops, the 200ms timer resets SurfaceView back to `MATCH_PARENT`.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
Let me explain in a more concise way ..... In the old code, whenever the window shrank, Clank called `SetDrawPaused(true)` which stops rendering new frames until SurfaceView finishes shrinking. In this CL, we remove `SetDrawPaused` so Viz keeps producing frames even if SurfaceView stays at (W_max, H_max). The shrinking window frame just clips the oversized surface, so there's no freeze and no gutter.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
that's not what I'm asking. I'm asking in this case SurfaceView no longer resizes on window resize, so there is no "synchronization lock", how is that not a problem?
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
No, because the surface is already bigger than the window and the bigger part will be croped. The sync lock is needed to hold the window frame expansion until a larger surface buffer is allocated to prevent gutters (eg: the window expands past W_max triggers sync lock)
When SurfaceView is held at (W_max, H_max), the surface buffer is already larger than the window frame. Expanding or shrinking the window just reveals or crops the already-allocated surface, so there's no unallocated nor need for a sync lock.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
that sounds like you are saying "synchronization lock" just isn't needed, then why not just do what I suggested before and just start with display-sized surface when drag starts, and don't bother with growing surfaceview on size increase?
we are going in circles here...
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
[IIUC](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/compositor_impl_android.cc;drc=e4680f2108abd2269c3706cacfd46e50af36aff1;l=419), Viz renders GPU buffers matching the active window viewport. If SurfaceView container bounds are set to full display upfront, expanding the window from 1000px to 1200px doesn't trigger layout bounds changes. Android's `ViewRootImpl` skips BLASTSyncEngine sync, expanding the window frame to 1200px instantly while the GPU buffer inside is still 1000px will expose a 200px gutter.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
[IIUC](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/compositor_impl_android.cc;drc=e4680f2108abd2269c3706cacfd46e50af36aff1;l=419), Viz renders GPU buffers matching the active window viewport. If SurfaceView container bounds are set to full display upfront,
Assuming 1200px is the display size in this example, then SurfaceView and viz should already be sized to 1200px at this point
expanding the window from 1000px to 1200px doesn't trigger layout bounds changes. Android's `ViewRootImpl` skips BLASTSyncEngine sync, expanding the window frame to 1200px instantly while the GPU buffer inside is still 1000px
No, viz should be 1200px already
will expose a 200px gutter.
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin added 1 comment![]( "Open in Gerrit")
Viz surface size (SetWindowBounds) [controls](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/renderer_host/render_widget_host_view_android.cc;drc=0976730981877ed30e1cb06c63ad7d60c030c7f4;l=3218) Blink's CSS Web Viewport (host_->SetViewportRectAndScale) as well so If we set `SetWindowBounds(1200px)` upfront while the window is 1000px then web content will be cut off. In addition, as the user drags (1000 px → 1050 px → 1200 px), web text fails to reflow dynamically to fit the moving window border.
Related details
- Bo Liu
- David Trainor
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. |
Bo Liu (Gerrit)
Bo Liu added 1 comment![]( "Open in Gerrit")
web content should still resize with the window, I mean that should be obvious, so only the surface is display-sized, and viz can fill in the gutter with whatever color is necessary, and whatever details that needs to be worked out to make all this happen needs to be worked out
but again, whatever problem are you are describing with going straight to display size will also happen if you do the diagonal resize, then grow, then shrink, then grow, so it's details that needs to be worked out anyway
Related details
- David Trainor
- ben chin
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. |
ben chin (Gerrit)
ben chin voted and added 1 comment![]( "Open in Gerrit")
Votes added by ben chin
| Commit-Queue | +1 |
1 comment
updated the patch to adopt your suggestion of using a display-sized SurfaceView upfront.
Related details
- Bo Liu
- David Trainor
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. |