[libcamera-devel] [PATCH v1 5/6] meson: Generate a helper .so containing libcamera's internal headers

[Laurent Pinchart]

Hello,

On Fri, May 14, 2021 at 11:34:51AM +0100, Kieran Bingham wrote:
> On 14/05/2021 08:58, Umang Jain wrote:
> > The IPU3 closed source IPA shall live externally(out-of-tree) however,
> > it needs access to some of the internal libcamera headers. These headers
> > are not typically intended for public usage hence, shouldn't be placed
> > as  headers in $includedir. In order to solve this issue, generate a
> > helper shared library, which can be used to link with, by the external
> > IPU3 closed source IPA.
> 
> This is a much bigger task than just sharing the headers.
> Headers should be accompanied with implementation.
> 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  include/libcamera/internal/meson.build | 6 ++++++
> >  meson.build                            | 7 +++++++
> >  src/libcamera/meson.build              | 6 ++++++
> >  3 files changed, 19 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> > index 6cff1b90..fcbf4212 100644
> > --- a/include/libcamera/internal/meson.build
> > +++ b/include/libcamera/internal/meson.build
> > @@ -49,3 +49,9 @@ libcamera_internal_headers = files([
> >      'v4l2_subdevice.h',
> >      'v4l2_videodevice.h',
> >  ])
> > +
> > +libcamera_helpers = files ([
> > +    'buffer.h',
> > +    'file.h',
> > +    'log.h',
> > +])
> > diff --git a/meson.build b/meson.build
> > index b89797f3..c48c5a66 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -160,6 +160,13 @@ pkg_mod.generate(libraries : libcamera,
> >                   description : 'Complex Camera Support Library',
> >                   subdirs : 'libcamera')
> >  
> > +# \todo Bikeshedding on camera_helper naming
> > +pkg_mod.generate(libraries : libcamera_helper_lib,
> > +                 version : '0.0',
> > +                 name : 'libcamera_helper_lib',
> > +                 filebase : 'camera_helper',
> > +                 description : 'libcamera internal helper library')
> > +
> >  # Check for python installation and modules.
> >  py_mod = import('python')
> >  py_mod.find_installation('python3', modules: py_modules)
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index 7bc59b84..788b767d 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -162,4 +162,10 @@ libcamera_dep = declare_dependency(sources : [
> >                                     include_directories : libcamera_includes,
> >                                     link_with : libcamera)
> >  
> > +# libcamera helper shared library
> > +libcamera_helper_lib = shared_library('camera_helper', [libcamera_helpers],
> > +                                      install: true,
> > +                                      install_dir: libcamera_libdir,
> > +                                      dependencies: libcamera_dep)
> 
> As a separate library, it should be separated from src/libcamera
> 
> So dependant upon the name selected this should have it's own structure
> 
>  include/
> 	libcamera-support/
> 		file.h
> 		log.h
> 		buffer.h

I had envisioned include/libcamera/support/ (bikeshedding on the
'support' name aside).

>  src/
> 	libcamera-support/
> 		file.cpp
> 		log.cpp
> 		buffer.cpp

And possibly similarly here, although I wouldn't object keeping the
files in src/libcamera/

> And then libcamera should link against it. But things are not that
> simple, and the dependencies between all these are quite convoluted.
> 
> 
> Of course the library name is hard to choose.
> It could be
> 	libcamera-helper,
> 	libcamera-utils,
> 	libcamera-support,
> 	libcamera-internal ...

My initial preference is libcamera-utils.

> And then the scope of what goes in there could be interesting too.
> 
> log and file are system helpers, accessing files and generic logging.
> Buffers are (our) common buffer abstraction
> 
> 
> 
> But there's others to consider (in the future, not necessarily immediately)
> 
> 
> ## definitely libcamera internal?
> 
> byte_stream_buffer.h
> camera_controls.h
> camera_sensor.h
> camera_sensor.h.orig

I think we can skip this one ;-)

> camera_sensor_properties.h
> control_serializer.h
> control_validator.h
> delayed_controls.h
> event_dispatcher.h
> event_dispatcher_poll.h
> event_notifier.h
> ipa_data_serializer.h
> ipa_manager.h
> ipa_module.h
> ipa_proxy.h
> pipeline_handler.h
> tracepoints
> utils.h

utils.h sounds fairly useful for IPA modules.

> # Linux V4L2/MediaController/Pixel Formats / buffers
> device_enumerator.h
> device_enumerator_sysfs.h
> device_enumerator_udev.h
> media_device.h
> media_object.h
> v4l2_controls.h
> v4l2_device.h
> v4l2_pixelformat.h
> v4l2_subdevice.h
> v4l2_videodevice.h

All this is definitely libcamera internal, IPA modules must never access
devices.

> # Pixel Formats and buffers
> bayer_format.h
> buffer.h
> formats.h
> 
> 
> # Operating system specific abstractions ?
> file.h
> ipc_pipe.h
> ipc_pipe_unixsocket.h
> ipc_unixsocket.h

IPC should be handled transparently by the generated code for the IPA
module, so I'd skip it for now. We'll have to revisit this as part of
the IPC mechanism rework discussions (related to whether to keep
spawning a process directly, or use an external daemon), for now I'd
prefer only moving code that we know is of interest to IPA modules,
excluding the proxy.

> log.h
> message.h
> process.h
> pub_key.h

I'd also skip process.h and pub_key.h, I don't see a need for IPA
modules to use those.

> semaphore.h
> sysfs.h

Same for sysfs.h.

> thread.h
> timer.h

Timers and threads require an event dispatcher, so you would need to
also move event_dispatcher.h and event_dispatcher_poll.h, and, as a
consequence, event_notifier.h.

> > +
> >  subdir('proxy/worker')
> > 

-- 
Regards,

Laurent Pinchart