| Message ID | 20220808151918.25411-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 777b0e0a655cce258a2b11e98546c3fc5a5be031 |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for the patch. 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? 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@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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')), > )
Quoting Umang Jain via libcamera-devel (2022-08-10 07:44:31) > Hi Laurent, > > Thank you for the patch. > > 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. -- Kieran > 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@ideasonboard.com> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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')), > > )
Quoting Laurent Pinchart via libcamera-devel (2022-08-08 16:19:18) > 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. Not used by public API == Private. So I don't think this is even RFC ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> However I'd still like to make -base more widely reusable sometime, But then that needs to go alongside something that goes 'on top' of libcamera too to support add on helpers ... (libcamera-top? still waiting to complete the bikeshedding there before I create the library heheh). > Signed-off-by: Laurent Pinchart <laurent.pinchart@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 >
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@ideasonboard.com> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@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')), > > > )
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')), )
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@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(-)