[libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and element skeleton
Nicolas Dufresne
nicolas at ndufresne.ca
Tue Feb 25 17:52:30 CET 2020
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 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.
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 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 :-)
More information about the libcamera-devel
mailing list