Skip to content
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

Improve Firefox surface.frame event quirk #3740

Merged
merged 4 commits into from
Feb 7, 2025

Conversation

RAOF
Copy link
Contributor

@RAOF RAOF commented Feb 6, 2025

We currently queue a send_frame_events() 16ms after receiving
a surface.commit() with no buffer but that requests a frame
event.

When send_frame_events() is triggered it sends all the frame
events at the time of trigger; this results in us sending frame
events at a sub-optimal time and breaks the way that WLCS uses
the frame event to determine when a buffer has been used.

Instead, maintain a separate heartbeat_quirk_frame_events list
that gets sent by the 16ms timer.

We want to move the `executor_send_frame_callback` into its
consumer; this obviously doesn't work if it's `const`.
@RAOF RAOF requested a review from a team as a code owner February 6, 2025 06:09
@RAOF RAOF changed the title Mireng 952/improve firefox frame event quirk Improve Firefox surface.frame event quirk Feb 6, 2025
@RAOF
Copy link
Contributor Author

RAOF commented Feb 6, 2025

I think we might also want to fix issue #3739 while we're here, but I didn't want to block this on that. I've had a look, and it's not much work, but there's some subtlety around wl_event_loop lifetime.

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible, it isn't immediately obvious why the wp-viewporter tests are failing. But as they sync using the frame callback there's likely an implicit assumption that no longer holds

RAOF added 3 commits February 7, 2025 11:03
We currently queue a `send_frame_events()` 16ms after receiving
a `surface.commit()` with no buffer but that requests a frame
event.

When `send_frame_events()` is triggered it sends *all* the frame
events at the time of trigger; this results in us sending frame
events at a sub-optimal time and breaks the way that WLCS uses
the frame event to determine when a buffer has been used.

Instead, maintain a separate `heartbeat_quirk_frame_events` list
that gets sent by the 16ms timer.
Oops! The `FrameExecutor` (annoyingly!) doesn't execute on the
Wayland event loop. We need a second bounce to get the
`send_frame_callbacks()` call there.
@RAOF RAOF force-pushed the MIRENG-952/improve-firefox-frame-event-quirk branch from c028ed1 to 6c88bcd Compare February 7, 2025 00:10
@RAOF
Copy link
Contributor Author

RAOF commented Feb 7, 2025

Ok! The problem was that the new code was broken! (it was never triggered). The WpViewporter tests happen to use buffer-less frame events, so that's why those broke.

It seems that Firefox is more tolerant of not sending those events than it was (although, now that I've actually fixed it I can see that there are behavioural differences that just seemed like regular weirdness 🤷‍♀️ )

@RAOF
Copy link
Contributor Author

RAOF commented Feb 7, 2025

There's now also a WLCS test that catches this: canonical/wlcs#350

Copy link
Collaborator

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's what we missed!

@AlanGriffiths AlanGriffiths added this pull request to the merge queue Feb 7, 2025
Merged via the queue into main with commit df4d105 Feb 7, 2025
36 of 39 checks passed
@AlanGriffiths AlanGriffiths deleted the MIRENG-952/improve-firefox-frame-event-quirk branch February 7, 2025 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants