[libcamera-devel] [RFC PATCH] libcamera: base: Make message.h and mutex.h private

[Laurent Pinchart]

On Wed, Aug 10, 2022 at 09:24:49AM +0100, Kieran Bingham wrote:
> Quoting Umang Jain via libcamera-devel (2022-08-10 07:44:31)
> > On 8/8/22 20:49, Laurent Pinchart via libcamera-devel wrote:
> > > The message.h and mutex.h headers are not used in the libcamera public
> > > API. Make them private to avoid there usage in applications, and to
> > > prevent having to maintain them with a stable ABI.
> > >
> > > As mutex.h is used by libcamerasrc, the GStreamer element must switch
> > > from the libcamera_public to the libcamera_private dependency.
> > 
> > Should the gstreamer element be moving away from using those headers as 
> > well?
> 
> I think that would be down to where the gstreamer element lives.
> 
> Anything built within the libcamera code base can use the private
> headers, as we can ensure that they are updated accordingly.
> 
> But if the gstlibcamerasrc element moves to the gstreamer project, that
> would have to be refactored. However, it likely can't move there until
> we have a stable API and ABI for libcamera, so I think it's fine to use
> the internal headers for now.

That's a very good point, we may need to move away from mutex.h in this
case. We could do so already, but I don't think there's an urgency, and
it doesn't block marking mutex.h as private. There's one possible issue
though, in that further usage of private APIs in libcamerasrc will not
be caught by the compiler. That would make libcamerasrc more dependent
on private APIs, which we may not want to facilitate a future
conversion. I think this could be handled through reviews.

> > If it's a question on locks (I see Mutex and MutexLocker) being used, 
> > replacing with GLib equivalent can be a way forwards to drop those (now 
> > private) headers?
> > 
> > In that case, this patch can introduce a \todo in gstreamer element.
> > 
> > Anyways,
> > 
> > Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >   include/libcamera/base/message.h | 2 ++
> > >   include/libcamera/base/mutex.h   | 2 ++
> > >   src/gstreamer/meson.build        | 2 +-
> > >   3 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/libcamera/base/message.h b/include/libcamera/base/message.h
> > > index 65572c7470e9..b939af6f79bb 100644
> > > --- a/include/libcamera/base/message.h
> > > +++ b/include/libcamera/base/message.h
> > > @@ -9,6 +9,8 @@
> > >   
> > >   #include <atomic>
> > >   
> > > +#include <libcamera/base/private.h>
> > > +
> > >   #include <libcamera/base/bound_method.h>
> > >   
> > >   namespace libcamera {
> > > diff --git a/include/libcamera/base/mutex.h b/include/libcamera/base/mutex.h
> > > index 2d23e49e4546..52441c55287a 100644
> > > --- a/include/libcamera/base/mutex.h
> > > +++ b/include/libcamera/base/mutex.h
> > > @@ -10,6 +10,8 @@
> > >   #include <condition_variable>
> > >   #include <mutex>
> > >   
> > > +#include <libcamera/base/private.h>
> > > +
> > >   #include <libcamera/base/thread_annotations.h>
> > >   
> > >   namespace libcamera {
> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > > index 77c79140eb37..eda246d7ffc8 100644
> > > --- a/src/gstreamer/meson.build
> > > +++ b/src/gstreamer/meson.build
> > > @@ -42,7 +42,7 @@ endif
> > >   libcamera_gst = shared_library('gstlibcamera',
> > >       libcamera_gst_sources,
> > >       cpp_args : libcamera_gst_cpp_args,
> > > -    dependencies : [libcamera_public, gstvideo_dep, gstallocator_dep],
> > > +    dependencies : [libcamera_private, gstvideo_dep, gstallocator_dep],
> > >       install: true,
> > >       install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),
> > >   )

-- 
Regards,

Laurent Pinchart