-
-
Notifications
You must be signed in to change notification settings - Fork 22.3k
Add CameraFeed support for Android #106094
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
base: master
Are you sure you want to change the base?
Conversation
1e343e1
to
7804756
Compare
#98416 (comment) Were these comments also taken into account in this PR?
I think we can increase |
From the following chart, it looks like we're removing support for 1.3% Android devices by bumping the min version to 24: https://composables.com/android-distribution-chart There may be value in bumping the min sdk version, but it should not be done in this PR as it would require discussions and alignments.
|
if (rotation == Surface.ROTATION_90) { | ||
return 270; | ||
} else if (rotation == Surface.ROTATION_180) { | ||
return 180; | ||
} else if (rotation == Surface.ROTATION_270) { | ||
return 90; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason 270
and 90
are inverted?
@@ -306,6 +309,19 @@ public int getScreenOrientation() { | |||
} | |||
} | |||
|
|||
public int getDisplayRotation() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you validate the issue raised in #98709 for the use of windowManager.getDefaultDisplay().getRotation()
doesn't apply here as well.
Since the Camera2 API is part of the Android NDK, targeting API levels below 24 will result in compile errors if you attempt to link against it directly, as the symbols are only available from API level 24. A simple runtime API level check won't prevent these compile-time issues. Are you planning to use:
Which approach are you considering? |
@shiena I didn't realize it was only introduced in the NDK since API level 24 as described in https://developer.android.com/ndk/guides/stable_apis#camera
There are workarounds described in https://developer.android.com/ndk/guides/using-newer-apis, but I'm not sure the added complexity cost is warranted. cc @syntaxerror247 @Alex2782 @akien-mga We may need to bump the min version to 24 if we want a simple integration of this feature.
On the negative side, this removes support for 1.3% Android device.. though Android 7 (api level 24) is now 9 years old. My preference would be to make the bump to api version 24, primarily to ensure vulkan is available on all api versions we support. |
I´ve seen other PRs using #defines, so I recommend using that for now, and they can be removed once the change to level 24 is approved. That should also fix the failing test. |
No, I wouldn’t request any further work from the author until we decide whether or not to bump the api version. |
We could consider using API guards https://developer.android.com/ndk/guides/using-newer-apis#avoiding_repetition_of_api_guards. It might take some effort, but it's probably the best alternative to raising the min-sdk. That said, I’m also in favor of bumping it to 24, just a bit hesitant given that it would drop support for about 1.3% of devices. |
It's something that should be done in a separate PR, but yeah I think it's probably time now to bump the min SDK to 24, at least by default. Maybe we can preserve support for 21 via a few defines and a SCons argument, so that users who really want to support the oldest devices, and don't care about 24+ APIs like CameraFeed, could still compile their own templates. If that doesn't make things too complex. |
A Scons argument sounds good if someone still needs it. We discontinued Android 5 and 6 support for our (non-Godot) app two years ago since they accounted for just 0.5% of users on the Google Play Store back then. (Active installations of our app) |
I have created #106148 to bump the min supported version. Let's continue the discussion for that topic in that PR. |
Separated and reorganized the Android implementation from #98416.
Updated the minimum target API level from 21 to 24 because camera2 NDK requires Android API level 24 or higher.
Additionally, updated AndroidManifest.xml to enable camera usage in the Godot editor for Android.