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

Various fixes #15633

Merged
merged 3 commits into from Mar 1, 2019

Conversation

Projects
None yet
5 participants
@notspiff
Copy link
Contributor

commented Feb 28, 2019

Description

  • missing initialization
  • missing thread safety
  • log spam
@@ -287,7 +287,7 @@ bool CWinSystemX11GLContext::RefreshGLContext(bool force)
m_vaapiProxy.reset(X11::VaapiProxyCreate());
X11::VaapiProxyConfig(m_vaapiProxy.get(), GetDisplay(),
static_cast<CGLContextEGL*>(m_pGLContext)->m_eglDisplay);
bool general, deepColor;
bool general=false, deepColor;

This comment has been minimized.

Copy link
@pkerling

pkerling Feb 28, 2019

Member

Why do you initialize only one of them?

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 28, 2019

Author Contributor

the other is only used if general is true, which means it was set. so it isn't necessary. i can do it if you prefer it for OCD reasons or whatever ;)

This comment has been minimized.

Copy link
@Rechi

Rechi Feb 28, 2019

Member

You have to look into the function to know that. Also in the future it may change.

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 28, 2019

Author Contributor

done.

This comment has been minimized.

Copy link
@pkerling

pkerling Feb 28, 2019

Member

To be honest this doesn't make much sense to me here :/ VAAPIRegisterRender always initializes general to false, as it can be expected to. This confusion is one of the reasons I detest reference out parameters in general though.

This comment has been minimized.

Copy link
@notspiff

notspiff Feb 28, 2019

Author Contributor

it does not if you have no vaapi renderer available.

This comment has been minimized.

Copy link
@pkerling

pkerling Feb 28, 2019

Member

Acknowledged. Putting the variable initialization there would make just as much sense I guess, so fine by me.

@notspiff notspiff force-pushed the notspiff:various_fixes branch from dff21b2 to 2c7b5ba Feb 28, 2019

@notspiff notspiff force-pushed the notspiff:various_fixes branch from 2c7b5ba to 995685c Feb 28, 2019

@@ -30,6 +30,8 @@ class CSlideShowPic
TRANSITION_EFFECT type;
int start;
int length;

TRANSITION() : type(TRANSITION_NONE), start(0), length(0) {}

This comment has been minimized.

Copy link
@garbear

garbear Feb 28, 2019

Member

Not default initialization in the fields above?

@@ -287,7 +287,7 @@ bool CWinSystemX11GLContext::RefreshGLContext(bool force)
m_vaapiProxy.reset(X11::VaapiProxyCreate());
X11::VaapiProxyConfig(m_vaapiProxy.get(), GetDisplay(),
static_cast<CGLContextEGL*>(m_pGLContext)->m_eglDisplay);
bool general, deepColor;
bool general = false, deepColor = false;

This comment has been minimized.

Copy link
@garbear

garbear Feb 28, 2019

Member

Opportunity to split definitions to different lines, if you're so inclined.

@notspiff

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

i'm this close to dropping this pr. come on people!

notspiff added some commits Feb 27, 2019

various uninited variable usage
according to valgrind.
fixed: put add-on instance creation/destruction in a critical section
if not, unloading/loading of so files leads to calls to
no longer existent function pointers.

@notspiff notspiff force-pushed the notspiff:various_fixes branch from 995685c to 21c5ac5 Mar 1, 2019

@pkerling
Copy link
Member

left a comment

Sorry for the trouble

@MartijnKaijser MartijnKaijser merged commit 30de82e into xbmc:master Mar 1, 2019

1 check passed

default You're awesome. Have a cookie
Details

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Mar 1, 2019

@notspiff notspiff deleted the notspiff:various_fixes branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.