Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Refactor the EGL implementation selection code in EGLWrapper. #2464

Merged
merged 1 commit into from

5 participants

@smspillaz

Templatize some of the more repetitive initialize-check logic so that
we can just try and create every individual type, see if creation succeeded
and then return true if it did.

InitializeWrapper ensures that the window system gets initialized after
the function returns.

I haven't had the opportunity to test this manually, as I don't have the tools to compile on android, rpi or the like at the moment.

@smspillaz smspillaz closed this
@smspillaz smspillaz reopened this
@theuni
Owner

Mind explaining why?

The current code is a butt-load of copy/paste, but very much on purpose to improve readability and debugging. It's not exactly a performance-critical section. Your change is certainly elegant, but I'm not sure it's necessary.

@smspillaz
@whaupt

"(This is also why I have guess->Initialize () in a destructor because of the risk of new throwing an exception, having it in a destructor makes it exception safe)"

I would suggest to prevent any exceptions from leaving your destructor as exceptions in destructors may result in undefined behavior and you will not know that guess->Initialize() guarantees to be exception free.

@smspillaz
@theuni
Owner

Fair enough. My first instinct was to use simple macros, hence the comment about the ability to debug, but you've gone to the other end of the spectrum here, so that argument doesn't hold.

I suppose my main hesitation is that I'm not that familiar with boost, so it makes me nervous, as the primary maintainer of this code, to be adding complexity for very little (imo) gain.

That said, ignorance is hardly a reason to deny a change, so I'll read-up and review once you've updated for the issue pointed out above.

@theuni
Owner
The best way in which readability can be improved is probably just to
improve those function names (I'm sure they're not 100% right).

Mind elaborating?

@smspillaz
@theuni
Owner

Header-only boost is ok around here, but c++11 is a no-go.

@akva2

noncopyable is extremely little code to write. seems like it's very easy to shortcut this entire discussion but writing the two lines of code yourself ;)

@smspillaz
@theuni
Owner

Yes, and for the sake of not blocking future ports. As well as maintining support for older and more exotic toolchains. But regardless, it's definitely not worth the dependency for some cosmetics ;)

I'm happy to discuss the reasons further or irc/forum if you'd like, but let's not continue spamming the other devs here.

@smspillaz

Okay, updated based on the current commentary.

I guess CreateEGLNativeType is probably the best name for that function.

@smspillaz

ping?

@theuni
Owner

Well if we're going for reducing duplication here, why not send in a nativetype pointer to be alloc'd by the creator, then return a bool on success? Then you can drop that InitializeNativeType helper and early returns, and it'd look like:

  CEGLNativeType *nativeGuess = NULL;
  if (CreateEGLNativeType< CEGLNativeTypeAndroid >(implementation, nativeGuess) ||
      CreateEGLNativeType< CEGLNativeTypeAmlogic >(implementation, nativeGuess) ||
      CreateEGLNativeType< CEGLNativeTypeRaspberryPI >(implementation, nativeGuess))
  {
    m_nativeTypes = nativeGuess;
    m_nativeTypes->Initialize();
  }
  return m_nativeTypes != NULL;

@theuni
Owner

If you agree with the above, I'm ok merging the change for the April window. Seems like a nice solution for the ugliness of factory creation.

xbmc/windowing/egl/EGLWrapper.cpp
((15 lines not shown))
{
- if(implementation == nativeGuess->GetNativeName() || implementation == "auto")
+ if(guess->CheckCompatibility())
@theuni Owner
theuni added a note

guess && guess->CheckCompatibility

Unlikely, but doesn't hurt to be safe.

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

Yep, even better.

@smspillaz

Alrighty, done :)

@theuni
Owner

Looks good. Please squash it down to 1 commit (just squash and force-push to your branch) and I'll queue it for April.

@smspillaz smspillaz Templatize some of the more repetitive initialize-check logic so that
we can just try and create every individual type, see if creation succeeded
and then return true if it did.
1634066
@smspillaz

Done, thanks

@theuni theuni was assigned
@MartijnKaijser MartijnKaijser merged commit 1ecee80 into xbmc:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 25, 2013
  1. @smspillaz

    Templatize some of the more repetitive initialize-check logic so that

    smspillaz authored
    we can just try and create every individual type, see if creation succeeded
    and then return true if it did.
This page is out of date. Refresh to see the latest.
Showing with 35 additions and 33 deletions.
  1. +35 −33 xbmc/windowing/egl/EGLWrapper.cpp
View
68 xbmc/windowing/egl/EGLWrapper.cpp
@@ -41,53 +41,55 @@ CEGLWrapper::~CEGLWrapper()
Destroy();
}
-bool CEGLWrapper::Initialize(const std::string &implementation)
+namespace
{
- bool ret = false;
- CEGLNativeType *nativeGuess = NULL;
-
- nativeGuess = new CEGLNativeTypeAndroid;
- if (nativeGuess->CheckCompatibility())
+ bool
+ CorrectGuess(CEGLNativeType *guess,
+ const std::string &implementation)
{
- if(implementation == nativeGuess->GetNativeName() || implementation == "auto")
- {
- m_nativeTypes = nativeGuess;
- ret = true;
- }
- }
+ assert(guess != NULL);
- if (!ret)
- {
- delete nativeGuess;
- nativeGuess = new CEGLNativeTypeAmlogic;
- if (nativeGuess->CheckCompatibility())
+ if(guess->CheckCompatibility())
{
- if(implementation == nativeGuess->GetNativeName() || implementation == "auto")
+ if (implementation == guess->GetNativeName() ||
+ implementation == "auto")
{
- m_nativeTypes = nativeGuess;
- ret = true;
+ return true;
}
}
+
+ return false;
}
- if (!ret)
+ template <class NativeType>
+ CEGLNativeType * CreateEGLNativeType(const std::string &implementation)
{
- delete nativeGuess;
- nativeGuess = new CEGLNativeTypeRaspberryPI;
- if (nativeGuess->CheckCompatibility())
- {
- if(implementation == nativeGuess->GetNativeName() || implementation == "auto")
- {
- m_nativeTypes = nativeGuess;
- ret = true;
- }
- }
+ CEGLNativeType *guess = new NativeType;
+ if(CorrectGuess(guess, implementation))
+ return guess;
+
+ delete guess;
+ return NULL;
}
+}
+
+bool CEGLWrapper::Initialize(const std::string &implementation)
+{
+ CEGLNativeType *nativeGuess = NULL;
+
+ // Try to create each backend in sequence and go with the first one
+ // that we know will work
+ if ((nativeGuess = CreateEGLNativeType<CEGLNativeTypeAndroid>(implementation)) ||
+ (nativeGuess = CreateEGLNativeType<CEGLNativeTypeAmlogic>(implementation)) ||
+ (nativeGuess = CreateEGLNativeType<CEGLNativeTypeRaspberryPI>(implementation)))
+ {
+ m_nativeTypes = nativeGuess;
- if (ret && m_nativeTypes)
m_nativeTypes->Initialize();
+ return true;
+ }
- return ret;
+ return false;
}
bool CEGLWrapper::Destroy()
Something went wrong with that request. Please try again.