BeginFrameSourceWayland improved handling for late/missing callbacks [chromium/src : main]
Kramer Ge (Gerrit)
Kramer Ge added 6 comments![]( "Open in Gerrit")
const bool suspended = frame_manager_->IsWindowSuspended();does the compositor always ack a `frame` callback when it un-suspends us? Since we do not trust compositor delivering frame callbacks consistently I think we probably want to observe when window is unsuspended.
wl::Object<wl_callback> no_damage_frame_callback_;There's a potential TODO here, I think we can think about integrating this synthetic callback request as dummy entries into the `submitted_frames_` s.t. we can deduce what bfs should do by looking at the last submitted frame.
MaybeProcessPendingFrame();This can cause `OnFrameCallbackUnavailable()` being called multiple times back-to-back, if:
```
!should_skip_frame_callbacks_ && !!submitted_frames_.back()->wl_frame_callback
```
which happens when `surface->AttachBuffer()` returns `false`. or when `DiscardFrame()` happens. Ideally we only want to do one beginframe at the end. Perhaps we can do a `ScopedFrameCallbackCount` object which tracks whether this recursive call requested any callback, and call `OnFrameCallbackUnavailable()` if it destructs and that number is 0.
if (frame_timing_observer_ && !should_skip_frame_callbacks_ &&`should_skip_frame_callbacks_` is true when we know chrome is capturing content, meanwhile the window is suspended. So we should temporarily drive a synthetic begin frame timer in this scenario. With timeout being 100ms google meet video captured in a minimized window would be 10fps.
// On Mutter, we don't receive frame.callback acks if we don't attach a
// new wl_buffer, which leads to graphics freeze. So only setup
// frame_callback when we're attaching a different buffer and frame
// callbacks are not being skipped due to video capture in the background.I forgot this workaround existed. Is this observation still true? If true, `no_damage_frame_callback_` we set will not be acked and we're still relying on the 100ms timeout.
if (no_damage_frame_callback_ ||I think we should reset this field when `Hide()`, because the surface may become unmapped.
Related details
- Mitchell Cohen
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. |
Mitchell Cohen (Gerrit)
Mitchell Cohen added 7 comments![]( "Open in Gerrit")
Thanks @fang...@chromium.org. Your review inspired me to change the ownership model so that the frame source now owns the BFS, which makes it much easier to observe state changes and control the lifecycle. Let me know what you think!
const bool suspended = frame_manager_->IsWindowSuspended();does the compositor always ack a `frame` callback when it un-suspends us? Since we do not trust compositor delivering frame callbacks consistently I think we probably want to observe when window is unsuspended.
Done
There's a potential TODO here, I think we can think about integrating this synthetic callback request as dummy entries into the `submitted_frames_` s.t. we can deduce what bfs should do by looking at the last submitted frame.
Done
MaybeProcessPendingFrame();This can cause `OnFrameCallbackUnavailable()` being called multiple times back-to-back, if:
```
!should_skip_frame_callbacks_ && !!submitted_frames_.back()->wl_frame_callback
```
which happens when `surface->AttachBuffer()` returns `false`. or when `DiscardFrame()` happens. Ideally we only want to do one beginframe at the end. Perhaps we can do a `ScopedFrameCallbackCount` object which tracks whether this recursive call requested any callback, and call `OnFrameCallbackUnavailable()` if it destructs and that number is 0.
True, but it is not a huge issue if `OnFrameCallbackUnavailable()` is called multiple times since the frame source will only handle one frame per vsync.
I think the cleanest improvement would be to replace the tail call with a while loop and some internal state, but it seems like too much for this CL. Thoughts?
if (frame_timing_observer_ && !should_skip_frame_callbacks_ &&`should_skip_frame_callbacks_` is true when we know chrome is capturing content, meanwhile the window is suspended. So we should temporarily drive a synthetic begin frame timer in this scenario. With timeout being 100ms google meet video captured in a minimized window would be 10fps.
Done
// On Mutter, we don't receive frame.callback acks if we don't attach a
// new wl_buffer, which leads to graphics freeze. So only setup
// frame_callback when we're attaching a different buffer and frame
// callbacks are not being skipped due to video capture in the background.I forgot this workaround existed. Is this observation still true? If true, `no_damage_frame_callback_` we set will not be acked and we're still relying on the 100ms timeout.
I am unsure about this, I haven't been able to repro it myself.
I think we should reset this field when `Hide()`, because the surface may become unmapped.
- Kramer Ge
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. |