[droid] External Player Support #2059

Merged
merged 1 commit into from Jan 25, 2013

Conversation

Projects
None yet
5 participants

Enable external player support on android. Player filename is the package name (ie. com.some.player) of the Android application.

davilla was assigned Jan 10, 2013

@Montellese Montellese commented on an outdated diff Jan 10, 2013

xbmc/android/activity/XBMCApp.cpp
@@ -730,6 +730,96 @@ bool CXBMCApp::HasLaunchIntent(const string &package)
return true;
}
+bool CXBMCApp::StartActivityWithExtra(const string &package, const string &intent, const string &dataType, const string &dataURI)
+{
+ if (!m_activity || !package.size() || !intent.size())
+ return false;
+
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity package: '%s'", package.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity intent: '%s'", intent.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity dataType: '%s'", dataType.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity dataURI: '%s'", dataURI.c_str());
@Montellese

Montellese Jan 10, 2013

Owner

Can't these go on one line? Furthermore the method is called StartActivityWithExtra so using "StartActivity" in the debug output could be confusing because there's also a StartActivity() method.

@Montellese Montellese and 1 other commented on an outdated diff Jan 10, 2013

xbmc/android/activity/XBMCApp.cpp
@@ -730,6 +730,96 @@ bool CXBMCApp::HasLaunchIntent(const string &package)
return true;
}
+bool CXBMCApp::StartActivityWithExtra(const string &package, const string &intent, const string &dataType, const string &dataURI)
+{
+ if (!m_activity || !package.size() || !intent.size())
+ return false;
+
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity package: '%s'", package.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity intent: '%s'", intent.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity dataType: '%s'", dataType.c_str());
+ CLog::Log(LOGDEBUG, "CXBMCApp::StartActivity dataURI: '%s'", dataURI.c_str());
+
+ jthrowable exc;
+ JNIEnv *env = NULL;
+ AttachCurrentThread(&env);
+ jobject oActivity = m_activity->clazz;
+ jclass cActivity = env->GetObjectClass(oActivity);
@Montellese

Montellese Jan 10, 2013

Owner

Please move this all the way down right before where you use it. We only have a limited amount of JNI references available and therefore we need to create them as late as possible and destroy them as early as possible.

PS : I assume you copied this from the StartActivity() method so maybe you can change it there as well if you got time.

@davilla

davilla Jan 10, 2013

Contributor

if should be CXBMCApp::StartActivityWithExtra not CXBMCApp::StartActivity.

@Montellese Montellese commented on an outdated diff Jan 10, 2013

xbmc/android/activity/XBMCApp.cpp
+ env->DeleteLocalRef(oUri);
+ }
+ env->DeleteLocalRef(cIntent);
+ env->DeleteLocalRef(sPackage);
+
+ // Java equivalent for the following JNI
+ // startActivity(oIntent);
+ jmethodID mStartActivity = env->GetMethodID(cActivity, "startActivity", "(Landroid/content/Intent;)V");
+ env->CallVoidMethod(oActivity, mStartActivity, oIntent);
+ env->DeleteLocalRef(cActivity);
+ env->DeleteLocalRef(oIntent);
+
+ exc = env->ExceptionOccurred();
+ if (exc)
+ {
+ CLog::Log(LOGERROR, "CXBMCApp::StartActivity Failed to load %s. Exception follows:", package.c_str());
@Montellese

Montellese Jan 10, 2013

Owner

Same as above, please use "StartActivityWithExtras".

Owner

Montellese commented Jan 10, 2013

Apart from the few nitpicks I mentioned, this looks good to me. Maybe StartActivityWithExtras() and StartActivity() could be merged into one method StartActivity() to save the code duplication? All the extra params of StartActivityWithExtras() could have a default value (empty string), in that case it should behave the same as StartActivity() right?

Can't say much about the external player support as I don't know much about it. I'm guessing it probably won't work for some streams and network protocols that xbmc supports but the android player might not? There's a discussion about external player support on android in http://forum.xbmc.org/showthread.php?tid=138464 so maybe check for any gotchas in there as well.

@Montellese I like the default of passing an empty string and combining the two methods, I'll be starting on this soon.

In terms of external players supporting the various streams/proto's there are some gotchas. I am working on putting together a compatibility matrix with some users on xbmcandroid.com/forums which I plan on publishing. However, the playercorefactory.xml rules can be used to work around some of the limits of external players from what I can tell.

@theuni I can take a stab at de-duping the code. Wouldn't mind seeing this through to the end.

Member

theuni commented Jan 11, 2013

@Montellese lol, seems we think exactly alike. The github link from mail sent me to the commits and not the PR, so I commented there without seeing your comments. We said the exact same things ;)

@theuni, @Montellese, @davilla : I am still working on the re-factor. Ran into some JNI failures and need to look closer at it. I am hesitant to commit broken code, but I can if you would like to take a closer look at what I have done thus far.

Contributor

davilla commented Jan 15, 2013

we can wait for you to resolve your issues.

Latest commits include a re-factor to combine the StartActivity methods into a single method with default parameters

Contributor

davilla commented Jan 23, 2013

@theuni, @Montellese, I'm ok with this going in.

Contributor

davilla commented Jan 23, 2013

squash down please.

Owner

Montellese commented Jan 23, 2013

Fine by me for Frodo+1.

Contributor

davilla commented Jan 24, 2013

Member

jmarshallnz commented Jan 24, 2013

Fine with me

davilla merged commit 12af4b4 into xbmc:master Jan 25, 2013

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