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

Greg/vsync #1950

Closed
wants to merge 2 commits into from
Closed

Greg/vsync #1950

wants to merge 2 commits into from

Conversation

gregory38
Copy link
Contributor

To be discussed because OpenGL tearing/compositor/vsync is a nightmare.

Related to issue #1437

@mirh
Copy link

mirh commented May 21, 2017

Seriously, it's just about nvidia driver giving us problems.
I'm not saying we should flip the bird (I already posted on their dev forums, hoping they can clarify.. just anything)...
but in the meantime for as much I know, you might as well rollout the perfect implementation in the ideal® world (what I think you/I/we had already in mind), plus only:

  • just wait to reintroduce WS_POPUP
  • to the DwmIsCompositionEnabled && window!=monitor check, add && vendorId==nvidia

@tabnk
Copy link

tabnk commented May 22, 2017

Interesting :). Going to test soon ...

image

@tabnk
Copy link

tabnk commented May 22, 2017

illegal else without matching if

D:\torrent\pcsx2\plugins\GSdx\GSWndWGL.cpp 332

void GSWndWGL::SetVSync(int vsync)
{
m_vsync = vsync;
// m_swapinterval uses an integer as parameter
// 0 -> disable vsync
// n -> wait n frame
else if (m_swapinterval && vsync != 0x10000) m_swapinterval(vsync);
}

@gregory38 gregory38 force-pushed the greg/vsync branch 2 times, most recently from a428dd2 to f465e9e Compare May 22, 2017 07:20
@tabnk
Copy link

tabnk commented May 22, 2017

Tested on Windows 10 with Nvidia GPU. Full screen and GDSX in OpenGL. Nvidia vsync setting handle by pcsx2.

Disabled: No tearing but frame pacing.
Adaptive: No tearing and no frame pacing.
Windows Compositor: No tearing but minor/some frame pacing.

Btw, both adaptive and Windows compositor increase input lag

@mirh
Copy link

mirh commented May 22, 2017

Windows Compositor: No tearing but minor/some frame pacing.

Isn't that the same of what was reportedly perfect?

Btw, both adaptive and Windows compositor increase input lag

Mhhh, so my paranoia in #1941 wasn't totally tinfoil 😝
http://mojolabs.nz/posts.php?topic=94676#1088686
Might this make some sense?

EDIT: btw, how do you measure input lag? Just guesswork?

@tabnk
Copy link

tabnk commented May 22, 2017

Game like GT4 with input lag will be easily noticeable.

Copy link
Member

@ssakash ssakash left a comment

Choose a reason for hiding this comment

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

Haven't tested the code yet, just some minor reviews based on a brief look at the code.

default: return 0;
}

return 0;

This comment was marked as spam.

This comment was marked as spam.

m_check_DclickFullscreen = new pxCheckBox( this, _("Double-click toggles fullscreen mode") );
m_check_AspectRatioSwitch = new pxCheckBox(this, _("Switch to 4:3 aspect ratio when an FMV plays"));
//m_check_ExclusiveFS = new pxCheckBox( this, _("Use exclusive fullscreen mode (if available)") );

m_text_Zoom->SetToolTip( pxEt( L"Zoom = 100: Fit the entire image to the window without any cropping.\nAbove/Below 100: Zoom In/Out\n0: Automatic-Zoom-In untill the black-bars are gone (Aspect ratio is kept, some of the image goes out of screen).\n NOTE: Some games draw their own black-bars, which will not be removed with '0'.\n\nKeyboard: CTRL + NUMPAD-PLUS: Zoom-In, CTRL + NUMPAD-MINUS: Zoom-Out, CTRL + NUMPAD-*: Toggle 100/0"
) );

m_check_VsyncEnable->SetToolTip( pxEt( L"Vsync eliminates screen tearing but typically has a big performance hit. It usually only applies to fullscreen mode, and may not work with all GS plugins."
m_combo_vsync->SetToolTip( pxEt( L"Vsync eliminates screen tearing but typically has a big performance hit. It usually only applies to fullscreen mode, and may not work with all GS plugins."

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@@ -967,7 +965,7 @@ EXPORT_C GSsetFrameSkip(int frameskip)

EXPORT_C GSsetVsync(int enabled)
{
s_vsync = !!enabled;
s_vsync = enabled;

This comment was marked as spam.

This comment was marked as spam.

return 0;

if (g_LimiterMode == Limit_Turbo)
return 0;

This comment was marked as spam.

This comment was marked as spam.

@gregory38
Copy link
Contributor Author

gregory38 commented May 25, 2017

https://github.com/PCSX2/pcsx2/blob/master/pcsx2/gui/FrameForGS.cpp#L494

This function uses an extra parameter for flags. Technically it ought to be wx flags but I think MS flags will be forwarded.

@gregory38
Copy link
Contributor Author

@tabnk
I added the hidden option EnableVsyncWindowFlag that must be equivalent to WS_POPUP. Could you test if it is working as expecting.

@tabnk
Copy link

tabnk commented May 29, 2017

For this build, whether EnableVsyncWindowFlag disabled or enabled cannot go full screen.

@gregory38
Copy link
Contributor Author

@tabnk I fixed my code. Could you retry it thanks you.

@tabnk
Copy link

tabnk commented May 30, 2017

With EnableVsyncWindowFlag=enabled at PCSX2_ui.ini. Nvidia setting handle by pcsx2.

vsync disabled: No tearing but frame pacing
vsync standard: No tearing but frame pacing
vsync adaptive: No tearing but less frame pacing
vsync window compositor: No tearing but little frame pacing

Edit by greg with previous result without EnableVsyncWindowFlag

Tested on Windows 10 with Nvidia GPU. Full screen and GDSX in OpenGL. Nvidia vsync setting handle by pcsx2.

Disabled: No tearing but frame pacing.
Adaptive: No tearing and no frame pacing.
Windows Compositor: No tearing but minor/some frame pacing.

Btw, both adaptive and Windows compositor increase input lag

@mirh
Copy link

mirh commented May 30, 2017

Coooooould you test <381 drivers?

@tabnk
Copy link

tabnk commented May 30, 2017

Can't tested in driver 381 below. Already customise per game setting on Nvidia control panel so if install old driver than all my setting will deleted.

@gregory38
Copy link
Contributor Author

@tabnk
Do you feel that new option have any impact ? In the end what is the best configuration ?

@tabnk
Copy link

tabnk commented May 30, 2017

Actually I'm felt almost no different whether EnableVsyncWindowFlag is enabled or disabled.

Depends on individual preference, both Adaptive and Windows Compositor are good.

@mirh
Copy link

mirh commented May 30, 2017

https://forums.geforce.com/default/topic/914268/export-import-nvidia-control-panel-settings-and-color-profiles/#4796732
Also, last time I checked settings were kept unless checking "clean install" option.

@gregory38
Copy link
Contributor Author

Well I'm not sure the flag propagate well to the windows API. Need a windows dev to confirm it.

@tabnk
Copy link

tabnk commented May 30, 2017

Comparing to this

// newStyle |= WS_POPUP;
and your new code.

In full screen mode and disable all vsync, earlier one got tearing but your no tearing. Maybe your doesn't propagate well.

@gregory38
Copy link
Contributor Author

New version that should propagate the flag.

@tabnk
Copy link

tabnk commented Jun 1, 2017

On new version, screen tearing work on full screen.

@mirh
Copy link

mirh commented Jun 1, 2017

Does any of the v-sync options fix it then?

@tabnk
Copy link

tabnk commented Jun 1, 2017

With tearing enable on full screen.

vsync disabled: Tearing and frame pacing
vsync standard: Tearing and some frame pacing
vsync adaptive: No tearing and no noticeable frame pacing. Very good for my GT4.
vsync window compositor: Tearing but maybe some frame pacing

@gregory38
Copy link
Contributor Author

So, I don't know exactly what to do. Here my current proposal

  • Keep the combo box but with 3 states (OFF / ON / adaptive). Please share you opinion on the name. Adaptive is also called "Late Vsync". Therefore keep integer setup on GS side. I need to add a fallback for drivers that don't support it.
  • I removed Vsync from the preset as "Late vsync" should be better but it isn't supported by all drivers. Opinion is welcome.
  • Keep the hidden WS_POPUP option
  • Drop vsync window compositor (or at least don't merge it until we understand fully the behavior and potentially auto detect the fullscreen mode).

@mirh
Copy link

mirh commented Jun 12, 2017

Drivers that don't support adaptive should just not have it presented/available, I guess.
For the remainder, I'm fine.
(assuming this DwmIsCompositionEnabled && window!=monitor check gets added)
Then, nvidia users can even screw for all that I care, considering I have been asking about trying previous drivers and nobody seems to listen.

@gregory38
Copy link
Contributor Author

Drivers that don't support adaptive should just not have it presented/available, I guess.

The thing is that you know about the support when you call the GL swap interval function. And the option is on the core side. Instead we could just fallback to vsync.

 (assuming this DwmIsCompositionEnabled && window!=monitor check gets added)

I propose that we merge the others parts. And then we try to find how to write stuff for DWM.

@mirh
Copy link

mirh commented Jun 13, 2017

The thing is that you know about the support when you call the GL swap interval function. And the option is on the core side. Instead we could just fallback to vsync.

Oh right. Then I guess whatever you said make sense.

I propose that we merge the others parts. And then we try to find how to write stuff for DWM.

Again, I don't think the problem is dwm, and that's also simply what's already used by glfw guys too.
Besides, wasn't all this fuss basically only about the discovery of dwmflush?

gregory38 added 2 commits July 3, 2017 19:22
We have now 4 states
* no
* on
* Compositor handle : DWM on windows/GL
* Dynamic/adaptative : Could be nice tradeoff between perf and quality

So far only the first 2 are implemented in the GS plugin

Dynamic will use the value -1 as it is a direct mapping of the GL api.
Compositor will use the flag 0x10000

Note: potentially the tooltip can be improved

v2: use || instead of multiple if
So far the implementation is really basic but it is enough for testing
@gregory38
Copy link
Contributor Author

Note: I pushed a rebased version. So only the important bits remain.

@lightningterror
Copy link
Contributor

Can this be closed since there is a new improved PR ?

@gregory38
Copy link
Contributor Author

Yes let's close it.

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

Successfully merging this pull request may close these issues.

6 participants