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

Support for Android API 25 #5736

Merged
merged 18 commits into from
Mar 11, 2018

Conversation

HalfdanJ
Copy link
Member

This PR is adding support for Android API 24. I had to add code requesting permissions for camera when build on more recent sdk.

Adding this allows support of ARCore for example, this requires a minSdk level of 24.

I've tested multiple apps on phones ranging from API level 19-26, all working.

image

@HalfdanJ HalfdanJ requested a review from arturoc August 30, 2017 19:29
@arturoc
Copy link
Member

arturoc commented Aug 31, 2017

looks good to me. perhaps the checkCameraPermission functions could be made more generic? it seems a little arbitrary to only have a permission check for that particular permission and seems like just passing the permission enum as a parameter it would work for anything no?

@HalfdanJ
Copy link
Member Author

True, the enum is just a Java string, and sending that from c++ requires a lot of code i think, so felt this was cleaner code

@HalfdanJ HalfdanJ requested a review from danzeeeman August 31, 2017 18:18
Dan Moore added 3 commits February 8, 2018 13:56
implmenting permission request and check for all permissions
bumping compileSdkVersion to include permissions as [this medium post](https://medium.com/google-developers/picking-your-compilesdkversion-minsdkversion-targetsdkversion-a098a0341ebd) says we can do it without bumping minSdkVersion
Copy link
Member

@danzeeeman danzeeeman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@HalfdanJ
Copy link
Member Author

HalfdanJ commented Mar 4, 2018

This solution seems to work across all api levels from 19. I've tested in on firebase test lab that runs the camera example on physical devices on all api levels.
image

@arturoc , your comments should have been handled by dans commits.

}

ndk {
platformVersion = "19"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found out this is the trick to keep NDK compiling against 19, while bumping java to 25.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found solution here: android/ndk#126 (comment)

@HalfdanJ HalfdanJ changed the title Support for Android API 24 Support for Android API 25 Mar 4, 2018
@HalfdanJ
Copy link
Member Author

HalfdanJ commented Mar 4, 2018

Still passing all device tests
image and travis. So should be ready to merge. @arturoc

Copy link
Member

@arturoc arturoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good except for the couple of things with the permissions enum, if you can change that i'll merge it

@@ -78,6 +108,8 @@ void ofxAndroidUnlockScreenSleep();
bool ofxAndroidIsOnline();
bool ofxAndroidIsWifiOnline();
bool ofxAndroidIsMobileOnline();
void ofxAndroidRequestPermission(int permission);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change this to accept an ofxAndroidPermission instead of int?

@@ -11,8 +11,38 @@
#include "ofConstants.h"
#include "ofEvent.h"



enum ANDROID_PERMISSION{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ofxAndroidPermission since that's the standard we use also the individual enumerations should be OFX_ANDROID_PERMISSION_READ_CALENDAR...

@@ -54,6 +58,105 @@ private static String getPackageName(){
return OFAndroidLifeCycle.getActivity().getPackageName();
}

public static String parsePermission(int permission){
String PERMISSION = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this variable as all capitals in java is weird no?

public static String parsePermission(int permission){
String PERMISSION = "";
switch(permission){
case 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having just numbers here looks very error prone can't we just get this value through jni's getStaticField(ID) instead of having the switch case here?

@danzeeeman
Copy link
Member

yeah you can make these changes @arturoc

@HalfdanJ
Copy link
Member Author

I gave it a pass at your comments @arturoc . I didn't use getStaticField, since the string values are the same as the constant names, so it seems like overkill. But I moved the string logic from java to c++ so there isn't any relying on the integer values.

@HalfdanJ
Copy link
Member Author

Test fails on pulling the arturo profile picture :) Seems like the forum is down?

[  error ] ofImage: loadImage(): couldn't load image from ofBuffer, unable to guess image format from memory
[  error ] ofImage: loadImage(): couldn't load image from ""https://forum.openframeworks.cc/user_avatar/forum.openframeworks.cc/arturo/45/3965_1.png""

@arturoc
Copy link
Member

arturoc commented Mar 11, 2018

thanks so much jonas, that looks great. i broke the forum yesterday trying to update it, it's already working on a new server but we still have to change the dns entry. as soon as that's ready i'll restart the tests and merge this

@arturoc arturoc merged commit 7978597 into openframeworks:master Mar 11, 2018
@nishimotz
Copy link

androidAudioExample raises error as follows and audio input is not working.

04-09 19:09:33.299   203 27094 W ServiceManager: Permission failure: android.permission.RECORD_AUDIO from uid=10197 pid=27011
04-09 19:09:33.299   203 27094 E         : Request requires android.permission.RECORD_AUDIO
04-09 19:09:33.299   203 27094 E AudioFlinger: openRecord() permission denied: recording not allowed
04-09 19:09:33.300 27011 27038 E AudioRecord: AudioFlinger could not create record track, status: -1
04-09 19:09:33.302   203 27046 I AudioFlinger: AudioFlinger's thread 0xb2600000 ready to run
04-09 19:09:33.306 27011 27038 E AudioRecord-JNI: Error creating AudioRecord instance: initialization check failed with status -1.
04-09 19:09:33.306 27011 27038 E android.media.AudioRecord: Error code -20 when initializing native AudioRecord object.
04-09 19:09:33.306 27011 27038 I OF      : sound input setup with buffersize: 3552
04-09 19:09:33.309 27011 27038 I ofAppAndroidWindow: resize 1080x1776
04-09 19:09:33.310 27011 27047 I OF      : no audio input

@danzeeeman
Copy link
Member

danzeeeman commented Apr 10, 2018 via email

@danzeeeman
Copy link
Member

danzeeeman commented Apr 10, 2018 via email

@arturoc
Copy link
Member

arturoc commented Apr 10, 2018

I just checked and the permissions seem to be ok, perhaps this is an issue with the PG overwriting the manifest or something? @nishimotz can you open a new issue? i doubt this is related to the update to android 25

@danzeeeman
Copy link
Member

danzeeeman commented Apr 10, 2018 via email

@HalfdanJ
Copy link
Member Author

Right, it's still needed on the manifest file, but we need to call the request permission function call somewhere since recording audio is probably a "dangerous permission". We should probably do it on the audio recorder code like we do with the video input.

@danzeeeman
Copy link
Member

danzeeeman commented Apr 10, 2018 via email

@HalfdanJ
Copy link
Member Author

No the manifest is still needed. That is what indicates what might be asked (for example shown on play store).

@danzeeeman
Copy link
Member

danzeeeman commented Apr 10, 2018 via email

@nishimotz
Copy link

@arturoc As reported in #5963, I doubt this is permission problem as well. ofSoundStream seems broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants