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

FIX: [droid] handle audio headsets #7345

Merged
merged 1 commit into from Oct 14, 2015

Conversation

@koying
Copy link
Contributor

koying commented Jun 27, 2015

Properly only enable PCM stereo when a headset (bluetooth or wired) is connected

/cc @anssih @Montellese @MartijnKaijser

if (action == "android.intent.action.HEADSET_PLUG")
newstate = (intent.getIntExtra("state", 0) != 0);
else
newstate = (intent.getIntExtra("android.bluetooth.profile.extra.STATE", 0) == 2);

This comment has been minimized.

Copy link
@Montellese

Montellese Jun 28, 2015

Member

Could we turn the 2 here into a constant with a readable name so that it's more obvious?

Not part of this PR but in general it would probably be nice to turn all those intent action strings into constants as well because they are used in multiple locations which makes the chance for typos higher.

This comment has been minimized.

Copy link
@koying

koying Aug 16, 2015

Author Contributor

Could we turn the 2 here into a constant with a readable name so that it's more obvious?

Not really, as the values are completely dependent on this specific intent. I can definitely add a comment, though ;)

bool CJNIAudioManager::isBluetoothA2dpOn()
{
return call_method<jboolean>(m_object,
"isBluetoothA2dpOn",

This comment has been minimized.

Copy link
@Montellese

Montellese Jun 28, 2015

Member

Odd indentation here (same in isWiredHeadsetOn(). Shouldn't it be moved to the same column as the m_object one line above?

@@ -35,6 +35,7 @@ class CJNIIntent : public CJNIBase
std::string getType() const ;

int getIntExtra(const std::string &name, int defaultValue) const;
std::string getStringExtra(const std::string &name) const;

This comment has been minimized.

Copy link
@Montellese

Montellese Jun 28, 2015

Member

Where is this used?

This comment has been minimized.

Copy link
@koying

koying Aug 16, 2015

Author Contributor

I thought that I'd need it, but finally not.
When this happen in the jni library, I tend to leave it there because the end goal is to have it as complete as possible, so any enrichment is a step in the right direction, even if not actually currently used, and removing it is wasted work that might have to be redone in the future

@Montellese

This comment has been minimized.

Copy link
Member

Montellese commented Jun 28, 2015

I haven't checked the Android API but the changes look sane. Do we really need the bluetooth permission just to check it's state (i.e. without interacting with it)?

@anssih

This comment has been minimized.

Copy link
Member

anssih commented Jun 28, 2015

There is also a "android.bluetooth.headset.profile.action.CONNECTION_STATE_CHANGED" broadcast intent, do we need to handle that as well or is "android.bluetooth.a2dp.profile.action.CONNECTION_STATE_CHANGED" enough, or is it something totally diferent?

@Montellese Querying is OK without bluetooth permission, but according to the docs listening to android.bluetooth.*.CONNECTION_STATE_CHANGED is not.

@koying koying added the v16 Jarvis label Jul 5, 2015
@koying

This comment has been minimized.

Copy link
Contributor Author

koying commented Aug 16, 2015

@anssih IIRC, "android.bluetooth.headset.profile.action.CONNECTION_STATE_CHANGED" is for mono headsets+mic (i.e. for actual phone conversations), which is out-of-scope of Kodi as far as I'm concerned.

@koying koying force-pushed the koying:fixdroidheadsets branch from 73f978e to a3ed4cf Aug 16, 2015
@koying

This comment has been minimized.

Copy link
Contributor Author

koying commented Aug 16, 2015

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the Jarvis 16.0-alpha3 milestone Aug 27, 2015
@mkortstiege

This comment has been minimized.

Copy link
Member

mkortstiege commented Sep 12, 2015

@anssih, @Montellese what's the status here?

koying added a commit that referenced this pull request Oct 14, 2015
FIX: [droid] handle audio headsets
@koying koying merged commit fa0964e into xbmc:master Oct 14, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@koying koying deleted the koying:fixdroidheadsets branch Oct 14, 2015
@MartijnKaijser

This comment has been minimized.

Copy link
Member

MartijnKaijser commented on a3ed4cf Oct 20, 2015

@koying think we missed adding
uses-feature android:name="android.hardware.bluetooth" android:required="false"

This comment has been minimized.

Copy link
Contributor Author

koying replied Oct 20, 2015

Yep, indeed. Good catch.

This comment has been minimized.

Copy link
Member

MartijnKaijser replied Oct 20, 2015

This comment has been minimized.

Copy link
Contributor Author

koying replied Oct 20, 2015

Thanks

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