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

[osx] vsync should default to always #4463

Merged
merged 2 commits into from Mar 31, 2014

Conversation

jmarshallnz
Copy link
Contributor

@Montellese I'm not sure if there's a universal way to solve this.

I just copy n pasted the existing win32/android override.

It may also need overriding elsewhere, not sure. @popcornmix rpi maybe?

@popcornmix
Copy link
Member

@jmarshallnz
Copy link
Contributor Author

Heh - I missed that one, yup.

@jmarshallnz
Copy link
Contributor Author

I've updated this with an updater that ignores the incorrect value.

I've also switched the default in settings.xml to VSYNC_ALWAYS. This means that the quirks are around the other way, so in Linux, Android + FreeBSD I've quirked to VSYNC_DRIVER.

This should in theory be identical to what we had before, but be more resilient, as only those platforms that support VSYNC_DRIVER have it defaulted as such.

@jmarshallnz
Copy link
Contributor Author

@Montellese, @t-nelson, please sanity check :)

@Montellese
Copy link
Member

The settings stuff looks fine but I don't know on which platforms VSYNC_DRIVER is available and makes sense.

@davilla
Copy link
Contributor

davilla commented Mar 29, 2014

VSYNC for Android please.

@jmarshallnz
Copy link
Contributor Author

@davilla: Should VSYNC_DRIVER even be an option for vsync on android? (ATM you have it available in the setting as android defines TARGET_POSIX right?)

@popcornmix: I guess the same question could be asked of RPi.

@davilla
Copy link
Contributor

davilla commented Mar 29, 2014

VSYNC_DRIVER is never for Android, never on Arm/Linux. VSYNC_DRIVER seems to be some sort of desktop/linux hack around misbehaving video drivers. So if there is no X11 (maybe wayland too), then no need for it to even show.

@jmarshallnz
Copy link
Contributor Author

OK, I'll ifdef out for android as well in that case (I'm guessing also RPi, but I'll wait on popcornmix).

Pity that everyone still defines TARGET_LINUX :)

@jmarshallnz
Copy link
Contributor Author

@davilla mind taking a look at the ifdef's in 997bb53

@davilla
Copy link
Contributor

davilla commented Mar 29, 2014

Not for at least 24+ hours. About to enter a closed Al tube for 6 hours.

@davilla
Copy link
Contributor

davilla commented Mar 29, 2014

hehe, I took a peek, looks ok.

@jmarshallnz
Copy link
Contributor Author

Thanks :)

@popcornmix
Copy link
Member

VSYNC_DRIVER doesn't make any sense on Pi.
VSYNC should always be enabled really (except maybe for benchmarking).

@jmarshallnz
Copy link
Contributor Author

Thanks - I'll add RPi to the ifdef (by the looks RPi specifies TARGET_LINUX)

Jonathan Marshall added 2 commits March 30, 2014 12:43
…t don't support it. Only show or set on Linux and FreeBSD.
…systems (ALWAYS) and quirk it on Linux and FreeBSD
@jmarshallnz
Copy link
Contributor Author

Ok, removed the option from rpi as well. @popcornmix if you could check the ifdefs here that'd be great.

jenkins build this please

@popcornmix
Copy link
Member

Looks okay to me. VSYNC_DRIVER has gone from list, and Always Enabled is the default.

jmarshallnz added a commit that referenced this pull request Mar 31, 2014
[osx] vsync should default to always
@jmarshallnz jmarshallnz merged commit 889c88f into xbmc:master Mar 31, 2014
@jmarshallnz
Copy link
Contributor Author

thanks :)

@jmarshallnz jmarshallnz deleted the osx_vsync_always branch March 31, 2014 04:59
jmarshallnz added a commit that referenced this pull request Mar 31, 2014
[osx] vsync should default to always
@MartijnKaijser MartijnKaijser modified the milestones: Helix 14.0-alpha1, Gotham13.0-rc1 May 21, 2014
@MartijnKaijser MartijnKaijser removed this from the Gotham13.0-rc1 milestone May 21, 2014
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.

None yet

5 participants