feat!: use display link api to implement vsync on macos.

[crupest]

Fix #2073, fix #2093

This pr uses display link api on macos to correctly do vsync. The opengl vsync seems broken on macos.

BLOCKED by #2097

UNDER WORKING: Don't merge now. Just to stop from duplicate work.

BREAKING CHANGE: This may totally change the structure of vsync usage.

TODOS:

• After Window creation fails #2097 fixed, have a leading test of whether this works.
• Use the right way to wait for vsync.
• Create a context to share sync primitives between main thread and display link thread.
• Make VsyncMacos an individual enum value instead of using Windows one.
• Add logging.
• Run tracy to get test data.
• Examine the result.

@fredizzimo I have some questions about vsync usage. In current code, vsync seems owned by the update loop. But as I pointed out before, the display link is specific to a screen. So the structure of vsync usage should be changed as I think. To be simple, I make vsync owned by a window. And when window moves and the screen is changed, vsync updates. Do you have any ideas about this?

[fredizzimo]

@fredizzimo I have some questions about vsync usage. In current code, vsync seems owned by the update loop. But as I pointed out before, the display link is specific to a screen. So the structure of vsync usage should be changed as I think. To be simple, I make vsync owned by a window. And when window moves and the screen is changed, vsync updates. Do you have any ideas about this?

The changes you have made makes perfect sense and also simplifies the code without the need to pass the vsync object around. I don't remember the exact reason why it was owned by the update loop in the first place, it might have been needed in some of the earlier versions of the render loop, or just because I thought it fitted better there than in the window.

[crupest]

@fredizzimo I think coding is completed and it's time to examine it.

Actually when I follow up the main branch, the bug already disappears. And even using the display link api, there are still some frames lost. I don't feel there is a big difference between using or not using display link. And current performance is good enough as far as I'm concerned . I don't know this makes some sense any more.

Whatever, here are the new tracy:

1. main -> main branch, no command line arguments
2. display-link -> this pr branch, no command line arguments
3. no-vsync -> main branch, with --no-vsync

tracy-log.zip

[fredizzimo]

@crupest, thank you.

I think we need more testing before merging, I will ask for that both in the issue and on Discord, after one remaining issue is fixed as described below.

But from the traces it looks reasonable.

1. main -> main branch, no command line arguments - This jitters between a fast and a slow frame
2. display-link -> this pr branch, no command line arguments - Mostly looks good, but there are periods of jitter (more on that later)
3. no-vsync -> main branch, with --no-vsync -- No jitter, but seems to have two modes 8 ms and 16 ms

I think the display-link case can be improved by forcing the swap interval to 0, in and always taking this branch

neovide/src/renderer/opengl.rs

Line 129 in b79b7cc

surface.set_swap_interval(&context, SwapInterval::DontWait)

The reason for the jitter seems to be that swap_buffers sometimes blocks after the displaylink wait, and disabling that should hopefully fix it. Edit: I forgot to add that the branch can be simplified, since the only special case becomes when vsync is enabled and the platform is X11, where the swap interval should be set, in all other cases it should not be set.

NOTE: I have not yet had time to review the actual code.

[crupest]

@fredizzimo I have pushed the new code to force swap interval to 0.

Here is the new tracy: tracy.zip

Just take no hurry. Let's make it more mutual before merge.

By the way, I'll try to take a look on tracy by myself. I think it's good for me to learn on this.

Thank you very much!

[fredizzimo]

At least the tracy log looks good now with a stable 60 FPS. Except for the long red peaks, where nothing seem to be happening. But maybe you did not run it with --no-idle? In that case it's expected to not render when nothing happens.

[crupest]

@fredizzimo With --no-idle, tracy.zip

Looks much better as I can see. EDIT: I mean the tracy log.

[fredizzimo]

If you ran with --no-idle it's a bit weird. It looks like the update loop is actually called regularly during those frame drops, but it never calls draw_frame to actually render. I wonder why that happens?

It should be forced here with --no-idle

neovide/src/window/update_loop.rs

Lines 135 to 140 in 3e83b03

	if self.should_render || !self.idle {
	if self.vsync.uses_winit_throttling() {
	window_wrapper.windowed_context.window().request_redraw();
	} else {
	self.render(window_wrapper);
	}

[crupest]

Sorry I need to rest now. I'll take a look another day. I understand what you mean and I'll do some debug then.

[fredizzimo]

No problems, thank you for the work so far, it already seems like a huge improvement!

[crupest]

@fredizzimo Hey, I'm back again.

If you ran with --no-idle it's a bit weird. It looks like the update loop is actually called regularly during those frame drops, but it never calls draw_frame to actually render. I wonder why that happens?

I take a look into the tracy log of --no-idle myself. I can't find frames where draw_frame does not run. Would you mind pointing it out? Or I misunderstand something?

[crupest]

Emmmm. Do you mean the log without --no-idle, draw_frame is not called in the lost frame, not the --no-idle log?

[fredizzimo]

I mean the log from this message #2102 (comment)

Do you see the big red bars? If you zoom in on those, then you can see that draw frame is not called, even if the loop is run regularly.

On the other hand this one #2102 (comment) looks much better.

[crupest]

I mean the log from this message #2102 (comment)

Do you see the big red bars? If you zoom in on those, then you can see that draw frame is not called, even if the loop is run regularly.

On the other hand this one #2102 (comment) looks much better.

Yeah, now I get what you mean! Debugging now!

[crupest]

@fredizzimo Please help me analyze. I have made following extra log:

The log file and tracy file is in following zip.

new-logs.zip

[crupest]

@fredizzimo I think I know the cause now. Because there is actually NOTHING to redraw. I was keeping acting before with some little stop in between. So the frames are "lost".

nothing.tracy.zip

[fredizzimo]

I'm on the phone and can't check it with the computer right now.

But if you use --no-idle it should always redraw, unless that is broken somehow

[crupest]

Sorry I'm a little hysteric about this result.

To summarize, between the "lost" frames,

1. neovim does not tell you that you need to redraw because you didn't perform any actions
2. the settings are not changed
3. the window does not resize
4. ...

So during these time, prepare_frame does NOT return true to let you call render, which leads to the "lost" frames in tracy.

@fredizzimo I think I know the cause now. Because there is actually NOTHING to redraw. I was keeping acting before with some little stop in between. So the frames are "lost".

nothing.tracy.zip

I did a test on this later. I keep no moving and the frame is lost for a long time as I suppose.

I'm on the phone and can't check it with the computer right now.

But if you use --no-idle it should always redraw, unless that is broken somehow

Actually --no-idle works correctly.

@fredizzimo With --no-idle, tracy.zip

Looks much better as I can see. EDIT: I mean the tracy log.

From this test data, render is called no matter what prepare_frame returns, so it keeps synchronized with vsync, with only few frame lost as the vsync lost.

[fredizzimo]

It looks like everything works correctly then. The fact that it does not draw anything when there's nothing to do is by design to conserve power, especially on laptops. --no-idle, disables that and is mostly there for debugging purposes, performance profiling and and problem situations.

[fredizzimo]

Thank you, I merged this to get more testing.

[crupest]

@fredizzimo Thank you too!

From the beginning of this issue, you have taught me so many with plenty of patience. I really appreciate it. Never had I imaged I can complete this before. This is the first big contribution to one of my favorite projects. And I couldn't make it without your help.

By the way, the implementation of the display link might have some potential bugs since it connects the native platform and the project's rust codes. If anyone runs into a problem on this, please leave a message and I'll be very pleasant to fix them!

Whatever, cheers 🍻!

[fredizzimo]

Thank you, it's been a pleasure to work with you.

Open source is all about working together to achieve something, and a lot of patience is needed, since people usually have other things things in life, so things can take a long time to complete. It's a lot different from regular programming at a company.

This particular issue has been something we have wanted for several months, to make all platforms work roughly equally well when releasing the smooth rendering changes. But due to the lack of macOS developers, and the nature of the task, you have been the only one accepting the challenge, and I really appreciate that.

So again, thank you for this work!