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

[Kieran Bingham]

Hi Laurent,

On 01/05/2021 21:54, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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.

> ?
> 
>> +                             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']
> +   android_installdir = get_option('libdir') / 'camera_hal'
>  endif

Will install it to camera_hal directly when the platform is set.


>> 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.

--
Kieran


> 
>> +
>> +subdir('android')
>>  subdir('ipa')
>>  
>>  subdir('lc-compliance')
> 

-- 
Regards
--
Kieran