-
Notifications
You must be signed in to change notification settings - Fork 64
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
WIP: Android meson integration #26
Conversation
CC: @johnstultz-work |
Hello Kieran, Sorry for creating this PR in your personal repository. Hope you don't mind we do some preliminary review here until it will be ready for submission via mailing list. |
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.
It's a bit unfortunate not to be able to use the standard library - but I guess that's a limitation on Android.
And given that this is an 'android hal' it's probably not unreasonable to highglight that as an issue.
We use the HAL in ChromeOS where we can utilise the std::filesystem.
I wonder if it can be wrapped in some way so we can take advantage of the std::filesystem when it's there or not otherwise.
Also you've droped a check, you're no longer validating that the file is there before opening it. I guess the difference is subtle as the fopen will likely fail anyway, but it will be a different error message reported. Looks like previously that only reports a Debug level, so maybe this is actually better...!?
@@ -357,15 +357,8 @@ CameraHalConfig::CameraHalConfig() | |||
*/ | |||
int CameraHalConfig::parseConfigurationFile() | |||
{ | |||
std::filesystem::path filePath = LIBCAMERA_SYSCONF_DIR; |
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.
It's a bit unfortunate not to be able to use the standard library - but I guess that's a limitation on Android.
And given that this is an 'android hal' it's probably not unreasonable to highglight that as an issue.
We use the HAL in ChromeOS where we can utilise the std::filesystem.
I wonder if it can be wrapped in some way so we can take advantage of the std::filesystem when it's there or not otherwise.
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.
std::filesystem is present in AOSP but not available for vendor libraries.
To make it available Android.bp
should specify vendor: true
field HERE
But for some reason nobody added it there.
It is a single usage. Do we really want to have 2 implementations here depending on presence of std::filesystem?
@@ -357,15 +357,8 @@ CameraHalConfig::CameraHalConfig() | |||
*/ | |||
int CameraHalConfig::parseConfigurationFile() | |||
{ | |||
std::filesystem::path filePath = LIBCAMERA_SYSCONF_DIR; | |||
filePath /= "camera_hal.yaml"; | |||
if (!std::filesystem::is_regular_file(filePath)) { |
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.
Also you've dropped a check, you're no longer validating that the file is there before opening it. I guess the difference is subtle as the fopen will likely fail anyway, but it will be a different error message reported. Looks like previously that only reports a Debug level, so maybe this is actually better...!?
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.
So should I fstat() it and check is it a regular file or better to keep as is?
libgnutls = cc.find_library('gnutls', required : true) | ||
libdl = dependency('', required : false) | ||
if not cc.has_function('dlopen') | ||
libdl = cc.find_library('dl') |
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.
does this need to be required : true ?
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.
Perhaps that's default. I can't recall.
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.
mesa3d meson.build was used as a reference for doing this.
I've created another var null_dep to make things more clear.
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.
Could you please expand the commit message to explain why this is needed ?
Android.mk
Outdated
@@ -0,0 +1,75 @@ | |||
# Mesa 3-D graphics library |
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.
John Stultz has been telling me about this other work! Sounds like some good progress on getting things moving.
I guess this is not mesa 3-d graphics library here though ;-)
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.
Yes. It was a copy-paste from mesa3d. I spent a day to make it buildable for libcamera and to get quick feedback. Changed.
Can I use Apache-2.0 license for Android.mk files?
Android.mk
Outdated
LOCAL_SHARED_LIBRARIES := $(__MY_SHARED_LIBRARIES) | ||
|
||
# Modules 'camera.libcamera', produces '/vendor/lib{64}/hw/camera.libcamera.so' HAL | ||
$(eval $(call libcamera-lib,camera.libcamera,hw,LIBCAMERA_BIN)) |
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.
I'm planning to split out the android hal so it builds a separate .so rather than uses the libcamera.so - Just something to be aware of
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.
Sounds good. Probably LOCAL_CHECK_ELF_FILES := false
can be dropped then. It fails at this moment because ELF soname
is still libcamera.so
, but filename is camera.libcamera.so
.
If we can resolve the workaround/hacks, I'd say the meson_cross.mk should be something we could integrate into libcamera! Very interesting work. |
251b687
to
7d97046
Compare
Sounds good. Do you know if raspberry pi guys have any plans to drop boost dependency? |
It's used for json parsing and property trees. We're trying to find a way to not need boost but it hasn't been a high priority yet. |
Just been takling to Laurent about this - he's concerned that the android.mk files support will be deprecated and replaced by .bp files, which are only declarative. Is there a plan to manage this with Mesa? |
We know that someday Google plan to stop supporting Android.mk's . |
Hi, ) I just saw 969da31 which is causing conflicts. It is very similar to my ("libcamera: Make issetugid() optional"). I can assume someone else is also working on Android integration. |
Hi @rsglobal There is another team which has been working on android for quite some time as well. |
Commit 969da31 has been developed independently from this pull request. I'm sorry for not having noticed your patch first, the change came from a hack I noticed in a parallel development, as mentioned by Kieran. I'm also afraid I can't add a Suggested-by tag as the commit has been merged already. I'd recommend submitting patches to the libcamera-devel mailing list, as that's what we use for development. Kieran is kind enough to follow issues and pull requests on github, but he can't scale by himself :-) |
libgnutls = cc.find_library('gnutls', required : true) | ||
libdl = dependency('', required : false) | ||
if not cc.has_function('dlopen') | ||
libdl = cc.find_library('dl') |
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.
Could you please expand the commit message to explain why this is needed ?
if not cc.has_function('dlopen') | ||
libdl = cc.find_library('dl') | ||
endif | ||
libgnutls = cc.find_library('gnutls', required : false) |
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.
This changed from required : true to required : false. If that was intentional, it should be moved to a separate commit, with an explanation in the commit message.
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.
AOSP doesn't have this library. I'll move it out of this commit.
Android libc++fs library does not have VNDK support. Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
How to use: 1. Enable libcamera, add pipelines and IPAs into your board.mk file: board.mk: BOARD_LIBCAMERA_USES_MESON_BUILD := true BOARD_LIBCAMERA_IPAS := rkisp1 BOARD_LIBCAMERA_PIPELINES := simple rkisp1 2. Add the following package configuration into your device.mk file: device.mk: PRODUCT_PACKAGES += \ camera.libcamera PRODUCT_PROPERTY_OVERRIDES += \ ro.hardware.camera=libcamera Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
7d97046
to
38c125e
Compare
Updated:
|
Latest new: |
The proxy executable isn't necessary as long as you don't run isolated IPAs. For RPi this is fine, as the IPA can be built from source and signed accordingly. |
Thanks, It worked!!! Got rpi4 pipeline initialized on Android-12 with libcamera. Camera app now visible, but no preview image ATM.. |
One issue with the Android.mk approach is that Android is moving away from make. The current build system, Soong (https://android.googlesource.com/platform/build/soong/+/refs/heads/master/README.md), uses blueprint (Android.bp) files instead, which don't allow running arbitrary commands. They are moving to yet another build system, Bazel, as detailed in https://cs.android.com/android/platform/superproject/+/master:build/bazel/docs/concepts.md, but it will still take some time. |
So, what's the plan? |
Looks like raspberrypi pipeline is incomplete. It doesn't set bufferscount during validation as ipu3 does, which disapoints the Android. |
Interesting, the RPi hasn't been tested much with the Android layer, but I'd like to see it tested more. I see that there was some work to refactor bufferCount recently, but it has stalled. The latest version is https://lists.libcamera.org/pipermail/libcamera-devel/2021-August/023773.html This might be a path towards solving this - or otherwise, initialising a default bufferCount of say 4 in the RPi pipeline handler might help. Can you post this on the mailing list perhaps? |
I keep meaning to try: https://github.com/FydeOS/chromium_os-raspberry_pi/releases/tag/r92 but I haven't had any opportunity or time to do so. |
This is dropped now by the way. I wonder if it helps any development here. |
I'll check it soon, thanks |
Hey! @rsglobal @kbingham , I'm interested in this too and can offer help, a second pair of eyes, etc. if that would help get this merged to git.libcamera.org. Let me know! Additional context: waydroid/waydroid#519 |
Can you rebase to the latest libcamera, and make sure everything compiles successfully, then post to the libcamera mailing list please? |
It works.
Yes, but it would be nice to have code owner for these files from libcamera side. Otherwise it won't work for a long period. |
The first two commits can be reviewed on the list and merged. The third one is more problematic, as make-based build are deprecated in Android (see https://source.android.com/docs/setup/build). |
It was deprecated long time ago nevertheless it is still supported and currently works and used by mainline mesa3d and everybody is happy :) . Building using NDK is an alternative, but it may have a different set of challenges, and NDK doesn't allow to build it within AOSP. |
@rsglobal, My understanding is that @pinchartl's comment refers to Android's new Soong build system, based on Bazel. So, Android.mk would be replaced by Android.bp. The main challenge here is that libcamera uses the Soong build system. We have at least a few options if we want to use an Android.bp instead of the deprecated Android.mk:
If there's appetite for either of these options, I'm happy to help out where I can. I use Bazel regularly at work, but I'll need help understanding these Makefiles. For example, we'd have something like this: cc_library_shared {
name: "camera.libcamera",
relative_install_path: "hw",
vendor: true,
srcs: [
"foo",
"bar",
],
shared_libs: [
"libc",
"libexif",
"libjpeg",
"libyuv_chromium",
"libdl",
"libyaml",
],
}
cc_library_shared {
name: "libcamera",
vendor: true,
srcs: [
"foo",
"bar",
],
shared_libs: [
"libc",
"libexif",
"libjpeg",
"libyuv_chromium",
"libdl",
"libyaml",
],
}
cc_library_shared {
name: "libcamera-base",
vendor: true,
srcs: [
"foo",
"bar",
],
shared_libs: [
"libc",
"libexif",
"libjpeg",
"libyuv_chromium",
"libdl",
"libyaml",
],
} ...Instead of this: # SPDX-License-Identifier: Apache-2.0
#
# Copyright (C) 2021, GlobalLogic Ukraine
# Copyright (C) 2021, Roman Stratiienko (r.stratiienko@gmail.com)
#
# Android.mk - Android makefile
#
ifneq ($(filter true, $(BOARD_LIBCAMERA_USES_MESON_BUILD)),)
LOCAL_PATH := $(call my-dir)
LIBCAMERA_TOP := $(dir $(LOCAL_PATH))
include $(CLEAR_VARS)
LOCAL_SHARED_LIBRARIES := libc libexif libjpeg libyuv_chromium libdl libyaml
MESON_GEN_PKGCONFIGS := libexif libjpeg yaml-0.1 libyuv dl
ifeq ($(TARGET_IS_64_BIT),true)
LOCAL_MULTILIB := 64
else
LOCAL_MULTILIB := 32
endif
include $(LOCAL_PATH)/meson_cross.mk
ifdef TARGET_2ND_ARCH
LOCAL_MULTILIB := 32
include $(LOCAL_PATH)/meson_cross.mk
endif
#-------------------------------------------------------------------------------
define libcamera-lib
LOCAL_MODULE_CLASS := SHARED_LIBRARIES
LOCAL_MODULE := $1
LOCAL_VENDOR_MODULE := true
LOCAL_MODULE_RELATIVE_PATH := $2
ifdef TARGET_2ND_ARCH
LOCAL_SRC_FILES_$(TARGET_ARCH) := $(call relative_top_path,$(LOCAL_PATH))$($3)
LOCAL_SRC_FILES_$(TARGET_2ND_ARCH) := $(call relative_top_path,$(LOCAL_PATH))$(2ND_$3)
LOCAL_MULTILIB := both
else
LOCAL_SRC_FILES := $(call relative_top_path,$(LOCAL_PATH))$($3)
endif
LOCAL_CHECK_ELF_FILES := false
LOCAL_MODULE_SUFFIX := .so
include $(BUILD_PREBUILT)
include $(CLEAR_VARS)
endef
__MY_SHARED_LIBRARIES := $(LOCAL_SHARED_LIBRARIES)
include $(CLEAR_VARS)
LOCAL_SHARED_LIBRARIES := $(__MY_SHARED_LIBRARIES)
# Modules 'libcamera', produces '/vendor/lib{64}/libcamera.so'
$(eval $(call libcamera-lib,libcamera,,LIBCAMERA_BIN))
# Modules 'libcamera-base', produces '/vendor/lib{64}/libcamera-base.so'
$(eval $(call libcamera-lib,libcamera-base,,LIBCAMERA_BASE_BIN))
LOCAL_SHARED_LIBRARIES += libcamera libcamera-base
# Modules 'camera.libcamera', produces '/vendor/lib{64}/hw/camera.libcamera.so' HAL
$(eval $(call libcamera-lib,camera.libcamera,hw,LIBCAMERA_HAL_BIN))
#-------------------------------------------------------------------------------
endif |
I'll add that a lot of thought has been put into steering people away from using Interested to read your thoughts on this too, @kbingham! |
@rothn , Each solution has its pros and cons, maintainers should decide which way to go. Since libcamera advertises Android support , could you tell us more how and on what hardware it's used now? It would be very nice to have some android-related how-to docs. |
I would very much like it to be easier to use libcamera with AOSP, so this deserves a discussion on the libcamera mailing list (https://lists.libcamera.org/listinfo/libcamera-devel) not here. I can't see us changing the build system drastically though, we've chosen meson already - so we'd need to see real clear benefits for the change. We already support ChromeOS with meson. Keeping two builds in parallel will be a pain as well though, but if a build for android exists, I think perhaps it's better to live in libcamera.git than outside. At least then it's centralised - but we'd probably want a way to easily automate building against AOSP/NDK so we can track it. It seems I have a lot of questions about how we can handle this - but I'd really prefer we discuss the options on the actual development list so others can be involved, not just me. |
If we merge the Android.mk file, how do we build test it? |
The Android HAL is used by ChromeOS. We currently use this with Intel IPU3 Chromebooks, and I believe it can be used already by a Rockchip RK3399 ChromeTab too, though I don't think that one has been used in quite a while. Most of our development has been with the IPU3. |
Since there're a lot of issues that require resolution, I would suggest to start from something small, like build using Android NDK for raspberry-pi4 and starting semi-official support for it. Then you could extend Android coverage. And finally adapt Android.mk to build within AOSP. I think a lot of rpi4-based distributions already adapted meson/Android.mk and use libcamera, but I would still suggest using glodroid/rpi4 images because in this case I could support you. After successful NDK build you should be able to push binaries onto the board using |
@rsglobal Is this something that you are willing to do ? I.e. will you post the support to the libcamera mailing list, and provide documentation on how to build/use it ? |
Unfortunately I don't have enough free time for that right now. Also @pinchartl statements that Android.mk is obsolete makes me feel that I'm risking to waste a lot of time without eventually get things up-streamed. |
I'm happy to merge an Android.mk if we can build test it to catch breakage. If Android.mk options become obsolete by Android, then it won't work past that version, but it would still work for earlier versions that the deprecation - so I still think there's value. But ... the patches need to be posted to the libcamera-devel mailing list - They won't get merged here. |
To many effort has to be done to modify common code to support building within AOSP. I feel like review process will take forever. Also I hate mailing lists. If you can assign some smart trainee from libcamera team I could share all my experience and support him outside mailing list. |
We're a small team and resource constrained. I don't think we have anyone that could do that right now. @rothn , as you resumed this conversation here, is this something you want to take on? |
Yes, I'm happy to help, not least because this happens to support my
phone's camera :). I'd like to take a stab at creating a parallel
Android.bp which would be continuously tested with "blaze test ..." or
similar. Let me know if that would work for you.
I'll try posting this to the mailing list as well later this evening along
with some context, and you may prefer to reply there.
…On Tue, Oct 11, 2022, 1:10 PM Kieran Bingham ***@***.***> wrote:
We're a small team and resource constrained. I don't think we have anyone
that could do that right now.
@rothn <https://github.com/rothn> , as you resumed this conversation
here, is this something you want to take on?
—
Reply to this email directly, view it on GitHub
<#26 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABDTDN6LB3OGYQN5PPINQD3WCWUSJANCNFSM46J7WCKA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Mesa3d was living with similar approach for a long time. Once a week it required tuning and therefore Android build was usually broken. With current Android.mk approach we have just a 1 fix per 6 month which is a way better. Building using android NDK should similar to building for pure linux. |
Thanks, if we merge support to build android in libcamera, then some equivalent documentation for this should be added to libcamera too so it can be referenced for how people can use it. |
Wrapping the existing meson build sounds more supportable than a parallel build that needs constant maintenance everytime we make a change / add a new object / compile flags etc...
Good, that makes it easy to incorporate into the build tests. |
Require additional dependencies:
How to use:
Example of
/vendor/etc/libcamera/camera_hal.yaml
:https://chromium-review.googlesource.com/c/chromiumos/overlays/board-overlays/+/2866850/2/overlay-soraka-libcamera/media-libs/libcamera-configs/files/camera_hal.yaml