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

Greg/vsync take2 #2000

Merged
merged 5 commits into from Aug 5, 2017

Conversation

Projects
None yet
@gregory38
Contributor

gregory38 commented Jul 4, 2017

Replace PR #1950

This PR factorize some code in the OpenGL window API to allow adaptive Vsync. And automatically detect DWM crazyness. It misses a GUI update to add adaptive Vsync. Plan is to adapt commit of the previous PR. Meanwhile people can test the various driver behavior.

@gregory38 gregory38 added this to the Release 1.6 milestone Jul 4, 2017

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Jul 4, 2017

Member

Maybe @ssakash can do something about the GUI.

Member

lightningterror commented Jul 4, 2017

Maybe @ssakash can do something about the GUI.

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jul 4, 2017

Member

Nah, no worries. @gregory38 is way more competent than me when it comes to coding at all aspects. ;)

Member

ssakash commented Jul 4, 2017

Nah, no worries. @gregory38 is way more competent than me when it comes to coding at all aspects. ;)

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Jul 4, 2017

Member

He doesn't use Windows tho.

Embrace it.
https://www.youtube.com/watch?v=G6b5ubvfa3I

Member

lightningterror commented Jul 4, 2017

He doesn't use Windows tho.

Embrace it.
https://www.youtube.com/watch?v=G6b5ubvfa3I

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jul 5, 2017

Member

@gregory38

Don't hesitate to ping me if you need any help on Windows side.

Member

ssakash commented Jul 5, 2017

@gregory38

Don't hesitate to ping me if you need any help on Windows side.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 5, 2017

GSWndWGL::CompositorEnabled

I guess that.. in light of the remainder checks, the function could be named CompositorAffected, CompositorHandled, UnderCompositor or something?
I mean, I know there are other emus that are lurking at these PRs.. And perhaps the whole rationale is not entirely clear as in a 100 comments discussion.

if (!dwm)

A XP fallback 😆

if (GLLoader::nvidia_buggy_driver)

Cool naming standardization there.
I would suggest to add a TODO: figure out specifically affected circumstances. maybe?
Is v-sync still enforced by driver even on W7 with dwm off?
Are driver versions below 380.xx branch affected?

GSVector4i surface = GetClientRect();

You could mention above this line that this should make for the amd/intel case instead.


Anyway, I tested the thing
And something fishy is going on, because everytime I toggle fullscreen mode (be it exclusive or non exclusive!) unlimited framerate get clamped to 60FPS.
Even if I have v-sync disabled.

Maybe you should check surface**!=**window for truth?
Funnily, I was now thinking that since this comparison implicitly also takes into account the window border (which is necessarily 0 only when in fullscreen afaik?) we might as well not need the monitor size check.

mirh commented Jul 5, 2017

GSWndWGL::CompositorEnabled

I guess that.. in light of the remainder checks, the function could be named CompositorAffected, CompositorHandled, UnderCompositor or something?
I mean, I know there are other emus that are lurking at these PRs.. And perhaps the whole rationale is not entirely clear as in a 100 comments discussion.

if (!dwm)

A XP fallback 😆

if (GLLoader::nvidia_buggy_driver)

Cool naming standardization there.
I would suggest to add a TODO: figure out specifically affected circumstances. maybe?
Is v-sync still enforced by driver even on W7 with dwm off?
Are driver versions below 380.xx branch affected?

GSVector4i surface = GetClientRect();

You could mention above this line that this should make for the amd/intel case instead.


Anyway, I tested the thing
And something fishy is going on, because everytime I toggle fullscreen mode (be it exclusive or non exclusive!) unlimited framerate get clamped to 60FPS.
Even if I have v-sync disabled.

Maybe you should check surface**!=**window for truth?
Funnily, I was now thinking that since this comparison implicitly also takes into account the window border (which is necessarily 0 only when in fullscreen afaik?) we might as well not need the monitor size check.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 5, 2017

Contributor

@lightningterror @ssakash
Well as I said, there is already some code for wx on the other PR. I just need to tune it. And to take a decision on "string" translation and preset behavior. However I need to some help to test & debug this code. I didn't test anything on it. My current driver doesn't support adaptive Vsync and ofc I'm on Linux.

@mirh
Hum yes you're right condition is bad. And yes I will add some comments.

Contributor

gregory38 commented Jul 5, 2017

@lightningterror @ssakash
Well as I said, there is already some code for wx on the other PR. I just need to tune it. And to take a decision on "string" translation and preset behavior. However I need to some help to test & debug this code. I didn't test anything on it. My current driver doesn't support adaptive Vsync and ofc I'm on Linux.

@mirh
Hum yes you're right condition is bad. And yes I will add some comments.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 5, 2017

My current driver doesn't support adaptive Vsync

Ehrm.. What is GLX_MESA_swap_control for ?

EDIT:
besides after a year of wonderings, I found these tools to check windows styles.
We have an Overlapped window, 16CF0000-00000100 when windowed and 16030000-00000000 in fullscreen.
Which I guess is whatever here, minus WS_POPUP (and plus the possibly irrelevant WS_GROUP and WS_TABSTOP?)
Thing is, exclusive or non exclusive fullscreen didn't make a difference.
...
So, guess like I have to found some idea to detect that.
EDIT3: aside of numbers, the reminder interpretation is wrong

EDIT2: this can even force styles!
Anyway, my educate guess on exclusive/non-exclusive thing is that we could check for GetTopWindow and GetActiveWindow?
Not sure if both, just one or either. For example, I guess in a multi-monitor scenario the two could very much differ.
EDIT3: check if SPI_GETWORKAREA called from a fullscreen app returns monitor size?

mirh commented Jul 5, 2017

My current driver doesn't support adaptive Vsync

Ehrm.. What is GLX_MESA_swap_control for ?

EDIT:
besides after a year of wonderings, I found these tools to check windows styles.
We have an Overlapped window, 16CF0000-00000100 when windowed and 16030000-00000000 in fullscreen.
Which I guess is whatever here, minus WS_POPUP (and plus the possibly irrelevant WS_GROUP and WS_TABSTOP?)
Thing is, exclusive or non exclusive fullscreen didn't make a difference.
...
So, guess like I have to found some idea to detect that.
EDIT3: aside of numbers, the reminder interpretation is wrong

EDIT2: this can even force styles!
Anyway, my educate guess on exclusive/non-exclusive thing is that we could check for GetTopWindow and GetActiveWindow?
Not sure if both, just one or either. For example, I guess in a multi-monitor scenario the two could very much differ.
EDIT3: check if SPI_GETWORKAREA called from a fullscreen app returns monitor size?

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 6, 2017

BOOL dwm;

Wonder: isn't bool initialized to a random value this way?
That in the event of DwmIsCompositionEnabled failing, would make for !dwm check (aka is not 0?) meaningless?

Recent driver (+380.xx) seem to never bypass the compositor.

Actually, as I was saying I'm unsure.
Recent driver (+380.xx ??) seem to always get v-sync enforced someway. would seem more accurate.

At least not with current wxWindow setup.

Mhh, dunno. My educate guess over nvidia dev forums, judge this more spread than just wx, or any special case we may be falling under.

if (CompositorEnabled() && m_vsync != 0)

I'm not sure we can drop dwmflush from the "I don't want v-sync" use-case - if we still want to avoid stuttering.
But since I'm no eagle, I'll let other judge on this.

Anyway, for some reason I'm never getting v-sync to begin with (albeit it was kind of emotional to finally see tearing).

...

if (CompositorEnabled()) SetSwapInterval(0)

What happens to the CompositorEnabled function when none of the paths you placed are hit?
[remember me why we are not just using good old if 1 OR 2 OR 3 plus else?]

mirh commented Jul 6, 2017

BOOL dwm;

Wonder: isn't bool initialized to a random value this way?
That in the event of DwmIsCompositionEnabled failing, would make for !dwm check (aka is not 0?) meaningless?

Recent driver (+380.xx) seem to never bypass the compositor.

Actually, as I was saying I'm unsure.
Recent driver (+380.xx ??) seem to always get v-sync enforced someway. would seem more accurate.

At least not with current wxWindow setup.

Mhh, dunno. My educate guess over nvidia dev forums, judge this more spread than just wx, or any special case we may be falling under.

if (CompositorEnabled() && m_vsync != 0)

I'm not sure we can drop dwmflush from the "I don't want v-sync" use-case - if we still want to avoid stuttering.
But since I'm no eagle, I'll let other judge on this.

Anyway, for some reason I'm never getting v-sync to begin with (albeit it was kind of emotional to finally see tearing).

...

if (CompositorEnabled()) SetSwapInterval(0)

What happens to the CompositorEnabled function when none of the paths you placed are hit?
[remember me why we are not just using good old if 1 OR 2 OR 3 plus else?]

@ssakash

This comment has been minimized.

Show comment
Hide comment
@ssakash

ssakash Jul 6, 2017

Member

Wonder: isn't bool initialized to a random value this way?
That in the event of DwmIsCompositionEnabled failing, would make for !dwm check (aka is not 0?) meaningless?

No, dwm will be initialized after the call to DwmIsCompositionEnabled().

Member

ssakash commented Jul 6, 2017

Wonder: isn't bool initialized to a random value this way?
That in the event of DwmIsCompositionEnabled failing, would make for !dwm check (aka is not 0?) meaningless?

No, dwm will be initialized after the call to DwmIsCompositionEnabled().

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 7, 2017

Oh, I see thank you. Now, if that's good then:

// Only possible on Win7

Should maybe have a .

Then, I was thinking: if we are always performing the check, we really don't need WM_DWMCOMPOSITIONCHANGED.
But then.. Perhaps isn't a bit too much rude/heavy to indeed ALWAYS check the thing at every Flip?

mirh commented Jul 7, 2017

Oh, I see thank you. Now, if that's good then:

// Only possible on Win7

Should maybe have a .

Then, I was thinking: if we are always performing the check, we really don't need WM_DWMCOMPOSITIONCHANGED.
But then.. Perhaps isn't a bit too much rude/heavy to indeed ALWAYS check the thing at every Flip?

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 12, 2017

Contributor

@mirh it would be nice to comment in the code directly

I'm not sure we can drop dwmflush from the "I don't want v-sync" use-case - if we still want to avoid stuttering.

The question is what is the behavior of no-vsync + no dwm wait. Are you locked to 60fps but with stuttering or can you go lower/higher ?

Contributor

gregory38 commented Jul 12, 2017

@mirh it would be nice to comment in the code directly

I'm not sure we can drop dwmflush from the "I don't want v-sync" use-case - if we still want to avoid stuttering.

The question is what is the behavior of no-vsync + no dwm wait. Are you locked to 60fps but with stuttering or can you go lower/higher ?

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 12, 2017

Contributor

Then, I was thinking: if we are always performing the check, we really don't need WM_DWMCOMPOSITIONCHANGED.
But then.. Perhaps isn't a bit too much rude/heavy to indeed ALWAYS check the thing at every Flip?

I'm not sure the event will be generated if we resize the surface. 60 check by seconds seems to be free vs MFLOPS/GFLOPS of a CPU.

Contributor

gregory38 commented Jul 12, 2017

Then, I was thinking: if we are always performing the check, we really don't need WM_DWMCOMPOSITIONCHANGED.
But then.. Perhaps isn't a bit too much rude/heavy to indeed ALWAYS check the thing at every Flip?

I'm not sure the event will be generated if we resize the surface. 60 check by seconds seems to be free vs MFLOPS/GFLOPS of a CPU.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 12, 2017

The question is what is the behavior of no-vsync + no dwm wait.

It's not even a question, because if you think to it.. That's literally whatever we always have now by default.

60 check by seconds seems to be free vs MFLOPS/GFLOPS of a CPU.

Not sure I have understood?

mirh commented Jul 12, 2017

The question is what is the behavior of no-vsync + no dwm wait.

It's not even a question, because if you think to it.. That's literally whatever we always have now by default.

60 check by seconds seems to be free vs MFLOPS/GFLOPS of a CPU.

Not sure I have understood?

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 14, 2017

Contributor

The question was in case the dwm compositor is still enabled. For example in not-full screen mode. Are we still locked at 60 fps with stuttering. Or can we go higher ? lower?

CPUs are capable of millions of operations per seconds. A couple of instructions every vsync is peanut

Contributor

gregory38 commented Jul 14, 2017

The question was in case the dwm compositor is still enabled. For example in not-full screen mode. Are we still locked at 60 fps with stuttering. Or can we go higher ? lower?

CPUs are capable of millions of operations per seconds. A couple of instructions every vsync is peanut

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 14, 2017

Contributor

@ssakash @mirh I need help for the option description. What do you think of

	m_combo_vsync->SetToolTip( pxEt( L"Vsync eliminates screen tearing but typically has a big performance hit. Adaptive Vsync is a trade-off between screen tearing and performance. Vsync usually only applies to fullscreen mode, and may not work with all GS/renderer plugins."
Contributor

gregory38 commented Jul 14, 2017

@ssakash @mirh I need help for the option description. What do you think of

	m_combo_vsync->SetToolTip( pxEt( L"Vsync eliminates screen tearing but typically has a big performance hit. Adaptive Vsync is a trade-off between screen tearing and performance. Vsync usually only applies to fullscreen mode, and may not work with all GS/renderer plugins."
@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 14, 2017

The question was in case the dwm compositor is still enabled.

Of course. I'm never assuming it to be disabled.

Are we still locked at 60 fps with stuttering. Or can we go higher ? lower?

As I was saying this is basically what we have now, with all that entails.

For example in not-full screen mode.

And given what's the majority of our good-willing testers.. Basically in every test not-of-mine you can 100% expect this not to matter.

CPUs are capable of millions of operations per seconds. A couple of instructions every vsync is peanut

Mhh.. ok, but still why not somewhere else?

Also, v-sync being enabled or not.. If you are affected by compositor, it's not like you aren't still in need of dwmflush.

Vsync usually only applies to fullscreen mode, and may not work with all GS/renderer plugins.

Mhh, what about:
V-sync is usually being already performed by OS compositor and shouldn't require additional explicit enforcement.

Then, I guess we could even mention which use-cases trigger what.. But I'm not sure the lengthy explanation would be worth the scariness of the wall of text.

mirh commented Jul 14, 2017

The question was in case the dwm compositor is still enabled.

Of course. I'm never assuming it to be disabled.

Are we still locked at 60 fps with stuttering. Or can we go higher ? lower?

As I was saying this is basically what we have now, with all that entails.

For example in not-full screen mode.

And given what's the majority of our good-willing testers.. Basically in every test not-of-mine you can 100% expect this not to matter.

CPUs are capable of millions of operations per seconds. A couple of instructions every vsync is peanut

Mhh.. ok, but still why not somewhere else?

Also, v-sync being enabled or not.. If you are affected by compositor, it's not like you aren't still in need of dwmflush.

Vsync usually only applies to fullscreen mode, and may not work with all GS/renderer plugins.

Mhh, what about:
V-sync is usually being already performed by OS compositor and shouldn't require additional explicit enforcement.

Then, I guess we could even mention which use-cases trigger what.. But I'm not sure the lengthy explanation would be worth the scariness of the wall of text.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 14, 2017

Contributor

V-sync is usually being already performed by OS compositor and shouldn't require additional explicit enforcement.

The string is intended for the user. And I'm pretty sure people won't understand it correctly. Because it depends on the OS/driver/...

Contributor

gregory38 commented Jul 14, 2017

V-sync is usually being already performed by OS compositor and shouldn't require additional explicit enforcement.

The string is intended for the user. And I'm pretty sure people won't understand it correctly. Because it depends on the OS/driver/...

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 14, 2017

Contributor

Besides, we need to explain the adaptive vsync .... However I think we can remove the part about the "GS supported stuff". We have only a single GS plugin nowadays.

Contributor

gregory38 commented Jul 14, 2017

Besides, we need to explain the adaptive vsync .... However I think we can remove the part about the "GS supported stuff". We have only a single GS plugin nowadays.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 14, 2017

And I'm pretty sure people won't understand it correctly. Because it depends on the OS/driver/

Yees.
So we either describe everything in detail in the tooltip, or we just keep vague and tell user: trust us, we passed through all it.
From his side he should just have to ask himself: do I have the kind of mind/monitor combo that I don't care about tearing? Or not?
IMO.

we need to explain the adaptive vsync

Fine point, but first - I'm not even sure the downsides are real to begin with.
Too bad there's nobody on this planet capable to test that /irony-not-irony

mirh commented Jul 14, 2017

And I'm pretty sure people won't understand it correctly. Because it depends on the OS/driver/

Yees.
So we either describe everything in detail in the tooltip, or we just keep vague and tell user: trust us, we passed through all it.
From his side he should just have to ask himself: do I have the kind of mind/monitor combo that I don't care about tearing? Or not?
IMO.

we need to explain the adaptive vsync

Fine point, but first - I'm not even sure the downsides are real to begin with.
Too bad there's nobody on this planet capable to test that /irony-not-irony

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 16, 2017

Contributor

Ok. But in this case, I think it would be better to directly state that option is only available when the OS compositor doesn't do vsync. In you statement, some people might think the option override the OS/driver setting. What do you think ?

Contributor

gregory38 commented Jul 16, 2017

Ok. But in this case, I think it would be better to directly state that option is only available when the OS compositor doesn't do vsync. In you statement, some people might think the option override the OS/driver setting. What do you think ?

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 16, 2017

Mhh
I think that you should look at it from user side, not dev "whatever it takes to do it" one.
In other words: if ticking that option doesn't do anything, but user still is cool, that's the most important thing.

Then... I guess like we should provide some concise explanation (to avoid unpleasant misunderstandings like "why v-sync is unticked but I still have it"?)

Vertical synchronization is a [method/protocol/technique/something] that locks frames displaying to monitor pixels refresh rate in order to avoid tearing. This may lead to stuttering when framerate is subpar.
Adaptive (half-tick) trades this absolute clamp to allow any range of performance.

or

Adaptive trades this absolute clamp to allow any range of performance. and will be used if supported.

Then here stuff gets tricky and I dunno if whatever I say should be considered GL only or not.

Most of times* though applications don't need to [do anything for/explicitly enable any of] this, since OS compositor will already handle it (at the cost of just a frame of latency).
The setting here tries to be smart enough to recognize such cases and will avoid double-applying wrecks.

*SOME exceptions being [at least] amd and intel drivers in opengl fullscreen mode

mirh commented Jul 16, 2017

Mhh
I think that you should look at it from user side, not dev "whatever it takes to do it" one.
In other words: if ticking that option doesn't do anything, but user still is cool, that's the most important thing.

Then... I guess like we should provide some concise explanation (to avoid unpleasant misunderstandings like "why v-sync is unticked but I still have it"?)

Vertical synchronization is a [method/protocol/technique/something] that locks frames displaying to monitor pixels refresh rate in order to avoid tearing. This may lead to stuttering when framerate is subpar.
Adaptive (half-tick) trades this absolute clamp to allow any range of performance.

or

Adaptive trades this absolute clamp to allow any range of performance. and will be used if supported.

Then here stuff gets tricky and I dunno if whatever I say should be considered GL only or not.

Most of times* though applications don't need to [do anything for/explicitly enable any of] this, since OS compositor will already handle it (at the cost of just a frame of latency).
The setting here tries to be smart enough to recognize such cases and will avoid double-applying wrecks.

*SOME exceptions being [at least] amd and intel drivers in opengl fullscreen mode

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 17, 2017

Contributor

The things is lot of people report me that

  • I disabled vsync but fps is still 60 fps
  • I enabled vsync but I can still see tearing (hopefully this one will be fixed)

Thinking about it again, I think people can google vsync and such. So let's do something easy. What do you think about that (You can tune it).

Most of times, window compositor already handle Vsync. If you suffer of screen tearing, you can still enable it here at performance/stuttering cost.

Contributor

gregory38 commented Jul 17, 2017

The things is lot of people report me that

  • I disabled vsync but fps is still 60 fps
  • I enabled vsync but I can still see tearing (hopefully this one will be fixed)

Thinking about it again, I think people can google vsync and such. So let's do something easy. What do you think about that (You can tune it).

Most of times, window compositor already handle Vsync. If you suffer of screen tearing, you can still enable it here at performance/stuttering cost.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 17, 2017

I disabled vsync but fps is still 60 fps

Unless we are talking about frame limiting, I think to have been the only one reporting that.
And it was about dwmflush, if any, not something else.

In this case, I think triple buffering would be a godsend.

I enabled vsync but I can still see tearing

Where ever, except this?

Thinking about it again, I think people can google vsync and such.

Yes, I think they could - but I don't think they should. Tooltips are exactly for that ultimately.

Then, I don't like giving this half-assed impression "we don't have any clue about problems, experiment".
Not because I value reputation or something, but because actually we do know the great part of the situation.
In the end as I was saying, user should just have to ask himself what side of the personal trade-off he is (that's what user settings are for after all, right?)

Ideally the perfect tooltip should manage to suggest this dilemma.

Vertical synchronization is a GPU ↔ monitor frames locking mechanism used to avoid tearing.
When not already performed by the OS (e.g. window compositor is disabled or graphics drivers is forcing a different fullscreen behavior) pcsx2 will use this setting.
-Triple buffering note- ?
[adaptive note]

Plus, figure out what variable refresh rate entails I guess.

mirh commented Jul 17, 2017

I disabled vsync but fps is still 60 fps

Unless we are talking about frame limiting, I think to have been the only one reporting that.
And it was about dwmflush, if any, not something else.

In this case, I think triple buffering would be a godsend.

I enabled vsync but I can still see tearing

Where ever, except this?

Thinking about it again, I think people can google vsync and such.

Yes, I think they could - but I don't think they should. Tooltips are exactly for that ultimately.

Then, I don't like giving this half-assed impression "we don't have any clue about problems, experiment".
Not because I value reputation or something, but because actually we do know the great part of the situation.
In the end as I was saying, user should just have to ask himself what side of the personal trade-off he is (that's what user settings are for after all, right?)

Ideally the perfect tooltip should manage to suggest this dilemma.

Vertical synchronization is a GPU ↔ monitor frames locking mechanism used to avoid tearing.
When not already performed by the OS (e.g. window compositor is disabled or graphics drivers is forcing a different fullscreen behavior) pcsx2 will use this setting.
-Triple buffering note- ?
[adaptive note]

Plus, figure out what variable refresh rate entails I guess.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 17, 2017

Contributor

Please let's keep 3 buffering outside of the topic. 3B creates others issues.

In the end as I was saying, user should just have to ask himself what side of the personal trade-off he is (that's what user settings are for after all, right?)

Yes. We still need something easy enough to understand. I'm afraid the translator might change the sense a bit. It is quite a complex topic. Anyway I like you latest proposal.

What do you think for adaptive.
Adaptive -> Adaptive Vsync is a relaxed Vsync to reduce, but not remove, tearing with a smaller speed penalty.

Contributor

gregory38 commented Jul 17, 2017

Please let's keep 3 buffering outside of the topic. 3B creates others issues.

In the end as I was saying, user should just have to ask himself what side of the personal trade-off he is (that's what user settings are for after all, right?)

Yes. We still need something easy enough to understand. I'm afraid the translator might change the sense a bit. It is quite a complex topic. Anyway I like you latest proposal.

What do you think for adaptive.
Adaptive -> Adaptive Vsync is a relaxed Vsync to reduce, but not remove, tearing with a smaller speed penalty.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 17, 2017

I'm afraid the translator might change the sense a bit. It is quite a complex topic

Good point. Guess like we could put a "dev-only" comment (explaining long and wide) aside of the string?

Adaptive Vsync is a relaxed Vsync to reduce, but not remove, tearing with a smaller speed penalty

Yes and no. Since wording would all depend on the previous or not 3B note.
Which, I see what you are saying, but [waiting for a fixed built to test the conjecture] I'm afraid it might be needed to avoid hard-locking on dwmflush.

mirh commented Jul 17, 2017

I'm afraid the translator might change the sense a bit. It is quite a complex topic

Good point. Guess like we could put a "dev-only" comment (explaining long and wide) aside of the string?

Adaptive Vsync is a relaxed Vsync to reduce, but not remove, tearing with a smaller speed penalty

Yes and no. Since wording would all depend on the previous or not 3B note.
Which, I see what you are saying, but [waiting for a fixed built to test the conjecture] I'm afraid it might be needed to avoid hard-locking on dwmflush.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 17, 2017

Contributor

If you don't want to wait on dwmflush, the only good solution is to remove the compositor. 3B will bring us various issue such as extra latency. People should invest in 120Hz/144Hz display.

Contributor

gregory38 commented Jul 17, 2017

If you don't want to wait on dwmflush, the only good solution is to remove the compositor. 3B will bring us various issue such as extra latency. People should invest in 120Hz/144Hz display.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 17, 2017

If you don't want to wait on dwmflush, the only good solution is to remove the compositor. 3B will bring us various issue such as extra latency

Putting aside that I'm still not sure tbh (a fixed build would be welcome).. how would 3B affect latency?
I wasn't saying to do triple-buffered v-sync. Just the former thing to decouple frame rendering from presentation.

People should invest in 120Hz/144Hz display.

Yes, but we want perfection 🙃

mirh commented Jul 17, 2017

If you don't want to wait on dwmflush, the only good solution is to remove the compositor. 3B will bring us various issue such as extra latency

Putting aside that I'm still not sure tbh (a fixed build would be welcome).. how would 3B affect latency?
I wasn't saying to do triple-buffered v-sync. Just the former thing to decouple frame rendering from presentation.

People should invest in 120Hz/144Hz display.

Yes, but we want perfection 🙃

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Jul 31, 2017

Mhh.. guess like kudos for the "one thing at time" thought.

And unbelievably, I do am getting sync now. No BS.
It's a bit of a pity you have to close and re-open the rendering window for the setting to apply though?
Also, are "safe presets" expected to gray out v-sync?

mirh commented Jul 31, 2017

Mhh.. guess like kudos for the "one thing at time" thought.

And unbelievably, I do am getting sync now. No BS.
It's a bit of a pity you have to close and re-open the rendering window for the setting to apply though?
Also, are "safe presets" expected to gray out v-sync?

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Jul 31, 2017

Member

It's a bit of a pity you have to close and re-open the rendering window for the setting to apply though?

It probably requires the gs plugin to be restarted. Changing core gs /emulation settings doesn't do that if I'm correct.

Also, are "safe presets" expected to gray out v-sync?

It shouldn't. Any kind of preset actually grays it out. It's the same on master.

Member

lightningterror commented Jul 31, 2017

It's a bit of a pity you have to close and re-open the rendering window for the setting to apply though?

It probably requires the gs plugin to be restarted. Changing core gs /emulation settings doesn't do that if I'm correct.

Also, are "safe presets" expected to gray out v-sync?

It shouldn't. Any kind of preset actually grays it out. It's the same on master.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 31, 2017

Contributor
It probably requires the gs plugin to be restarted. Changing core gs /emulation settings doesn't do that if I'm correct.

It is the issue detected by turtleli. OpenGL is per-context, so you can't update vsync from the main thread. Once fixed, it will be easier to test Vsync.

It shouldn't. Any kind of preset actually grays it out. It's the same on master.

Sorry. What do you mean ? It should grays it out or not ?

Contributor

gregory38 commented Jul 31, 2017

It probably requires the gs plugin to be restarted. Changing core gs /emulation settings doesn't do that if I'm correct.

It is the issue detected by turtleli. OpenGL is per-context, so you can't update vsync from the main thread. Once fixed, it will be easier to test Vsync.

It shouldn't. Any kind of preset actually grays it out. It's the same on master.

Sorry. What do you mean ? It should grays it out or not ?

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Jul 31, 2017

Member

image

This , I don't see why it should gray it out , since it's vsync and not really a hack or anything.It's the same on the other presets as well.

edit: On a side note other options like frame skipping , clamping also are grayed out even on preset 6 but that's another issue.

Member

lightningterror commented Jul 31, 2017

image

This , I don't see why it should gray it out , since it's vsync and not really a hack or anything.It's the same on the other presets as well.

edit: On a side note other options like frame skipping , clamping also are grayed out even on preset 6 but that's another issue.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Jul 31, 2017

Contributor

I think it is as master. We could discuss about it. Search on github there is a related (maybe closed) issue on it.

Contributor

gregory38 commented Jul 31, 2017

I think it is as master. We could discuss about it. Search on github there is a related (maybe closed) issue on it.

@lightningterror

This comment has been minimized.

Show comment
Hide comment
@lightningterror

lightningterror Jul 31, 2017

Member

Found it #1642
Anyway it's not really an issue , more like for safety and doesn't really matter. Forget what I said 🤓

Btw seems that travis build didn't compile properly since git had issues earlier today.

Member

lightningterror commented Jul 31, 2017

Found it #1642
Anyway it's not really an issue , more like for safety and doesn't really matter. Forget what I said 🤓

Btw seems that travis build didn't compile properly since git had issues earlier today.

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Jul 31, 2017

Member

Actually the "close/resume emulation before vsync" is a different issue from what I detected. It was also reported in #1971 - "Vsync setting on linux has no effect until application restart" - which if you think about it is probably the same thing. I don't think we detect if the vsync setting has been changed while emulation is active, so if someone only changes the vsync setting but doesn't touch the hotkeys (let's pretend it's fixed or they're using d3dwhatever) then there'll be no change to the vsync setting until emulation is paused/resumed. So that's one more thing that probably needs fixed at some point.

Member

turtleli commented Jul 31, 2017

Actually the "close/resume emulation before vsync" is a different issue from what I detected. It was also reported in #1971 - "Vsync setting on linux has no effect until application restart" - which if you think about it is probably the same thing. I don't think we detect if the vsync setting has been changed while emulation is active, so if someone only changes the vsync setting but doesn't touch the hotkeys (let's pretend it's fixed or they're using d3dwhatever) then there'll be no change to the vsync setting until emulation is paused/resumed. So that's one more thing that probably needs fixed at some point.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 1, 2017

Contributor

You're right. Actually I got this behavior in my quick test of this PR (with a dirty hotkey hack fixes). But does it work on Windows ? Vsync setting is only set during opening and hotkey. Maybe we should just call GSsetVsync after any setting change.

Contributor

gregory38 commented Aug 1, 2017

You're right. Actually I got this behavior in my quick test of this PR (with a dirty hotkey hack fixes). But does it work on Windows ? Vsync setting is only set during opening and hotkey. Maybe we should just call GSsetVsync after any setting change.

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Aug 1, 2017

Member

But does it work on Windows ?

No, I don't think it does, I was just quoting from the linked issue.

Member

turtleli commented Aug 1, 2017

But does it work on Windows ?

No, I don't think it does, I was just quoting from the linked issue.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 1, 2017

Contributor

Ok.

So how do we proceed ? Merge this PR and let's you do the fix of the threading sync issue ? Or the reverse way ?

Contributor

gregory38 commented Aug 1, 2017

Ok.

So how do we proceed ? Merge this PR and let's you do the fix of the threading sync issue ? Or the reverse way ?

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Aug 1, 2017

This , I don't see why it should gray it out , since it's vsync and not really a hack or anything.It's the same on the other presets as well.

Thinking better, v-sync might lead to some kind of problem depending on whatever weird monitor refresh rate you may have, so I guess it may have sense to gray it out.

So, there's just the "window has to be restarted" problem imo.
Which.. I dunno, if it isn't all that complex could be merged here, given the PR is pretty lightweight at this point.

mirh commented Aug 1, 2017

This , I don't see why it should gray it out , since it's vsync and not really a hack or anything.It's the same on the other presets as well.

Thinking better, v-sync might lead to some kind of problem depending on whatever weird monitor refresh rate you may have, so I guess it may have sense to gray it out.

So, there's just the "window has to be restarted" problem imo.
Which.. I dunno, if it isn't all that complex could be merged here, given the PR is pretty lightweight at this point.

@turtleli

This comment has been minimized.

Show comment
Hide comment
@turtleli

turtleli Aug 2, 2017

Member

So how do we proceed ? Merge this PR and let's you do the fix of the threading sync issue ? Or the reverse way ?

Dunno. I think either way is fine. I'm not sure the compositor stubs should be in the commits since the DWM stuff has been removed for now.

Member

turtleli commented Aug 2, 2017

So how do we proceed ? Merge this PR and let's you do the fix of the threading sync issue ? Or the reverse way ?

Dunno. I think either way is fine. I'm not sure the compositor stubs should be in the commits since the DWM stuff has been removed for now.

gregory38 added some commits Jul 3, 2017

gsdx ogl: factorize SetVSync
Move common logic into base class
Add API to handle late Vsync (only stub)
gsdx ogl: test adaptive/late vsync driver support
Supported on GLX (but not Linux free driver) and WGL
core: add a getter for the vsync option
v2: allow all combinations of framelimiter and vsync options
v3:
* disable vsync when the user disable framelimiter with F4
* Use g_Conf->EmuOptions instead of EmuConfig
pcsx2 gui: uses a combo box for Vsync
The possible values are
* Disabled
* Enabled
* Adaptive Vsync (the new possibility)

@gregory38 gregory38 merged commit 41bfb6e into master Aug 5, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 5, 2017

Contributor

Ok. I merged the first part. I let you handle the set vsyc part.

Contributor

gregory38 commented Aug 5, 2017

Ok. I merged the first part. I let you handle the set vsyc part.

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Aug 5, 2017

Cool.
Now, aside of that, 3 other things to tackle:

  • can we get any objective "preference"/precedence between adaptive and standard vsync?
  • how to neatly integrate dwmflush?
  • adjust v-sync label

Interestingly here, even on a 60Hz screen, PAL framerate has no problems to be maintained with v-sync.

mirh commented Aug 5, 2017

Cool.
Now, aside of that, 3 other things to tackle:

  • can we get any objective "preference"/precedence between adaptive and standard vsync?
  • how to neatly integrate dwmflush?
  • adjust v-sync label

Interestingly here, even on a 60Hz screen, PAL framerate has no problems to be maintained with v-sync.

@NEOAethyr

This comment has been minimized.

Show comment
Hide comment
@NEOAethyr

NEOAethyr Aug 5, 2017

Well that's lame, disablng vsync with the frame limiter.
Maybe I have a 100-144hz monitor and I don't want a ton of tearing when I'm leveling up with no frame limiter...
VSync needs to be a separate option.

NEOAethyr commented on e863613 Aug 5, 2017

Well that's lame, disablng vsync with the frame limiter.
Maybe I have a 100-144hz monitor and I don't want a ton of tearing when I'm leveling up with no frame limiter...
VSync needs to be a separate option.

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 5, 2017

Contributor

Maybe, but if you cpu isn't fast enough, you will get back to 50-70 fps. The number of people with a powerful system + high speed monitor is close of 0. Current behavior is better default behavior (and it is actually the old behavior).

However, vsync will only be disabled if you use the F4 shortcut. If you disable the framelimiter in the option directly, it will keep you're vsync option.

Contributor

gregory38 replied Aug 5, 2017

Maybe, but if you cpu isn't fast enough, you will get back to 50-70 fps. The number of people with a powerful system + high speed monitor is close of 0. Current behavior is better default behavior (and it is actually the old behavior).

However, vsync will only be disabled if you use the F4 shortcut. If you disable the framelimiter in the option directly, it will keep you're vsync option.

This comment has been minimized.

Show comment
Hide comment
@MrCK1

MrCK1 Aug 5, 2017

Member

@gregory38 Is that functionality noted in a tooltip somewhere? We don't want a false bug report of VSync toggling when it isn't supposed to be, ect.

Member

MrCK1 replied Aug 5, 2017

@gregory38 Is that functionality noted in a tooltip somewhere? We don't want a false bug report of VSync toggling when it isn't supposed to be, ect.

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 6, 2017

Contributor

I don't know but it is the same behavior since ages. And by definition, Vsync is a frame limiter.

Contributor

gregory38 replied Aug 6, 2017

I don't know but it is the same behavior since ages. And by definition, Vsync is a frame limiter.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 6, 2017

Contributor

@mirh
It depends on the machine. And it is a tradeoff between frame latency / tearing / stuttering / frame drop / frame pacing.

  • Adaptive + no frame limiter will give you better compromise (except maybe frame latency)
  • If you can't stand tearing => full vsync but you will get frame drop
Contributor

gregory38 commented Aug 6, 2017

@mirh
It depends on the machine. And it is a tradeoff between frame latency / tearing / stuttering / frame drop / frame pacing.

  • Adaptive + no frame limiter will give you better compromise (except maybe frame latency)
  • If you can't stand tearing => full vsync but you will get frame drop
@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Aug 6, 2017

Well that's lame, disablng vsync with the frame limiter.

.-.
Is it really it?
I thought it was smart driver doing sorcery.

It depends on the machine.

Yes, and that's why any assumption with arbitrary values (like the alleged one above) should not ship.
Especially with no mention to the end-user. It just makes for extreme confusion.

mirh commented Aug 6, 2017

Well that's lame, disablng vsync with the frame limiter.

.-.
Is it really it?
I thought it was smart driver doing sorcery.

It depends on the machine.

Yes, and that's why any assumption with arbitrary values (like the alleged one above) should not ship.
Especially with no mention to the end-user. It just makes for extreme confusion.

@DonelBueno

This comment has been minimized.

Show comment
Hide comment
@DonelBueno

DonelBueno Aug 6, 2017

I've tested the latest build and enabling vsync (full or adaptative) still caps the framerate to my monitor's refresh rate even when I hit F4 while in-game.

DonelBueno commented Aug 6, 2017

I've tested the latest build and enabling vsync (full or adaptative) still caps the framerate to my monitor's refresh rate even when I hit F4 while in-game.

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 6, 2017

Contributor

@DonelBueno yeah. It isn't the purpose of this PR. It will be fixed soon.

Contributor

gregory38 commented Aug 6, 2017

@DonelBueno yeah. It isn't the purpose of this PR. It will be fixed soon.

@DonelBueno

This comment has been minimized.

Show comment
Hide comment
@DonelBueno

DonelBueno Aug 6, 2017

Oh, got it. Is it part of the "set vsync part" you mentioned?

DonelBueno commented Aug 6, 2017

Oh, got it. Is it part of the "set vsync part" you mentioned?

@gregory38

This comment has been minimized.

Show comment
Hide comment
@gregory38

gregory38 Aug 6, 2017

Contributor

yep

Contributor

gregory38 commented Aug 6, 2017

yep

@mirh

This comment has been minimized.

Show comment
Hide comment
@mirh

mirh Aug 6, 2017

I'm starting to go crazy.
I thought the set vsync part was about not having to close and reopen the window for vsync setting to apply?

mirh commented Aug 6, 2017

I'm starting to go crazy.
I thought the set vsync part was about not having to close and reopen the window for vsync setting to apply?

@gregory38 gregory38 deleted the greg/vsync-take2 branch Oct 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment