[libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and element skeleton

[Laurent Pinchart]

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 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.

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 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 ? :-)

> +	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 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