| Message ID | 20200129033210.278800-2-nicolas@ndufresne.ca |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Nicolas, Thank you for the patch. On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote: > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > --- > meson_options.txt | 5 +++++ > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++ > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++ > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++ > src/gstreamer/meson.build | 23 +++++++++++++++++++++++ > src/meson.build | 2 ++ > 6 files changed, 104 insertions(+) > create mode 100644 src/gstreamer/gstlibcamera.c > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp > create mode 100644 src/gstreamer/gstlibcamerasrc.h > create mode 100644 src/gstreamer/meson.build > > diff --git a/meson_options.txt b/meson_options.txt > index 79ee4de..1b78ed8 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -15,3 +15,8 @@ option('v4l2', > type : 'boolean', > value : false, > description : 'Compile the V4L2 compatibility layer') > + > +option('gstreamer', Should we keep options alphabetically sorted ? It always bothers my OCD otherwise as I never know where to insert new options in an unsorted list :-) > + type : 'feature', > + value : 'auto', > + description : 'Compile libcamera GStreamer plugin') > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c > new file mode 100644 > index 0000000..953fb29 > --- /dev/null > +++ b/src/gstreamer/gstlibcamera.c > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Collabora Ltd. > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + * > + * gstlibcamera.c - GStreamer plugin > + */ > + > +#include "gstlibcamerasrc.h" > + > +static gboolean > +plugin_init(GstPlugin *plugin) You don't have to break this line. I've run checkstyle.py on your whole series, and the most common issue it reports is the unneeded line break in function definitions. There's also an extra space after a function call in "gst: libcamerasrc: Add camera-name property", a trailing whitespace in "gst: libcamerapad: Allow storing a pool", and an extra blank line at end of file in "gst: Add initial device provider" (this one is reported by git-am only). It's very little, you did a good job :-) If you fix those small issues I think we'll be good to go from a coding style point of view. > +{ > + return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY, > + GST_TYPE_LIBCAMERA_SRC); > + return TRUE; > +} > + > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR, > + libcamera, "LibCamera capture plugin", libcamera is always written in all lowercase (it's part of the ascii art branding :-)). This comment applies to all patches in this series. > + plugin_init, VERSION, "LGPL", PACKAGE, "https://libcamera.org"); > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > new file mode 100644 > index 0000000..39a06a4 > --- /dev/null > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -0,0 +1,31 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Collabora Ltd. > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + * > + * gstlibcamerasrc.cpp - GStreamer Capture Element > + */ > + > +#include "gstlibcamerasrc.h" > + > +struct _GstLibcameraSrc { This structure doesn't seem to be used, do I understand correctly that it supports the G_DEFINE_TYPE() below through some magic that is, in glib terms, akin to C++ templates ? :-) > + GstElement parent; > +}; > + > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT); > + > +static void > +gst_libcamera_src_init(GstLibcameraSrc *self) Same for this function ? > +{ > +} > + > +static void > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > +{ > + GstElementClass *element_class = (GstElementClass *)klass; > + > + gst_element_class_set_metadata(element_class, > + "LibCamera Source", "Source/Video", > + "Linux Camera source using libcamera", > + "Nicolas Dufresne <nicolas.dufresne@collabora.com"); > +} > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h > new file mode 100644 > index 0000000..6d2f794 > --- /dev/null > +++ b/src/gstreamer/gstlibcamerasrc.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Collabora Ltd. > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + * > + * gstlibcamerasrc.h - GStreamer Capture Element > + */ > + > +#include <gst/gst.h> > + You can move this after the #ifndef / #define guard, it should reduce compilation time (very slightly). > +#ifndef __GST_LIBCAMERA_SRC_H__ > +#define __GST_LIBCAMERA_SRC_H__ > + > +G_BEGIN_DECLS > + > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type() > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src, > + GST_LIBCAMERA, SRC, GstElement) > + > +G_END_DECLS > + > +#endif /* __GST_LIBCAMERA_SRC_H__ */ > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > new file mode 100644 > index 0000000..32d4233 > --- /dev/null > +++ b/src/gstreamer/meson.build > @@ -0,0 +1,23 @@ > +libcamera_gst_sources = [ > + 'gstlibcamera.c', > + 'gstlibcamerasrc.cpp', > +] > + > +libcamera_gst_c_args = [ > + '-DVERSION="@0@"'.format(libcamera_git_version), > + '-DPACKAGE="@0@"'.format(meson.project_name()), > +] > + > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1', > + required : get_option('gstreamer')) Could you please align this with the ( ? > + > +if gst_dep.found() > + libcamera_gst = shared_library('gstlibcamera', We use 4 spaces to indent meson.build files. > + libcamera_gst_sources, > + c_args : libcamera_gst_c_args, > + include_directories : [], Do you need to specify an empty array explicitly here, can't you remove the parameter ? > + dependencies : [libcamera_dep, gst_dep], > + install: true, > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), > + ) > +endif > diff --git a/src/meson.build b/src/meson.build > index 5adcd61..d818d8b 100644 > --- a/src/meson.build > +++ b/src/meson.build > @@ -10,3 +10,5 @@ subdir('qcam') > if get_option('v4l2') > subdir('v4l2') > endif > + > +subdir('gstreamer') It feels good to see the skeleton, knowing there are 22 patches following it :-)
Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit : > Hi Nicolas, > > Thank you for the patch. > > On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote: > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > --- > > meson_options.txt | 5 +++++ > > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++ > > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++ > > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++ > > src/gstreamer/meson.build | 23 +++++++++++++++++++++++ > > src/meson.build | 2 ++ > > 6 files changed, 104 insertions(+) > > create mode 100644 src/gstreamer/gstlibcamera.c > > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp > > create mode 100644 src/gstreamer/gstlibcamerasrc.h > > create mode 100644 src/gstreamer/meson.build > > > > diff --git a/meson_options.txt b/meson_options.txt > > index 79ee4de..1b78ed8 100644 > > --- a/meson_options.txt > > +++ b/meson_options.txt > > @@ -15,3 +15,8 @@ option('v4l2', > > type : 'boolean', > > value : false, > > description : 'Compile the V4L2 compatibility layer') > > + > > +option('gstreamer', > > Should we keep options alphabetically sorted ? It always bothers my OCD > otherwise as I never know where to insert new options in an unsorted > list :-) > > > + type : 'feature', > > + value : 'auto', > > + description : 'Compile libcamera GStreamer plugin') > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c > > new file mode 100644 > > index 0000000..953fb29 > > --- /dev/null > > +++ b/src/gstreamer/gstlibcamera.c > > @@ -0,0 +1,21 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Collabora Ltd. > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > + * > > + * gstlibcamera.c - GStreamer plugin > > + */ > > + > > +#include "gstlibcamerasrc.h" > > + > > +static gboolean > > +plugin_init(GstPlugin *plugin) > > You don't have to break this line. This is GStreamer/GLib style, it has a really strong rational for searching C code. As you can with grep (or other regex) differentiate a definition from a declaration. > > I've run checkstyle.py on your whole series, and the most common issue > it reports is the unneeded line break in function definitions. There's I don't recall seeing any warning about unneeded break other then these though, I'll recheck. > also an extra space after a function call in "gst: libcamerasrc: Add > camera-name property", a trailing whitespace in "gst: libcamerapad: > Allow storing a pool", and an extra blank line at end of file in "gst: > Add initial device provider" (this one is reported by git-am only). It's > very little, you did a good job :-) If you fix those small issues I > think we'll be good to go from a coding style point of view. Sure, I'll be going to the patches one by one to apply the fixes, and will update. Let me know what the team wants for the declaration vs definition style. > > > +{ > > + return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY, > > + GST_TYPE_LIBCAMERA_SRC); > > + return TRUE; > > +} > > + > > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR, > > + libcamera, "LibCamera capture plugin", > > libcamera is always written in all lowercase (it's part of the ascii art > branding :-)). This comment applies to all patches in this series. > > > + plugin_init, VERSION, "LGPL", PACKAGE, "https://libcamera.org"); > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > new file mode 100644 > > index 0000000..39a06a4 > > --- /dev/null > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -0,0 +1,31 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Collabora Ltd. > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > + * > > + * gstlibcamerasrc.cpp - GStreamer Capture Element > > + */ > > + > > +#include "gstlibcamerasrc.h" > > + > > +struct _GstLibcameraSrc { > > This structure doesn't seem to be used, do I understand correctly that > it supports the G_DEFINE_TYPE() below through some magic that is, in > glib terms, akin to C++ templates ? :-) This is the instance structure, so this is what you will point to when you create that object. At this very early stage, it only used for allocation purpose by GObject library (through G_DEFINE_TYPE()). > > > + GstElement parent; > > +}; > > + > > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT); > > + > > +static void > > +gst_libcamera_src_init(GstLibcameraSrc *self) > > Same for this function ? In C++, would be called a default constructor. It's called by GObject, and it's not optional with helpers like G_DEFINE_TYPE. > > > +{ > > +} > > + > > +static void > > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > +{ > > + GstElementClass *element_class = (GstElementClass *)klass; > > + > > + gst_element_class_set_metadata(element_class, > > + "LibCamera Source", "Source/Video", > > + "Linux Camera source using libcamera", > > + "Nicolas Dufresne <nicolas.dufresne@collabora.com"); > > +} > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h > > new file mode 100644 > > index 0000000..6d2f794 > > --- /dev/null > > +++ b/src/gstreamer/gstlibcamerasrc.h > > @@ -0,0 +1,22 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Collabora Ltd. > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > + * > > + * gstlibcamerasrc.h - GStreamer Capture Element > > + */ > > + > > +#include <gst/gst.h> > > + > > You can move this after the #ifndef / #define guard, it should reduce > compilation time (very slightly). > > > +#ifndef __GST_LIBCAMERA_SRC_H__ > > +#define __GST_LIBCAMERA_SRC_H__ > > + > > +G_BEGIN_DECLS > > + > > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type() > > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src, > > + GST_LIBCAMERA, SRC, GstElement) > > + > > +G_END_DECLS > > + > > +#endif /* __GST_LIBCAMERA_SRC_H__ */ > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > new file mode 100644 > > index 0000000..32d4233 > > --- /dev/null > > +++ b/src/gstreamer/meson.build > > @@ -0,0 +1,23 @@ > > +libcamera_gst_sources = [ > > + 'gstlibcamera.c', > > + 'gstlibcamerasrc.cpp', > > +] > > + > > +libcamera_gst_c_args = [ > > + '-DVERSION="@0@"'.format(libcamera_git_version), > > + '-DPACKAGE="@0@"'.format(meson.project_name()), > > +] > > + > > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1', > > + required : get_option('gstreamer')) > > Could you please align this with the ( ? > > > + > > +if gst_dep.found() > > + libcamera_gst = shared_library('gstlibcamera', > > We use 4 spaces to indent meson.build files. > > > + libcamera_gst_sources, > > + c_args : libcamera_gst_c_args, > > + include_directories : [], > > Do you need to specify an empty array explicitly here, can't you remove > the parameter ? > > > + dependencies : [libcamera_dep, gst_dep], > > + install: true, > > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), > > + ) > > +endif > > diff --git a/src/meson.build b/src/meson.build > > index 5adcd61..d818d8b 100644 > > --- a/src/meson.build > > +++ b/src/meson.build > > @@ -10,3 +10,5 @@ subdir('qcam') > > if get_option('v4l2') > > subdir('v4l2') > > endif > > + > > +subdir('gstreamer') > > It feels good to see the skeleton, knowing there are 22 patches > following it :-) >
Hi Nicolas, On Tue, Feb 25, 2020 at 10:38:41AM -0500, Nicolas Dufresne wrote: > Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit : > > On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote: > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > --- > > > meson_options.txt | 5 +++++ > > > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++ > > > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++ > > > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++ > > > src/gstreamer/meson.build | 23 +++++++++++++++++++++++ > > > src/meson.build | 2 ++ > > > 6 files changed, 104 insertions(+) > > > create mode 100644 src/gstreamer/gstlibcamera.c > > > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp > > > create mode 100644 src/gstreamer/gstlibcamerasrc.h > > > create mode 100644 src/gstreamer/meson.build > > > > > > diff --git a/meson_options.txt b/meson_options.txt > > > index 79ee4de..1b78ed8 100644 > > > --- a/meson_options.txt > > > +++ b/meson_options.txt > > > @@ -15,3 +15,8 @@ option('v4l2', > > > type : 'boolean', > > > value : false, > > > description : 'Compile the V4L2 compatibility layer') > > > + > > > +option('gstreamer', > > > > Should we keep options alphabetically sorted ? It always bothers my OCD > > otherwise as I never know where to insert new options in an unsorted > > list :-) > > > > > + type : 'feature', > > > + value : 'auto', > > > + description : 'Compile libcamera GStreamer plugin') > > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c > > > new file mode 100644 > > > index 0000000..953fb29 > > > --- /dev/null > > > +++ b/src/gstreamer/gstlibcamera.c > > > @@ -0,0 +1,21 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Collabora Ltd. > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + * > > > + * gstlibcamera.c - GStreamer plugin > > > + */ > > > + > > > +#include "gstlibcamerasrc.h" > > > + > > > +static gboolean > > > +plugin_init(GstPlugin *plugin) > > > > You don't have to break this line. > > This is GStreamer/GLib style, it has a really strong rational for > searching C code. As you can with grep (or other regex) differentiate a > definition from a declaration. > > > I've run checkstyle.py on your whole series, and the most common issue > > it reports is the unneeded line break in function definitions. There's > > I don't recall seeing any warning about unneeded break other then these > though, I'll recheck. > > > also an extra space after a function call in "gst: libcamerasrc: Add > > camera-name property", a trailing whitespace in "gst: libcamerapad: > > Allow storing a pool", and an extra blank line at end of file in "gst: > > Add initial device provider" (this one is reported by git-am only). It's > > very little, you did a good job :-) If you fix those small issues I > > think we'll be good to go from a coding style point of view. > > Sure, I'll be going to the patches one by one to apply the fixes, and > will update. Let me know what the team wants for the declaration vs > definition style. I don't mind the exception to the coding style, but if clang-format report issues, we should then add a configuration file to this directory. Otherwise development becomes painful with too many false positives. > > > +{ > > > + return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY, > > > + GST_TYPE_LIBCAMERA_SRC); > > > + return TRUE; > > > +} > > > + > > > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR, > > > + libcamera, "LibCamera capture plugin", > > > > libcamera is always written in all lowercase (it's part of the ascii art > > branding :-)). This comment applies to all patches in this series. > > > > > + plugin_init, VERSION, "LGPL", PACKAGE, "https://libcamera.org"); > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > new file mode 100644 > > > index 0000000..39a06a4 > > > --- /dev/null > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > @@ -0,0 +1,31 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Collabora Ltd. > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + * > > > + * gstlibcamerasrc.cpp - GStreamer Capture Element > > > + */ > > > + > > > +#include "gstlibcamerasrc.h" > > > + > > > +struct _GstLibcameraSrc { > > > > This structure doesn't seem to be used, do I understand correctly that > > it supports the G_DEFINE_TYPE() below through some magic that is, in > > glib terms, akin to C++ templates ? :-) > > This is the instance structure, so this is what you will point to when > you create that object. At this very early stage, it only used for > allocation purpose by GObject library (through G_DEFINE_TYPE()). > > > > + GstElement parent; > > > +}; > > > + > > > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT); > > > + > > > +static void > > > +gst_libcamera_src_init(GstLibcameraSrc *self) > > > > Same for this function ? > > In C++, would be called a default constructor. It's called by GObject, > and it's not optional with helpers like G_DEFINE_TYPE. > > > > +{ > > > +} > > > + > > > +static void > > > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > > +{ > > > + GstElementClass *element_class = (GstElementClass *)klass; > > > + > > > + gst_element_class_set_metadata(element_class, > > > + "LibCamera Source", "Source/Video", > > > + "Linux Camera source using libcamera", > > > + "Nicolas Dufresne <nicolas.dufresne@collabora.com"); > > > +} > > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h > > > new file mode 100644 > > > index 0000000..6d2f794 > > > --- /dev/null > > > +++ b/src/gstreamer/gstlibcamerasrc.h > > > @@ -0,0 +1,22 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2019, Collabora Ltd. > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + * > > > + * gstlibcamerasrc.h - GStreamer Capture Element > > > + */ > > > + > > > +#include <gst/gst.h> > > > + > > > > You can move this after the #ifndef / #define guard, it should reduce > > compilation time (very slightly). > > > > > +#ifndef __GST_LIBCAMERA_SRC_H__ > > > +#define __GST_LIBCAMERA_SRC_H__ > > > + > > > +G_BEGIN_DECLS > > > + > > > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type() > > > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src, > > > + GST_LIBCAMERA, SRC, GstElement) > > > + > > > +G_END_DECLS > > > + > > > +#endif /* __GST_LIBCAMERA_SRC_H__ */ > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > > new file mode 100644 > > > index 0000000..32d4233 > > > --- /dev/null > > > +++ b/src/gstreamer/meson.build > > > @@ -0,0 +1,23 @@ > > > +libcamera_gst_sources = [ > > > + 'gstlibcamera.c', > > > + 'gstlibcamerasrc.cpp', > > > +] > > > + > > > +libcamera_gst_c_args = [ > > > + '-DVERSION="@0@"'.format(libcamera_git_version), > > > + '-DPACKAGE="@0@"'.format(meson.project_name()), > > > +] > > > + > > > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1', > > > + required : get_option('gstreamer')) > > > > Could you please align this with the ( ? > > > > > + > > > +if gst_dep.found() > > > + libcamera_gst = shared_library('gstlibcamera', > > > > We use 4 spaces to indent meson.build files. > > > > > + libcamera_gst_sources, > > > + c_args : libcamera_gst_c_args, > > > + include_directories : [], > > > > Do you need to specify an empty array explicitly here, can't you remove > > the parameter ? > > > > > + dependencies : [libcamera_dep, gst_dep], > > > + install: true, > > > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), > > > + ) > > > +endif > > > diff --git a/src/meson.build b/src/meson.build > > > index 5adcd61..d818d8b 100644 > > > --- a/src/meson.build > > > +++ b/src/meson.build > > > @@ -10,3 +10,5 @@ subdir('qcam') > > > if get_option('v4l2') > > > subdir('v4l2') > > > endif > > > + > > > +subdir('gstreamer') > > > > It feels good to see the skeleton, knowing there are 22 patches > > following it :-)
Le mardi 25 février 2020 à 18:23 +0200, Laurent Pinchart a écrit : > Hi Nicolas, > > On Tue, Feb 25, 2020 at 10:38:41AM -0500, Nicolas Dufresne wrote: > > Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit : > > > On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote: > > > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > --- > > > > meson_options.txt | 5 +++++ > > > > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++ > > > > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++ > > > > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++ > > > > src/gstreamer/meson.build | 23 +++++++++++++++++++++++ > > > > src/meson.build | 2 ++ > > > > 6 files changed, 104 insertions(+) > > > > create mode 100644 src/gstreamer/gstlibcamera.c > > > > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp > > > > create mode 100644 src/gstreamer/gstlibcamerasrc.h > > > > create mode 100644 src/gstreamer/meson.build > > > > > > > > diff --git a/meson_options.txt b/meson_options.txt > > > > index 79ee4de..1b78ed8 100644 > > > > --- a/meson_options.txt > > > > +++ b/meson_options.txt > > > > @@ -15,3 +15,8 @@ option('v4l2', > > > > type : 'boolean', > > > > value : false, > > > > description : 'Compile the V4L2 compatibility layer') > > > > + > > > > +option('gstreamer', > > > > > > Should we keep options alphabetically sorted ? It always bothers my OCD > > > otherwise as I never know where to insert new options in an unsorted > > > list :-) > > > > > > > + type : 'feature', > > > > + value : 'auto', > > > > + description : 'Compile libcamera GStreamer plugin') > > > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c > > > > new file mode 100644 > > > > index 0000000..953fb29 > > > > --- /dev/null > > > > +++ b/src/gstreamer/gstlibcamera.c > > > > @@ -0,0 +1,21 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2019, Collabora Ltd. > > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > + * > > > > + * gstlibcamera.c - GStreamer plugin > > > > + */ > > > > + > > > > +#include "gstlibcamerasrc.h" > > > > + > > > > +static gboolean > > > > +plugin_init(GstPlugin *plugin) > > > > > > You don't have to break this line. > > > > This is GStreamer/GLib style, it has a really strong rational for > > searching C code. As you can with grep (or other regex) differentiate a > > definition from a declaration. > > > > > I've run checkstyle.py on your whole series, and the most common issue > > > it reports is the unneeded line break in function definitions. There's > > > > I don't recall seeing any warning about unneeded break other then these > > though, I'll recheck. > > > > > also an extra space after a function call in "gst: libcamerasrc: Add > > > camera-name property", a trailing whitespace in "gst: libcamerapad: > > > Allow storing a pool", and an extra blank line at end of file in "gst: > > > Add initial device provider" (this one is reported by git-am only). It's > > > very little, you did a good job :-) If you fix those small issues I > > > think we'll be good to go from a coding style point of view. > > > > Sure, I'll be going to the patches one by one to apply the fixes, and > > will update. Let me know what the team wants for the declaration vs > > definition style. > > I don't mind the exception to the coding style, but if clang-format > report issues, we should then add a configuration file to this > directory. Otherwise development becomes painful with too many false > positives. clang seems to be totally random in this regard. First few patches uses this pattern and clang won't complain. I think it complains if it understand the type. Maybe you have configured "decorators" like export or attribute stuff to allowed on the previous line. I'll keep cleaning up and I'll have a better idea. > > > > > +{ > > > > + return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY, > > > > + GST_TYPE_LIBCAMERA_SRC); > > > > + return TRUE; > > > > +} > > > > + > > > > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR, > > > > + libcamera, "LibCamera capture plugin", > > > > > > libcamera is always written in all lowercase (it's part of the ascii art > > > branding :-)). This comment applies to all patches in this series. > > > > > > > + plugin_init, VERSION, "LGPL", PACKAGE, "https://libcamera.org"); > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > > new file mode 100644 > > > > index 0000000..39a06a4 > > > > --- /dev/null > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > > @@ -0,0 +1,31 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2019, Collabora Ltd. > > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > + * > > > > + * gstlibcamerasrc.cpp - GStreamer Capture Element > > > > + */ > > > > + > > > > +#include "gstlibcamerasrc.h" > > > > + > > > > +struct _GstLibcameraSrc { > > > > > > This structure doesn't seem to be used, do I understand correctly that > > > it supports the G_DEFINE_TYPE() below through some magic that is, in > > > glib terms, akin to C++ templates ? :-) > > > > This is the instance structure, so this is what you will point to when > > you create that object. At this very early stage, it only used for > > allocation purpose by GObject library (through G_DEFINE_TYPE()). > > > > > > + GstElement parent; > > > > +}; > > > > + > > > > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT); > > > > + > > > > +static void > > > > +gst_libcamera_src_init(GstLibcameraSrc *self) > > > > > > Same for this function ? > > > > In C++, would be called a default constructor. It's called by GObject, > > and it's not optional with helpers like G_DEFINE_TYPE. > > > > > > +{ > > > > +} > > > > + > > > > +static void > > > > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) > > > > +{ > > > > + GstElementClass *element_class = (GstElementClass *)klass; > > > > + > > > > + gst_element_class_set_metadata(element_class, > > > > + "LibCamera Source", "Source/Video", > > > > + "Linux Camera source using libcamera", > > > > + "Nicolas Dufresne <nicolas.dufresne@collabora.com"); > > > > +} > > > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h > > > > new file mode 100644 > > > > index 0000000..6d2f794 > > > > --- /dev/null > > > > +++ b/src/gstreamer/gstlibcamerasrc.h > > > > @@ -0,0 +1,22 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2019, Collabora Ltd. > > > > + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > + * > > > > + * gstlibcamerasrc.h - GStreamer Capture Element > > > > + */ > > > > + > > > > +#include <gst/gst.h> > > > > + > > > > > > You can move this after the #ifndef / #define guard, it should reduce > > > compilation time (very slightly). > > > > > > > +#ifndef __GST_LIBCAMERA_SRC_H__ > > > > +#define __GST_LIBCAMERA_SRC_H__ > > > > + > > > > +G_BEGIN_DECLS > > > > + > > > > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type() > > > > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src, > > > > + GST_LIBCAMERA, SRC, GstElement) > > > > + > > > > +G_END_DECLS > > > > + > > > > +#endif /* __GST_LIBCAMERA_SRC_H__ */ > > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build > > > > new file mode 100644 > > > > index 0000000..32d4233 > > > > --- /dev/null > > > > +++ b/src/gstreamer/meson.build > > > > @@ -0,0 +1,23 @@ > > > > +libcamera_gst_sources = [ > > > > + 'gstlibcamera.c', > > > > + 'gstlibcamerasrc.cpp', > > > > +] > > > > + > > > > +libcamera_gst_c_args = [ > > > > + '-DVERSION="@0@"'.format(libcamera_git_version), > > > > + '-DPACKAGE="@0@"'.format(meson.project_name()), > > > > +] > > > > + > > > > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1', > > > > + required : get_option('gstreamer')) > > > > > > Could you please align this with the ( ? > > > > > > > + > > > > +if gst_dep.found() > > > > + libcamera_gst = shared_library('gstlibcamera', > > > > > > We use 4 spaces to indent meson.build files. > > > > > > > + libcamera_gst_sources, > > > > + c_args : libcamera_gst_c_args, > > > > + include_directories : [], > > > > > > Do you need to specify an empty array explicitly here, can't you remove > > > the parameter ? > > > > > > > + dependencies : [libcamera_dep, gst_dep], > > > > + install: true, > > > > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), > > > > + ) > > > > +endif > > > > diff --git a/src/meson.build b/src/meson.build > > > > index 5adcd61..d818d8b 100644 > > > > --- a/src/meson.build > > > > +++ b/src/meson.build > > > > @@ -10,3 +10,5 @@ subdir('qcam') > > > > if get_option('v4l2') > > > > subdir('v4l2') > > > > endif > > > > + > > > > +subdir('gstreamer') > > > > > > It feels good to see the skeleton, knowing there are 22 patches > > > following it :-)
diff --git a/meson_options.txt b/meson_options.txt index 79ee4de..1b78ed8 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -15,3 +15,8 @@ option('v4l2', type : 'boolean', value : false, description : 'Compile the V4L2 compatibility layer') + +option('gstreamer', + type : 'feature', + value : 'auto', + description : 'Compile libcamera GStreamer plugin') diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c new file mode 100644 index 0000000..953fb29 --- /dev/null +++ b/src/gstreamer/gstlibcamera.c @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Collabora Ltd. + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> + * + * gstlibcamera.c - GStreamer plugin + */ + +#include "gstlibcamerasrc.h" + +static gboolean +plugin_init(GstPlugin *plugin) +{ + return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY, + GST_TYPE_LIBCAMERA_SRC); + return TRUE; +} + +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR, + libcamera, "LibCamera capture plugin", + plugin_init, VERSION, "LGPL", PACKAGE, "https://libcamera.org"); diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp new file mode 100644 index 0000000..39a06a4 --- /dev/null +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -0,0 +1,31 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Collabora Ltd. + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> + * + * gstlibcamerasrc.cpp - GStreamer Capture Element + */ + +#include "gstlibcamerasrc.h" + +struct _GstLibcameraSrc { + GstElement parent; +}; + +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT); + +static void +gst_libcamera_src_init(GstLibcameraSrc *self) +{ +} + +static void +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass) +{ + GstElementClass *element_class = (GstElementClass *)klass; + + gst_element_class_set_metadata(element_class, + "LibCamera Source", "Source/Video", + "Linux Camera source using libcamera", + "Nicolas Dufresne <nicolas.dufresne@collabora.com"); +} diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h new file mode 100644 index 0000000..6d2f794 --- /dev/null +++ b/src/gstreamer/gstlibcamerasrc.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Collabora Ltd. + * Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> + * + * gstlibcamerasrc.h - GStreamer Capture Element + */ + +#include <gst/gst.h> + +#ifndef __GST_LIBCAMERA_SRC_H__ +#define __GST_LIBCAMERA_SRC_H__ + +G_BEGIN_DECLS + +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type() +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src, + GST_LIBCAMERA, SRC, GstElement) + +G_END_DECLS + +#endif /* __GST_LIBCAMERA_SRC_H__ */ diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build new file mode 100644 index 0000000..32d4233 --- /dev/null +++ b/src/gstreamer/meson.build @@ -0,0 +1,23 @@ +libcamera_gst_sources = [ + 'gstlibcamera.c', + 'gstlibcamerasrc.cpp', +] + +libcamera_gst_c_args = [ + '-DVERSION="@0@"'.format(libcamera_git_version), + '-DPACKAGE="@0@"'.format(meson.project_name()), +] + +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1', + required : get_option('gstreamer')) + +if gst_dep.found() + libcamera_gst = shared_library('gstlibcamera', + libcamera_gst_sources, + c_args : libcamera_gst_c_args, + include_directories : [], + dependencies : [libcamera_dep, gst_dep], + install: true, + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')), + ) +endif diff --git a/src/meson.build b/src/meson.build index 5adcd61..d818d8b 100644 --- a/src/meson.build +++ b/src/meson.build @@ -10,3 +10,5 @@ subdir('qcam') if get_option('v4l2') subdir('v4l2') endif + +subdir('gstreamer')