Skip to content

Commit

Permalink
[gles] fixed, m_iVSyncMode var in two places with different type, opp…
Browse files Browse the repository at this point in the history
…s. pick one, please
  • Loading branch information
davilla committed Feb 27, 2013
1 parent 5dc1030 commit 8d2e0fe
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 4 deletions.
9 changes: 6 additions & 3 deletions xbmc/windowing/egl/WinSystemEGL.cpp
Expand Up @@ -43,7 +43,7 @@ CWinSystemEGL::CWinSystemEGL() : CWinSystemBase()
m_config = NULL;

m_egl = NULL;
m_iVSyncMode = false;
m_iVSyncMode = 0;
}

CWinSystemEGL::~CWinSystemEGL()
Expand Down Expand Up @@ -384,9 +384,12 @@ bool CWinSystemEGL::PresentRenderImpl(const CDirtyRegionList &dirty)

void CWinSystemEGL::SetVSyncImpl(bool enable)
{
m_iVSyncMode = enable;
if (!m_egl->SetVSync(m_display, m_iVSyncMode))
m_iVSyncMode = enable ? 10:0;

This comment has been minimized.

Copy link
@jmarshallnz

jmarshallnz Feb 27, 2013

Contributor

Is 10 intentional?

This comment has been minimized.

Copy link
@theuni

theuni Feb 27, 2013

Contributor

No, that's testing code from a long time ago. Should of course be 0/1.

But EGL does allow for this to be set to higher than 1, it's basically a vblank timing divisor.

This comment has been minimized.

Copy link
@davilla

davilla Feb 27, 2013

Author Contributor

darwin uses 10, it comes from x11 stuff, I'm not sure wtf 10 means anymore. It was needed with darwin when I changes osx and added ios, so I did the same here until someone figures out what these magic things really means and creates a proper enum for them.

This comment has been minimized.

Copy link
@davilla

davilla Feb 27, 2013

Author Contributor

@theuni , not here, in the actual m_egl->SetVSync code which only checks the value of the bool and sets the sync to 0 or 1. That 0 or 1 is the vblank timing divisor, for example 2 is also valid.... on some platforms.

This comment has been minimized.

Copy link
@popcornmix

popcornmix Feb 27, 2013

Member

Could we make m_iVSyncMode default to enabled?
Currently the "let driver choose" option for vsync leaves it disabled which seems inappropriate for any EGL platform, which are generally low powered devices that won't want to render faster than vsync.

This comment has been minimized.

Copy link
@davilla

davilla Feb 27, 2013

Author Contributor

pivos does that in the packaging steps. we hate ifdefs :)

This comment has been minimized.

Copy link
@popcornmix

popcornmix Feb 27, 2013

Member

But does it need an ifdef? Does anyone want it to be disabled?

This comment has been minimized.

Copy link
@davilla

davilla Feb 28, 2013

Author Contributor

maybe windows breaks if vsync is enabled ? who know, you are asking for a platform change here, so it needs to be checked and verified on all platforms that it might effect.

This comment has been minimized.

Copy link
@theuni

theuni Feb 28, 2013

Contributor

Looks like each renderer has its own definition of vsyncmode. In the case of gles, it appears as though we only test whether it's non-null or not. Am I missing something else?

if (!m_egl->SetVSync(m_display, enable))
{
m_iVSyncMode = 0;
CLog::Log(LOGERROR, "%s,Could not set egl vsync", __FUNCTION__);
}
}

void CWinSystemEGL::ShowOSMouse(bool show)
Expand Down
1 change: 0 additions & 1 deletion xbmc/windowing/egl/WinSystemEGL.h
Expand Up @@ -75,7 +75,6 @@ class CWinSystemEGL : public CWinSystemBase, public CRenderSystemGLES
EGLConfig m_config;

CEGLWrapper *m_egl;
bool m_iVSyncMode;
std::string m_extensions;
};

Expand Down

0 comments on commit 8d2e0fe

Please sign in to comment.