[libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and element skeleton
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 25 17:23:48 CET 2020
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 at collabora.com>
> > >
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at 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 at 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 at 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 at 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 at 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 :-)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list