[libcamera-devel] [PATCH] android: Split HAL to it's own shared library

[Laurent Pinchart]

Hi Kieran,

On Sun, May 02, 2021 at 09:02:45PM +0100, Kieran Bingham wrote:
> On 01/05/2021 21:54, Laurent Pinchart wrote:
> > On Fri, Apr 30, 2021 at 04:02:19PM +0100, Kieran Bingham wrote:
> >> The libcamera Android HAL implementation should not be an integral part
> >> of libcamera, but a support library that utilises the libcamera public
> >> API.
> >>
> >> Move the implementation to it's own distinct library.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>
> >> Please note that this no longer requires ChromeOS to manually symlink
> >> into the camera_hal directory.
> >>
> >> A modification similar to the following should be applied to the Chrome
> >> ebuilds if this is applied:
> >>
> >> diff --git a/media-libs/libcamera/libcamera-9999.ebuild b/media-libs/libcamera/libcamera-9999.ebuild
> >> index 433357f1ef74..cf90e438cfb3 100644
> >> --- a/media-libs/libcamera/libcamera-9999.ebuild
> >> +++ b/media-libs/libcamera/libcamera-9999.ebuild
> >> @@ -57,6 +57,4 @@ src_compile() {
> >>  
> >>  src_install() {
> >>         meson_src_install
> >> -
> >> -       dosym ../libcamera.so "/usr/$(get_libdir)/camera_hal/libcamera.so"
> >>  }
> > 
> > Could you submit a patch to CrOS ?
> > 
> >>  src/android/meson.build   | 10 ++++++++++
> >>  src/libcamera/meson.build | 11 -----------
> >>  src/meson.build           |  7 +++----
> >>  3 files changed, 13 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/src/android/meson.build b/src/android/meson.build
> >> index 2be20c97e118..6170448a73ce 100644
> >> --- a/src/android/meson.build
> >> +++ b/src/android/meson.build
> >> @@ -3,6 +3,7 @@
> >>  android_deps = [
> >>      dependency('libexif', required : get_option('android')),
> >>      dependency('libjpeg', required : get_option('android')),
> >> +    libcamera_dep,
> >>  ]
> >>  
> >>  android_enabled = true
> >> @@ -66,3 +67,12 @@ android_camera_metadata = static_library('camera_metadata',
> >>                                           android_camera_metadata_sources,
> >>                                           c_args : '-Wno-shadow',
> >>                                           include_directories : android_includes)
> >> +
> >> +android_hal = shared_library('android-hal',
> > 
> > android-hal.so won't be a very descriptive name. Maybe libcamera-hal.so
> 
> I started out with libcamera-hal, but then I decided I didn't like it so
> I updated to android-hal ...
> 
> Given that this is an android-hal implementation I thought that was
> /more/ descriptive ...
> 
> otherwise libcamera-hal sounds like it's an abstraction layer of libcamera.
> 
> If you prefer libcamera-hal I'll change it back.

Following that logic, every shared object on a Linux system should be
called linux-* :-) Given that we may have different HAL implementations
in the same directory (both multiple camera HAL implementations in
Chrome OS, and multiple HALs for unrelated devices in Android), it's
important for the file name to specify the project and possibly the fact
that it's related to cameras. With 'libcamera' as the project name, both
goals are reached :-)

> > ?
> > 
> >> +                             android_hal_sources,
> >> +                             name_prefix : '',
> >> +                             link_with : android_camera_metadata,
> >> +                             install : true,
> >> +                             cpp_args : libcamera_cpp_args,
> >> +                             include_directories : android_includes,
> >> +                             dependencies : android_deps)
> > 
> > Without install_dir, this will get installed in /usr/lib/, which isn't
> > where the CrOS camera service will look for it. We probably need a
> > configuration option to set the path, as it will differ between CrOS and
> > Android.
> 
> But I have install_dir ... uhm ... Ok so I forgot to save out the
> updated patch before sending ...
> 
> It's there in the next version already ;-) ...
> 
> > +android_installdir = get_option('libdir')
> > +
> >  if get_option('android_platform') == 'cros'
> >     libcamera_cpp_args += [ '-DOS_CHROMEOS']

Extra space.

> > +   android_installdir = get_option('libdir') / 'camera_hal'
> >  endif
> 
> Will install it to camera_hal directly when the platform is set.

Should install be set to true on CrOS only ? On Android meson isn't used
anyway, and on other systems, the Android HAL isn't needed.

> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index e0a48aa23ea5..f77b80b878d8 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -129,16 +129,6 @@ libcamera_deps = [
> >>      dependency('threads'),
> >>  ]
> >>  
> >> -libcamera_link_with = []
> >> -
> >> -if android_enabled
> >> -    libcamera_sources += android_hal_sources
> >> -    includes += android_includes
> >> -    libcamera_link_with += android_camera_metadata
> >> -
> >> -    libcamera_deps += android_deps
> >> -endif
> >> -
> >>  # We add '/' to the build_rpath as a 'safe' path to act as a boolean flag.
> >>  # The build_rpath is stripped at install time by meson, so we determine at
> >>  # runtime if the library is running from an installed location by checking
> >> @@ -147,7 +137,6 @@ endif
> >>  libcamera = shared_library('camera',
> >>                             libcamera_sources,
> >>                             install : true,
> >> -                           link_with : libcamera_link_with,
> >>                             cpp_args : libcamera_cpp_args,
> >>                             include_directories : includes,
> >>                             objects : libcamera_objects,
> >> diff --git a/src/meson.build b/src/meson.build
> >> index 64d22b9ce5b6..9dc32751bbb1 100644
> >> --- a/src/meson.build
> >> +++ b/src/meson.build
> >> @@ -31,11 +31,10 @@ endif
> >>  libcamera_cpp_args = []
> >>  libcamera_objects = []
> >>  
> >> -# The 'android' subdir must be processed first, and the build targets
> >> -# are included directly into the libcamera library when this is enabled.
> >> -subdir('android')
> >> -
> >> +# libcamera must be built first as a dependency to the other components.
> >>  subdir('libcamera')
> > 
> > This is really nice, having android first has been bugging me for a
> > while.
> 
> Me to, I somehow assumed last time I looked at this that there were
> deeper reasons for integrating into the libcamera.so - but there wasn't
> anything preventing the split really.
> 
> Anyway, v2 to come anyway with the correct install paths, and if desired
> - a rename.
> 
> >> +
> >> +subdir('android')
> >>  subdir('ipa')
> >>  
> >>  subdir('lc-compliance')

-- 
Regards,

Laurent Pinchart