libservo: Allow embedders to drive frame updates via `RefreshDriver` trait by coding-joedow · Pull Request #39072 · servo/servo

coding-joedow · 2025-09-01T13:55:58Z

Currently, the RefreshDriver creates a timer thread by default to limit the frame rate of animations to within 120fps. However, some platforms have already integrated VSYNC and even support LTPO, making the refresh rate of the current RefreshDriver mismatched with the display refresh rate of the platform. The main idea of this PR is to introduce the BeginFrameSource and BeginFrameSourceObserver types, allowing VSYNC to be integrated into the RefreshDriver.

Testing: It's hard to test rendering rate but some manual tests show that it takes effect on OHOS platform, and other behavior is covered by existing WPT tests.

mrobinson · 2025-09-01T15:32:28Z

I would like that the API we use for vsync platforms to be the implementation of a generalized interface that we also use for the fallback timer. For inspiration I think we can can follow the API that we use for clipboard. For instance, when you do not install your own custom RefreshDriver, you would fall back to the timer version.

coding-joedow · 2025-09-02T02:18:50Z

I would like that the API we use for vsync platforms to be the implementation of a generalized interface that we also use for the fallback timer. For inspiration I think we can can follow the API that we use for clipboard. For instance, when you do not install your own custom RefreshDriver, you would fall back to the timer version.

@mrobinson Thanks for your suggession, but I don't understand clearly. In fact, the newly-added BeginFrameSource is a generalized interface and the DefaultBeginFrameSource is the fallback timer. Let me guess, do you think that on platforms with Vsync integrated, the entire RefreshDriver could be feature-gated, just like the clipboard?

Besides, this PR's current change try to add a general frame tick observer and notification mechanism. However, this might be slightly beyond the scope of this PR. If a type similar to RefreshDriverDelegate were added, solely for the purpose of RefreshDriver observing frame ticks, perhaps the intent of this PR could be made clearer.

jschwe · 2025-09-02T07:05:31Z

For inspiration I think we can can follow the API that we use for clipboard. For instance, when you do not install your own custom RefreshDriver, you would fall back to the timer version.

Could you elaborate here to make sure we are on the same page? As far as I can see there currently is a ClipboardProvider trait, but no way for the embedder to install a custom clipboard provider. Instead messages are sent to/from the embedder.

I'm not fully up to date, so maybe I'm missing something, but for me it would make sense for the embedder to be able to install a custom refresh driver, which is then also responsible for calling wake() / informing the embedder of a new frame tick. @coding-joedow Is there any usecase for further observers?

mrobinson · 2025-09-02T08:35:13Z

Could you elaborate here to make sure we are on the same page? As far as I can see there currently is a ClipboardProvider trait, but no way for the embedder to install a custom clipboard provider. Instead messages are sent to/from the embedder.

There is a default ClipboardDelegate and then the embedder can call WebView::set_clipboard_delegate() to override it. My thought is that the the default RefreshDriver would use a timer (the current thing that we have) and then the embedder could override it by installing another RefreshDriver which would be driven by the vsync on platforms that support it. Thus we would expose a RefreshDriver trait and the embedder could implement it.

Currently the RefreshDriver has a pretty simple API, but I'm not entirely sure it is reasonable for vsync-based rendering so maybe a discussion about what the trait API looks like is in order.

coding-joedow · 2025-09-02T09:18:26Z

I'm not fully up to date, so maybe I'm missing something, but for me it would make sense for the embedder to be able to install a custom refresh driver, which is then also responsible for calling wake() / informing the embedder of a new frame tick. @coding-joedow Is there any usecase for further observers?

I think there is no other usecases for now.

coding-joedow · 2025-09-02T09:38:32Z

Currently the RefreshDriver has a pretty simple API, but I'm not entirely sure it is reasonable for vsync-based rendering so maybe a discussion about what the trait API looks like is in order.

Yes, the current RefreshDriver contains some animations logic except for the timer thread: 1) send TickAnimations message to script thread; 2) limit the rendering rate of animations. We need to clarify the role of RefreshDriver and how its implementation interacts with the compositor.

coding-joedow · 2025-09-02T09:46:04Z

In my humble opinion, if RefreshDriver becames a trait that can be optionally implemented by embedders, it should solely be responsible for notifying frame ticks and providing an API to start or stop notifications to avoid unnecessary execution of tick notification handlers.

coding-joedow · 2025-09-03T12:54:05Z

Currently the RefreshDriver has a pretty simple API, but I'm not entirely sure it is reasonable for vsync-based rendering so maybe a discussion about what the trait API looks like is in order.

Yes, the current RefreshDriver contains some animations logic except for the timer thread: 1) send TickAnimations message to script thread; 2) limit the rendering rate of animations. We need to clarify the role of RefreshDriver and how its implementation interacts with the compositor.

@mrobinson @jschwe Do you think it's would be better to move out these animations logic from RefreshDriver ?

mrobinson · 2025-09-04T10:10:49Z

Currently the RefreshDriver has a pretty simple API, but I'm not entirely sure it is reasonable for vsync-based rendering so maybe a discussion about what the trait API looks like is in order.

Yes, the current RefreshDriver contains some animations logic except for the timer thread: 1) send TickAnimations message to script thread; 2) limit the rendering rate of animations. We need to clarify the role of RefreshDriver and how its implementation interacts with the compositor.

@mrobinson @jschwe Do you think it's would be better to move out these animations logic from RefreshDriver ?

Sorry for the late reply. Yes, perhaps the animation logic should move out or we should create a RefreshDriverDelegate which handles the inner logic.

coding-joedow · 2025-09-04T13:23:04Z

Yes, perhaps the animation logic should move out or we should create a RefreshDriverDelegate which handles the inner logic.

Thanks for your reply. I prefer the first idea, but as @jschwe mentioned, there doesn't seem to be a particular reason to introduce a general frame tick notification mechanism at this point. Therefore, I will update this PR based on the RefreshDriverDelegate idea.

mrobinson · 2025-10-12T17:34:43Z

Apologies for taking so long to get to this. I have more time now for API-related work.

@coding-joedow You are totally correct that this change is very close to what I was thinking. I have a few minor changes to suggest around the design (in order to make the API surface a bit simpler) and naming, but I would love to tackle those this week.

BTW, with this change and a few other minor ones, I was able to enable flinging for on desktop with --simulate-touch-events turned on. I think this will finally enable us to have a very consistent touch event handling story on all platforms!

coding-joedow · 2025-10-13T14:11:35Z

Apologies for taking so long to get to this. I have more time now for API-related work.

@coding-joedow You are totally correct that this change is very close to what I was thinking. I have a few minor changes to suggest around the design (in order to make the API surface a bit simpler) and naming, but I would love to tackle those this week.

BTW, with this change and a few other minor ones, I was able to enable flinging for on desktop with --simulate-touch-events turned on. I think this will finally enable us to have a very consistent touch event handling story on all platforms!

Nice, this sounds very promising. I'm a bit busy at the moment and can't handle this PR right now. Sorry, and feel free to implement your ideas based on this patch, or create a new PR.

mrobinson · 2025-10-13T15:59:28Z

I've pushed my modifications to this branch which slightly reworks the original design. In addition, it integrates the old WebView::on_vsync signal into this new design, thereby allowing all platforms that have touch event handling to support fling (this may need to be disabled for Android, which I think also supports fling natively). Notably this allows testing touch and fling using --simulate-touch-events on desktop.

cc @jschwe

webbeef · 2025-10-13T16:57:08Z

fwiw, I tested that patch on a Pinephone Pro running Mobian, and that works very well!

components/shared/embedder/lib.rs

ports/servoshell/egl/app_state.rs

components/servo/lib.rs

ports/servoshell/egl/ohos/simpleservo.rs

mukilan · 2025-10-20T06:00:51Z

components/compositing/refresh_driver.rs

+        const FRAME_DURATION: Duration = Duration::from_millis(1000 / 120);
+        self.queue_timer(
+            FRAME_DURATION,
+            Box::new(move || {


Seems like we can pass new_start_frame_callback directly as the timer's callback if it had a + 'static bound. Any reason we can't or don't want to do it?

If I pass impl Fn() + Send + 'static I get this error:

`the size for values of type (dyn std::ops::Fn() + std::marker::Send + 'static) cannot be known at compilation time the trait std::marker::Sized is not implemented for (dyn std::ops::Fn() + std::marker::Send + 'static) (rustc E0277)

If I pass impl Fn() + Send + 'static I get this error:

the trait embedder_traits::RefreshDriver is not dyn compatible only type refresh_driver::TimerRefreshDriver implements embedder_traits::RefreshDriver; consider using it directly instead. (rustc E0038)

It seems we need to pass a Box.

@mrobinson not sure if you've seen this comment?

Sorry. I had the comment written, but it was still "Pending" in GitHub.

Hmm...not sure if expressed my intention clearly in the original comment.

I mean the type for new_start_frame_callback can be Box<dyn Fn() + Send + 'static> here as well as in the trait definition of RefreshDriver and other places. That should allow passing new_start_frame_callback directly to queue_timer as it also expects a Box<dyn Fn() + Send + 'static>.

Oh, I understand now. I've gone ahead and done this now.

components/compositing/refresh_driver.rs

components/compositing/lib.rs

components/compositing/refresh_driver.rs

mrobinson · 2025-10-20T09:39:47Z

@mukilan I've think I've addressed your review comments. PTAL.

github-actions · 2025-10-20T10:35:45Z

🔨 Triggering try run (#18649147210) for OpenHarmony

Signed-off-by: sharpshooter_pt <[email protected]>

Signed-off-by: Martin Robinson <[email protected]>

kongbai1996 · 2025-10-24T07:53:21Z

components/compositing/touch.rs

    }

-    pub fn on_vsync(&mut self) -> Option<FlingAction> {
+    pub fn notify_new_frame_start(&mut self) -> Option<FlingAction> {


Caused the problem of slippage failure.

coding-joedow requested review from jschwe and mrobinson as code owners September 1, 2025 13:55

coding-joedow marked this pull request as draft September 1, 2025 14:14

coding-joedow force-pushed the allow-vsync-integration-to-refresh-driver branch from 316c619 to 8e369ca Compare September 2, 2025 03:37

coding-joedow force-pushed the allow-vsync-integration-to-refresh-driver branch from 8e369ca to 69eb133 Compare September 4, 2025 06:27

mrobinson force-pushed the allow-vsync-integration-to-refresh-driver branch from 69eb133 to ee0fcf6 Compare October 13, 2025 15:50

mrobinson marked this pull request as ready for review October 13, 2025 15:50

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 13, 2025

mrobinson changed the title ~~compositor: allow OHOS to integrate VSYNC into RefreshDriver~~ libservo: Allow embedders to drive frame updates via RefreshDriver trait Oct 13, 2025

mrobinson mentioned this pull request Oct 13, 2025

ServoShell: introduce refresh_tick to trigger process_animation #37009

Closed

mukilan reviewed Oct 20, 2025

View reviewed changes

servo-highfive added the S-needs-rebase There are merge conflict errors. label Oct 20, 2025

mrobinson force-pushed the allow-vsync-integration-to-refresh-driver branch from ee0fcf6 to 6915372 Compare October 20, 2025 09:39

servo-highfive removed the S-needs-rebase There are merge conflict errors. label Oct 20, 2025

mrobinson force-pushed the allow-vsync-integration-to-refresh-driver branch from 6915372 to 30a4bd4 Compare October 20, 2025 10:05

mukilan approved these changes Oct 20, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 20, 2025

mrobinson force-pushed the allow-vsync-integration-to-refresh-driver branch from 30a4bd4 to bda1131 Compare October 20, 2025 10:24

servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 20, 2025

mrobinson added the T-ohos Do a try run on OpenHarmony label Oct 20, 2025

github-actions bot removed the T-ohos Do a try run on OpenHarmony label Oct 20, 2025

coding-joedow and others added 2 commits October 20, 2025 12:52

compositor: allow OHOS to integrate VSYNC into RefreshDriver

d7f1961

Signed-off-by: sharpshooter_pt <[email protected]>

Add the RefreshDriver trait

b82fda5

Signed-off-by: Martin Robinson <[email protected]>

mrobinson force-pushed the allow-vsync-integration-to-refresh-driver branch from bda1131 to b82fda5 Compare October 20, 2025 10:52

mrobinson approved these changes Oct 20, 2025

View reviewed changes

servo-highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 20, 2025

mrobinson enabled auto-merge October 20, 2025 10:53

mrobinson added this pull request to the merge queue Oct 20, 2025

servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 20, 2025

Merged via the queue into servo:main with commit 547dd23 Oct 20, 2025
30 checks passed

servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 20, 2025

mrobinson mentioned this pull request Oct 22, 2025

Useless calls from the embedder #25305

Closed

kongbai1996 reviewed Oct 24, 2025

View reviewed changes

kongbai1996 mentioned this pull request Oct 24, 2025

Compositor: Fling failure #40127

Open

Uh oh!

Conversation

coding-joedow commented Sep 1, 2025

Uh oh!

mrobinson commented Sep 1, 2025

Uh oh!

coding-joedow commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jschwe commented Sep 2, 2025

Uh oh!

mrobinson commented Sep 2, 2025

Uh oh!

coding-joedow commented Sep 2, 2025

Uh oh!

coding-joedow commented Sep 2, 2025

Uh oh!

coding-joedow commented Sep 2, 2025

Uh oh!

coding-joedow commented Sep 3, 2025

Uh oh!

mrobinson commented Sep 4, 2025

Uh oh!

coding-joedow commented Sep 4, 2025

Uh oh!

mrobinson commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coding-joedow commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Oct 13, 2025

Uh oh!

webbeef commented Oct 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukilan Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mrobinson Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mukilan Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mrobinson Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mukilan Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

mrobinson Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrobinson commented Oct 20, 2025

Uh oh!

github-actions bot commented Oct 20, 2025

Uh oh!

Uh oh!

kongbai1996 Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

coding-joedow commented Sep 2, 2025 •

edited

Loading

mrobinson commented Oct 12, 2025 •

edited

Loading

coding-joedow commented Oct 13, 2025 •

edited

Loading