[{"id":3648,"web_url":"https://patchwork.libcamera.org/comment/3648/","msgid":"<20200210232825.GB21893@pendragon.ideasonboard.com>","date":"2020-02-10T23:28:25","subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> meson_options.txt | 5 +++++\n> src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++\n> src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++\n> src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++\n> src/gstreamer/meson.build | 23 +++++++++++++++++++++++\n> src/meson.build | 2 ++\n> 6 files changed, 104 insertions(+)\n> create mode 100644 src/gstreamer/gstlibcamera.c\n> create mode 100644 src/gstreamer/gstlibcamerasrc.cpp\n> create mode 100644 src/gstreamer/gstlibcamerasrc.h\n> create mode 100644 src/gstreamer/meson.build\n> \n> diff --git a/meson_options.txt b/meson_options.txt\n> index 79ee4de..1b78ed8 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -15,3 +15,8 @@ option('v4l2',\n> type : 'boolean',\n> value : false,\n> description : 'Compile the V4L2 compatibility layer')\n> +\n> +option('gstreamer',\n\nShould we keep options alphabetically sorted ? It always bothers my OCD\notherwise as I never know where to insert new options in an unsorted\nlist :-)\n\n> + type : 'feature',\n> + value : 'auto',\n> + description : 'Compile libcamera GStreamer plugin')\n> diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> new file mode 100644\n> index 0000000..953fb29\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamera.c\n> @@ -0,0 +1,21 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamera.c - GStreamer plugin\n> + */\n> +\n> +#include \"gstlibcamerasrc.h\"\n> +\n> +static gboolean\n> +plugin_init(GstPlugin *plugin)\n\nYou don't have to break this line.\n\nI've run checkstyle.py on your whole series, and the most common issue\nit reports is the unneeded line break in function definitions. There's\nalso an extra space after a function call in \"gst: libcamerasrc: Add\ncamera-name property\", a trailing whitespace in \"gst: libcamerapad:\nAllow storing a pool\", and an extra blank line at end of file in \"gst:\nAdd initial device provider\" (this one is reported by git-am only). It's\nvery little, you did a good job :-) If you fix those small issues I\nthink we'll be good to go from a coding style point of view.\n\n> +{\n> +\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> +\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> +\treturn TRUE;\n> +}\n> +\n> +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR,\n> +\t\t libcamera, \"LibCamera capture plugin\",\n\nlibcamera is always written in all lowercase (it's part of the ascii art\nbranding :-)). This comment applies to all patches in this series.\n\n> +\t\t plugin_init, VERSION, \"LGPL\", PACKAGE, \"https://libcamera.org\");\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> new file mode 100644\n> index 0000000..39a06a4\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -0,0 +1,31 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamerasrc.cpp - GStreamer Capture Element\n> + */\n> +\n> +#include \"gstlibcamerasrc.h\"\n> +\n> +struct _GstLibcameraSrc {\n\nThis structure doesn't seem to be used, do I understand correctly that\nit supports the G_DEFINE_TYPE() below through some magic that is, in\nglib terms, akin to C++ templates ? :-)\n\n> +\tGstElement parent;\n> +};\n> +\n> +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT);\n> +\n> +static void\n> +gst_libcamera_src_init(GstLibcameraSrc *self)\n\nSame for this function ?\n\n> +{\n> +}\n> +\n> +static void\n> +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> +{\n> +\tGstElementClass *element_class = (GstElementClass *)klass;\n> +\n> +\tgst_element_class_set_metadata(element_class,\n> +\t\t\t\t \"LibCamera Source\", \"Source/Video\",\n> +\t\t\t\t \"Linux Camera source using libcamera\",\n> +\t\t\t\t \"Nicolas Dufresne +}\n> diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n> new file mode 100644\n> index 0000000..6d2f794\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamerasrc.h\n> @@ -0,0 +1,22 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamerasrc.h - GStreamer Capture Element\n> + */\n> +\n> +#include \n> +\n\nYou can move this after the #ifndef / #define guard, it should reduce\ncompilation time (very slightly).\n\n> +#ifndef __GST_LIBCAMERA_SRC_H__\n> +#define __GST_LIBCAMERA_SRC_H__\n> +\n> +G_BEGIN_DECLS\n> +\n> +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type()\n> +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src,\n> +\t\t GST_LIBCAMERA, SRC, GstElement)\n> +\n> +G_END_DECLS\n> +\n> +#endif /* __GST_LIBCAMERA_SRC_H__ */\n> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> new file mode 100644\n> index 0000000..32d4233\n> --- /dev/null\n> +++ b/src/gstreamer/meson.build\n> @@ -0,0 +1,23 @@\n> +libcamera_gst_sources = [\n> + 'gstlibcamera.c',\n> + 'gstlibcamerasrc.cpp',\n> +]\n> +\n> +libcamera_gst_c_args = [\n> + '-DVERSION=\"@0@\"'.format(libcamera_git_version),\n> + '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> +]\n> +\n> +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> + required : get_option('gstreamer'))\n\nCould you please align this with the ( ?\n\n> +\n> +if gst_dep.found()\n> + libcamera_gst = shared_library('gstlibcamera',\n\nWe use 4 spaces to indent meson.build files.\n\n> + libcamera_gst_sources,\n> + c_args : libcamera_gst_c_args,\n> + include_directories : [],\n\nDo you need to specify an empty array explicitly here, can't you remove\nthe parameter ?\n\n> + dependencies : [libcamera_dep, gst_dep],\n> + install: true,\n> + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> + )\n> +endif\n> diff --git a/src/meson.build b/src/meson.build\n> index 5adcd61..d818d8b 100644\n> --- a/src/meson.build\n> +++ b/src/meson.build\n> @@ -10,3 +10,5 @@ subdir('qcam')\n> if get_option('v4l2')\n> subdir('v4l2')\n> endif\n> +\n> +subdir('gstreamer')\n\nIt feels good to see the skeleton, knowing there are 22 patches\nfollowing it :-)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5F3E9609D7\n\tfor ;\n\tTue, 11 Feb 2020 00:28:41 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EC0F152A;\n\tTue, 11 Feb 2020 00:28:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581377321;\n\tbh=EsUSCMQYoHxD4id1xwF4UfAUDV/Fs9TfXMgK0csIu34=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fEPbFPkBmbACYoNRGnln3wsMMOhFOBqbHHe0zyD3hyF0S/cBV2yrJ+Cg720Z7GK41\n\tkFpR4kUrqyuj/mwfn3gISCMVFamZYOggPMH+FyGvIfY6pcQ4Fi5Yrq9tw1UKya/fHk\n\t3gNoCH1aOPLUV49WW86LwkE4+Z7UH+f5HEESXGFk=","Date":"Tue, 11 Feb 2020 01:28:25 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200210232825.GB21893@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-2-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-2-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Feb 2020 23:28:41 -0000"}},{"id":3841,"web_url":"https://patchwork.libcamera.org/comment/3841/","msgid":"<07a20741ed09085a3ee97331c4e8db83af5c9616.camel@ndufresne.ca>","date":"2020-02-25T15:38:41","subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> Thank you for the patch.\n> \n> On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > meson_options.txt | 5 +++++\n> > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++\n> > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++\n> > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++\n> > src/gstreamer/meson.build | 23 +++++++++++++++++++++++\n> > src/meson.build | 2 ++\n> > 6 files changed, 104 insertions(+)\n> > create mode 100644 src/gstreamer/gstlibcamera.c\n> > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp\n> > create mode 100644 src/gstreamer/gstlibcamerasrc.h\n> > create mode 100644 src/gstreamer/meson.build\n> > \n> > diff --git a/meson_options.txt b/meson_options.txt\n> > index 79ee4de..1b78ed8 100644\n> > --- a/meson_options.txt\n> > +++ b/meson_options.txt\n> > @@ -15,3 +15,8 @@ option('v4l2',\n> > type : 'boolean',\n> > value : false,\n> > description : 'Compile the V4L2 compatibility layer')\n> > +\n> > +option('gstreamer',\n> \n> Should we keep options alphabetically sorted ? It always bothers my OCD\n> otherwise as I never know where to insert new options in an unsorted\n> list :-)\n> \n> > + type : 'feature',\n> > + value : 'auto',\n> > + description : 'Compile libcamera GStreamer plugin')\n> > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> > new file mode 100644\n> > index 0000000..953fb29\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamera.c\n> > @@ -0,0 +1,21 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamera.c - GStreamer plugin\n> > + */\n> > +\n> > +#include \"gstlibcamerasrc.h\"\n> > +\n> > +static gboolean\n> > +plugin_init(GstPlugin *plugin)\n> \n> You don't have to break this line.\n\nThis is GStreamer/GLib style, it has a really strong rational for\nsearching C code. As you can with grep (or other regex) differentiate a\ndefinition from a declaration.\n\n> \n> I've run checkstyle.py on your whole series, and the most common issue\n> it reports is the unneeded line break in function definitions. There's\n\nI don't recall seeing any warning about unneeded break other then these\nthough, I'll recheck.\n\n> also an extra space after a function call in \"gst: libcamerasrc: Add\n> camera-name property\", a trailing whitespace in \"gst: libcamerapad:\n> Allow storing a pool\", and an extra blank line at end of file in \"gst:\n> Add initial device provider\" (this one is reported by git-am only). It's\n> very little, you did a good job :-) If you fix those small issues I\n> think we'll be good to go from a coding style point of view.\n\nSure, I'll be going to the patches one by one to apply the fixes, and\nwill update. Let me know what the team wants for the declaration vs\ndefinition style. \n\n> \n> > +{\n> > +\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > +\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> > +\treturn TRUE;\n> > +}\n> > +\n> > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR,\n> > +\t\t libcamera, \"LibCamera capture plugin\",\n> \n> libcamera is always written in all lowercase (it's part of the ascii art\n> branding :-)). This comment applies to all patches in this series.\n> \n> > +\t\t plugin_init, VERSION, \"LGPL\", PACKAGE, \"https://libcamera.org\");\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > new file mode 100644\n> > index 0000000..39a06a4\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -0,0 +1,31 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerasrc.cpp - GStreamer Capture Element\n> > + */\n> > +\n> > +#include \"gstlibcamerasrc.h\"\n> > +\n> > +struct _GstLibcameraSrc {\n> \n> This structure doesn't seem to be used, do I understand correctly that\n> it supports the G_DEFINE_TYPE() below through some magic that is, in\n> glib terms, akin to C++ templates ? :-)\n\nThis is the instance structure, so this is what you will point to when\nyou create that object. At this very early stage, it only used for\nallocation purpose by GObject library (through G_DEFINE_TYPE()).\n\n> \n> > +\tGstElement parent;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT);\n> > +\n> > +static void\n> > +gst_libcamera_src_init(GstLibcameraSrc *self)\n> \n> Same for this function ?\n\nIn C++, would be called a default constructor. It's called by GObject,\nand it's not optional with helpers like G_DEFINE_TYPE.\n\n> \n> > +{\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > +{\n> > +\tGstElementClass *element_class = (GstElementClass *)klass;\n> > +\n> > +\tgst_element_class_set_metadata(element_class,\n> > +\t\t\t\t \"LibCamera Source\", \"Source/Video\",\n> > +\t\t\t\t \"Linux Camera source using libcamera\",\n> > +\t\t\t\t \"Nicolas Dufresne > +}\n> > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n> > new file mode 100644\n> > index 0000000..6d2f794\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamerasrc.h\n> > @@ -0,0 +1,22 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamerasrc.h - GStreamer Capture Element\n> > + */\n> > +\n> > +#include \n> > +\n> \n> You can move this after the #ifndef / #define guard, it should reduce\n> compilation time (very slightly).\n> \n> > +#ifndef __GST_LIBCAMERA_SRC_H__\n> > +#define __GST_LIBCAMERA_SRC_H__\n> > +\n> > +G_BEGIN_DECLS\n> > +\n> > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src,\n> > +\t\t GST_LIBCAMERA, SRC, GstElement)\n> > +\n> > +G_END_DECLS\n> > +\n> > +#endif /* __GST_LIBCAMERA_SRC_H__ */\n> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > new file mode 100644\n> > index 0000000..32d4233\n> > --- /dev/null\n> > +++ b/src/gstreamer/meson.build\n> > @@ -0,0 +1,23 @@\n> > +libcamera_gst_sources = [\n> > + 'gstlibcamera.c',\n> > + 'gstlibcamerasrc.cpp',\n> > +]\n> > +\n> > +libcamera_gst_c_args = [\n> > + '-DVERSION=\"@0@\"'.format(libcamera_git_version),\n> > + '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > +]\n> > +\n> > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > + required : get_option('gstreamer'))\n> \n> Could you please align this with the ( ?\n> \n> > +\n> > +if gst_dep.found()\n> > + libcamera_gst = shared_library('gstlibcamera',\n> \n> We use 4 spaces to indent meson.build files.\n> \n> > + libcamera_gst_sources,\n> > + c_args : libcamera_gst_c_args,\n> > + include_directories : [],\n> \n> Do you need to specify an empty array explicitly here, can't you remove\n> the parameter ?\n> \n> > + dependencies : [libcamera_dep, gst_dep],\n> > + install: true,\n> > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > + )\n> > +endif\n> > diff --git a/src/meson.build b/src/meson.build\n> > index 5adcd61..d818d8b 100644\n> > --- a/src/meson.build\n> > +++ b/src/meson.build\n> > @@ -10,3 +10,5 @@ subdir('qcam')\n> > if get_option('v4l2')\n> > subdir('v4l2')\n> > endif\n> > +\n> > +subdir('gstreamer')\n> \n> It feels good to see the skeleton, knowing there are 22 patches\n> following it :-)\n>","headers":{"Return-Path":"","Received":["from mail-qt1-x834.google.com (mail-qt1-x834.google.com\n\t[IPv6:2607:f8b0:4864:20::834])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0E366042E\n\tfor ;\n\tTue, 25 Feb 2020 16:38:44 +0100 (CET)","by mail-qt1-x834.google.com with SMTP id t13so9332124qto.3\n\tfor ;\n\tTue, 25 Feb 2020 07:38:44 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\tg18sm7560209qki.13.2020.02.25.07.38.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 25 Feb 2020 07:38:42 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=5ORFwsxdFUYXuwqIRBIwM26mj0AnYjrvihwdkXTvcSk=;\n\tb=vv4xpRizRgZL/ZaOLHr6GSgfr+Xq9LsGJjVQck8F6c7LNEZIMEuDAb6/E2NRPTPtn5\n\tOYvf+GyMryv60WZxDJkJbuY3N1YpsAkypETMe2OBiGToIom1FizEj1zimZjPX7FhPAbr\n\tQJCIN//d3niIqRy3TFMDivxPZWlJiCM/0Hq7/fy6kPknBe54GfooskrxUhZwdh3BguWM\n\t6s/KOekcxOf3YgE1gX7dLv6mLUCDpDGnchsRLHQIpAV9Lr8gW4hKFHjDTiUKSxvF0Uhi\n\tip0M0clbIza1ooinUZ7jhaYe2Y/GcwUoYeBGLtYF/kvhtbrt8E9k62HtaF4l0wP2aPn2\n\tZBgw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=5ORFwsxdFUYXuwqIRBIwM26mj0AnYjrvihwdkXTvcSk=;\n\tb=XDk4mSlS/Oed9A3T7zMsY+l3FO5BUSvXhVTrPGjeXtTZKaLaUY9YA2C7aCZJnr3NUD\n\tWUSIHNNvsBGQJO20LgVxSaHBVtsJFr8qlcvkKzjdfaO7VF11fG8YZ0FHZQ/NFbRXDpSt\n\tRuZv526i8m3yACV7fngtavqNIEpAJwMHr1WKHPsWpnKNHXszoXb4HGL3lv5XBRIVm/zZ\n\t8nxkSkAwkGicw8j7eMrugClLL9MZOS7bxnx9RbfhTsJq7s/s7dcusu6oA4pjh3Nq7ZcP\n\tRYdJl8+zD808BaQFQe96HtUHDirrXL40xidI3QoJUlxp3pCUJdh75PvIPHHXrxemqtyo\n\tzeMg==","X-Gm-Message-State":"APjAAAWOvPQuiCNl/ZsjQtEFsSVyxj0BEccPvIMbEzljupeT5gzpJiW9\n\tO4k2p51SXiss0dGvfiHfRBEivw==","X-Google-Smtp-Source":"APXvYqzE6O9+k+NItMqTmctKxasYFVAenUKjfUHODztZcaU72pIfqehXCLITv+cYU8zJBtFO8WN7Xw==","X-Received":"by 2002:ac8:5298:: with SMTP id s24mr2135942qtn.54.1582645123257;\n\tTue, 25 Feb 2020 07:38:43 -0800 (PST)","Message-ID":"<07a20741ed09085a3ee97331c4e8db83af5c9616.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 25 Feb 2020 10:38:41 -0500","In-Reply-To":"<20200210232825.GB21893@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-2-nicolas@ndufresne.ca>\n\t<20200210232825.GB21893@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 25 Feb 2020 15:38:45 -0000"}},{"id":3842,"web_url":"https://patchwork.libcamera.org/comment/3842/","msgid":"<20200225162348.GM4764@pendragon.ideasonboard.com>","date":"2020-02-25T16:23:48","subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Tue, Feb 25, 2020 at 10:38:41AM -0500, Nicolas Dufresne wrote:\n> Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit :\n> > On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne \n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > meson_options.txt | 5 +++++\n> > > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++\n> > > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++\n> > > src/gstreamer/meson.build | 23 +++++++++++++++++++++++\n> > > src/meson.build | 2 ++\n> > > 6 files changed, 104 insertions(+)\n> > > create mode 100644 src/gstreamer/gstlibcamera.c\n> > > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp\n> > > create mode 100644 src/gstreamer/gstlibcamerasrc.h\n> > > create mode 100644 src/gstreamer/meson.build\n> > > \n> > > diff --git a/meson_options.txt b/meson_options.txt\n> > > index 79ee4de..1b78ed8 100644\n> > > --- a/meson_options.txt\n> > > +++ b/meson_options.txt\n> > > @@ -15,3 +15,8 @@ option('v4l2',\n> > > type : 'boolean',\n> > > value : false,\n> > > description : 'Compile the V4L2 compatibility layer')\n> > > +\n> > > +option('gstreamer',\n> > \n> > Should we keep options alphabetically sorted ? It always bothers my OCD\n> > otherwise as I never know where to insert new options in an unsorted\n> > list :-)\n> > \n> > > + type : 'feature',\n> > > + value : 'auto',\n> > > + description : 'Compile libcamera GStreamer plugin')\n> > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> > > new file mode 100644\n> > > index 0000000..953fb29\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamera.c\n> > > @@ -0,0 +1,21 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamera.c - GStreamer plugin\n> > > + */\n> > > +\n> > > +#include \"gstlibcamerasrc.h\"\n> > > +\n> > > +static gboolean\n> > > +plugin_init(GstPlugin *plugin)\n> > \n> > You don't have to break this line.\n> \n> This is GStreamer/GLib style, it has a really strong rational for\n> searching C code. As you can with grep (or other regex) differentiate a\n> definition from a declaration.\n> \n> > I've run checkstyle.py on your whole series, and the most common issue\n> > it reports is the unneeded line break in function definitions. There's\n> \n> I don't recall seeing any warning about unneeded break other then these\n> though, I'll recheck.\n>\n> > also an extra space after a function call in \"gst: libcamerasrc: Add\n> > camera-name property\", a trailing whitespace in \"gst: libcamerapad:\n> > Allow storing a pool\", and an extra blank line at end of file in \"gst:\n> > Add initial device provider\" (this one is reported by git-am only). It's\n> > very little, you did a good job :-) If you fix those small issues I\n> > think we'll be good to go from a coding style point of view.\n> \n> Sure, I'll be going to the patches one by one to apply the fixes, and\n> will update. Let me know what the team wants for the declaration vs\n> definition style. \n\nI don't mind the exception to the coding style, but if clang-format\nreport issues, we should then add a configuration file to this\ndirectory. Otherwise development becomes painful with too many false\npositives.\n\n> > > +{\n> > > +\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > > +\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> > > +\treturn TRUE;\n> > > +}\n> > > +\n> > > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR,\n> > > +\t\t libcamera, \"LibCamera capture plugin\",\n> > \n> > libcamera is always written in all lowercase (it's part of the ascii art\n> > branding :-)). This comment applies to all patches in this series.\n> > \n> > > +\t\t plugin_init, VERSION, \"LGPL\", PACKAGE, \"https://libcamera.org\");\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > new file mode 100644\n> > > index 0000000..39a06a4\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -0,0 +1,31 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamerasrc.cpp - GStreamer Capture Element\n> > > + */\n> > > +\n> > > +#include \"gstlibcamerasrc.h\"\n> > > +\n> > > +struct _GstLibcameraSrc {\n> > \n> > This structure doesn't seem to be used, do I understand correctly that\n> > it supports the G_DEFINE_TYPE() below through some magic that is, in\n> > glib terms, akin to C++ templates ? :-)\n> \n> This is the instance structure, so this is what you will point to when\n> you create that object. At this very early stage, it only used for\n> allocation purpose by GObject library (through G_DEFINE_TYPE()).\n> \n> > > +\tGstElement parent;\n> > > +};\n> > > +\n> > > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT);\n> > > +\n> > > +static void\n> > > +gst_libcamera_src_init(GstLibcameraSrc *self)\n> > \n> > Same for this function ?\n> \n> In C++, would be called a default constructor. It's called by GObject,\n> and it's not optional with helpers like G_DEFINE_TYPE.\n> \n> > > +{\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > +{\n> > > +\tGstElementClass *element_class = (GstElementClass *)klass;\n> > > +\n> > > +\tgst_element_class_set_metadata(element_class,\n> > > +\t\t\t\t \"LibCamera Source\", \"Source/Video\",\n> > > +\t\t\t\t \"Linux Camera source using libcamera\",\n> > > +\t\t\t\t \"Nicolas Dufresne > > +}\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n> > > new file mode 100644\n> > > index 0000000..6d2f794\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamerasrc.h\n> > > @@ -0,0 +1,22 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2019, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamerasrc.h - GStreamer Capture Element\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > \n> > You can move this after the #ifndef / #define guard, it should reduce\n> > compilation time (very slightly).\n> > \n> > > +#ifndef __GST_LIBCAMERA_SRC_H__\n> > > +#define __GST_LIBCAMERA_SRC_H__\n> > > +\n> > > +G_BEGIN_DECLS\n> > > +\n> > > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type()\n> > > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src,\n> > > +\t\t GST_LIBCAMERA, SRC, GstElement)\n> > > +\n> > > +G_END_DECLS\n> > > +\n> > > +#endif /* __GST_LIBCAMERA_SRC_H__ */\n> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > new file mode 100644\n> > > index 0000000..32d4233\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/meson.build\n> > > @@ -0,0 +1,23 @@\n> > > +libcamera_gst_sources = [\n> > > + 'gstlibcamera.c',\n> > > + 'gstlibcamerasrc.cpp',\n> > > +]\n> > > +\n> > > +libcamera_gst_c_args = [\n> > > + '-DVERSION=\"@0@\"'.format(libcamera_git_version),\n> > > + '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > > +]\n> > > +\n> > > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > > + required : get_option('gstreamer'))\n> > \n> > Could you please align this with the ( ?\n> > \n> > > +\n> > > +if gst_dep.found()\n> > > + libcamera_gst = shared_library('gstlibcamera',\n> > \n> > We use 4 spaces to indent meson.build files.\n> > \n> > > + libcamera_gst_sources,\n> > > + c_args : libcamera_gst_c_args,\n> > > + include_directories : [],\n> > \n> > Do you need to specify an empty array explicitly here, can't you remove\n> > the parameter ?\n> > \n> > > + dependencies : [libcamera_dep, gst_dep],\n> > > + install: true,\n> > > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > > + )\n> > > +endif\n> > > diff --git a/src/meson.build b/src/meson.build\n> > > index 5adcd61..d818d8b 100644\n> > > --- a/src/meson.build\n> > > +++ b/src/meson.build\n> > > @@ -10,3 +10,5 @@ subdir('qcam')\n> > > if get_option('v4l2')\n> > > subdir('v4l2')\n> > > endif\n> > > +\n> > > +subdir('gstreamer')\n> > \n> > It feels good to see the skeleton, knowing there are 22 patches\n> > following it :-)","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6824D6042E\n\tfor ;\n\tTue, 25 Feb 2020 17:24:09 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFF4D43F;\n\tTue, 25 Feb 2020 17:24:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582647849;\n\tbh=AmR9VyPypiWzJd88/Yle6wKO2qsXY/BMvD2yobUJPCw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=doeDvjdB3rnxaijcv/f6sEtdjIY7gdGCusXKwkj8eE+BpuZUcOndHLQeSVFTqJCFo\n\tMOSj/RHgDotyvPfrc48TjI/rXsZKJ0qwMxmiuLFu9VaDAflT31ZqQ2+kyUT5DrnrYf\n\tnZvNVKpDT/lJAlAGyqB4rSc1vxuKnuOJUsZxo6vU=","Date":"Tue, 25 Feb 2020 18:23:48 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200225162348.GM4764@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-2-nicolas@ndufresne.ca>\n\t<20200210232825.GB21893@pendragon.ideasonboard.com>\n\t<07a20741ed09085a3ee97331c4e8db83af5c9616.camel@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<07a20741ed09085a3ee97331c4e8db83af5c9616.camel@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 25 Feb 2020 16:24:09 -0000"}},{"id":3843,"web_url":"https://patchwork.libcamera.org/comment/3843/","msgid":"<55920119578dffa14bb45e5e4477da6ffcb2add4.camel@ndufresne.ca>","date":"2020-02-25T16:52:30","subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mardi 25 février 2020 à 18:23 +0200, Laurent Pinchart a écrit :\n> Hi Nicolas,\n> \n> On Tue, Feb 25, 2020 at 10:38:41AM -0500, Nicolas Dufresne wrote:\n> > Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit :\n> > > On Tue, Jan 28, 2020 at 10:31:48PM -0500, Nicolas Dufresne wrote:\n> > > > From: Nicolas Dufresne \n> > > > \n> > > > Signed-off-by: Nicolas Dufresne \n> > > > ---\n> > > > meson_options.txt | 5 +++++\n> > > > src/gstreamer/gstlibcamera.c | 21 +++++++++++++++++++++\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 31 +++++++++++++++++++++++++++++++\n> > > > src/gstreamer/gstlibcamerasrc.h | 22 ++++++++++++++++++++++\n> > > > src/gstreamer/meson.build | 23 +++++++++++++++++++++++\n> > > > src/meson.build | 2 ++\n> > > > 6 files changed, 104 insertions(+)\n> > > > create mode 100644 src/gstreamer/gstlibcamera.c\n> > > > create mode 100644 src/gstreamer/gstlibcamerasrc.cpp\n> > > > create mode 100644 src/gstreamer/gstlibcamerasrc.h\n> > > > create mode 100644 src/gstreamer/meson.build\n> > > > \n> > > > diff --git a/meson_options.txt b/meson_options.txt\n> > > > index 79ee4de..1b78ed8 100644\n> > > > --- a/meson_options.txt\n> > > > +++ b/meson_options.txt\n> > > > @@ -15,3 +15,8 @@ option('v4l2',\n> > > > type : 'boolean',\n> > > > value : false,\n> > > > description : 'Compile the V4L2 compatibility layer')\n> > > > +\n> > > > +option('gstreamer',\n> > > \n> > > Should we keep options alphabetically sorted ? It always bothers my OCD\n> > > otherwise as I never know where to insert new options in an unsorted\n> > > list :-)\n> > > \n> > > > + type : 'feature',\n> > > > + value : 'auto',\n> > > > + description : 'Compile libcamera GStreamer plugin')\n> > > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> > > > new file mode 100644\n> > > > index 0000000..953fb29\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/gstlibcamera.c\n> > > > @@ -0,0 +1,21 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Collabora Ltd.\n> > > > + * Author: Nicolas Dufresne \n> > > > + *\n> > > > + * gstlibcamera.c - GStreamer plugin\n> > > > + */\n> > > > +\n> > > > +#include \"gstlibcamerasrc.h\"\n> > > > +\n> > > > +static gboolean\n> > > > +plugin_init(GstPlugin *plugin)\n> > > \n> > > You don't have to break this line.\n> > \n> > This is GStreamer/GLib style, it has a really strong rational for\n> > searching C code. As you can with grep (or other regex) differentiate a\n> > definition from a declaration.\n> > \n> > > I've run checkstyle.py on your whole series, and the most common issue\n> > > it reports is the unneeded line break in function definitions. There's\n> > \n> > I don't recall seeing any warning about unneeded break other then these\n> > though, I'll recheck.\n> > \n> > > also an extra space after a function call in \"gst: libcamerasrc: Add\n> > > camera-name property\", a trailing whitespace in \"gst: libcamerapad:\n> > > Allow storing a pool\", and an extra blank line at end of file in \"gst:\n> > > Add initial device provider\" (this one is reported by git-am only). It's\n> > > very little, you did a good job :-) If you fix those small issues I\n> > > think we'll be good to go from a coding style point of view.\n> > \n> > Sure, I'll be going to the patches one by one to apply the fixes, and\n> > will update. Let me know what the team wants for the declaration vs\n> > definition style. \n> \n> I don't mind the exception to the coding style, but if clang-format\n> report issues, we should then add a configuration file to this\n> directory. Otherwise development becomes painful with too many false\n> positives.\n\nclang seems to be totally random in this regard. First few patches uses\nthis pattern and clang won't complain. I think it complains if it\nunderstand the type. Maybe you have configured \"decorators\" like export\nor attribute stuff to allowed on the previous line. I'll keep cleaning\nup and I'll have a better idea.\n\n> \n> > > > +{\n> > > > +\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > > > +\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> > > > +\treturn TRUE;\n> > > > +}\n> > > > +\n> > > > +GST_PLUGIN_DEFINE(GST_VERSION_MAJOR, GST_VERSION_MINOR,\n> > > > +\t\t libcamera, \"LibCamera capture plugin\",\n> > > \n> > > libcamera is always written in all lowercase (it's part of the ascii art\n> > > branding :-)). This comment applies to all patches in this series.\n> > > \n> > > > +\t\t plugin_init, VERSION, \"LGPL\", PACKAGE, \"https://libcamera.org\");\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > new file mode 100644\n> > > > index 0000000..39a06a4\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -0,0 +1,31 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Collabora Ltd.\n> > > > + * Author: Nicolas Dufresne \n> > > > + *\n> > > > + * gstlibcamerasrc.cpp - GStreamer Capture Element\n> > > > + */\n> > > > +\n> > > > +#include \"gstlibcamerasrc.h\"\n> > > > +\n> > > > +struct _GstLibcameraSrc {\n> > > \n> > > This structure doesn't seem to be used, do I understand correctly that\n> > > it supports the G_DEFINE_TYPE() below through some magic that is, in\n> > > glib terms, akin to C++ templates ? :-)\n> > \n> > This is the instance structure, so this is what you will point to when\n> > you create that object. At this very early stage, it only used for\n> > allocation purpose by GObject library (through G_DEFINE_TYPE()).\n> > \n> > > > +\tGstElement parent;\n> > > > +};\n> > > > +\n> > > > +G_DEFINE_TYPE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT);\n> > > > +\n> > > > +static void\n> > > > +gst_libcamera_src_init(GstLibcameraSrc *self)\n> > > \n> > > Same for this function ?\n> > \n> > In C++, would be called a default constructor. It's called by GObject,\n> > and it's not optional with helpers like G_DEFINE_TYPE.\n> > \n> > > > +{\n> > > > +}\n> > > > +\n> > > > +static void\n> > > > +gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > +{\n> > > > +\tGstElementClass *element_class = (GstElementClass *)klass;\n> > > > +\n> > > > +\tgst_element_class_set_metadata(element_class,\n> > > > +\t\t\t\t \"LibCamera Source\", \"Source/Video\",\n> > > > +\t\t\t\t \"Linux Camera source using libcamera\",\n> > > > +\t\t\t\t \"Nicolas Dufresne > > > +}\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.h b/src/gstreamer/gstlibcamerasrc.h\n> > > > new file mode 100644\n> > > > index 0000000..6d2f794\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.h\n> > > > @@ -0,0 +1,22 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Collabora Ltd.\n> > > > + * Author: Nicolas Dufresne \n> > > > + *\n> > > > + * gstlibcamerasrc.h - GStreamer Capture Element\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +\n> > > \n> > > You can move this after the #ifndef / #define guard, it should reduce\n> > > compilation time (very slightly).\n> > > \n> > > > +#ifndef __GST_LIBCAMERA_SRC_H__\n> > > > +#define __GST_LIBCAMERA_SRC_H__\n> > > > +\n> > > > +G_BEGIN_DECLS\n> > > > +\n> > > > +#define GST_TYPE_LIBCAMERA_SRC gst_libcamera_src_get_type()\n> > > > +G_DECLARE_FINAL_TYPE(GstLibcameraSrc, gst_libcamera_src,\n> > > > +\t\t GST_LIBCAMERA, SRC, GstElement)\n> > > > +\n> > > > +G_END_DECLS\n> > > > +\n> > > > +#endif /* __GST_LIBCAMERA_SRC_H__ */\n> > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > > new file mode 100644\n> > > > index 0000000..32d4233\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/meson.build\n> > > > @@ -0,0 +1,23 @@\n> > > > +libcamera_gst_sources = [\n> > > > + 'gstlibcamera.c',\n> > > > + 'gstlibcamerasrc.cpp',\n> > > > +]\n> > > > +\n> > > > +libcamera_gst_c_args = [\n> > > > + '-DVERSION=\"@0@\"'.format(libcamera_git_version),\n> > > > + '-DPACKAGE=\"@0@\"'.format(meson.project_name()),\n> > > > +]\n> > > > +\n> > > > +gst_dep = dependency('gstreamer-video-1.0', version : '>=1.16.1',\n> > > > + required : get_option('gstreamer'))\n> > > \n> > > Could you please align this with the ( ?\n> > > \n> > > > +\n> > > > +if gst_dep.found()\n> > > > + libcamera_gst = shared_library('gstlibcamera',\n> > > \n> > > We use 4 spaces to indent meson.build files.\n> > > \n> > > > + libcamera_gst_sources,\n> > > > + c_args : libcamera_gst_c_args,\n> > > > + include_directories : [],\n> > > \n> > > Do you need to specify an empty array explicitly here, can't you remove\n> > > the parameter ?\n> > > \n> > > > + dependencies : [libcamera_dep, gst_dep],\n> > > > + install: true,\n> > > > + install_dir : '@0@/gstreamer-1.0'.format(get_option('libdir')),\n> > > > + )\n> > > > +endif\n> > > > diff --git a/src/meson.build b/src/meson.build\n> > > > index 5adcd61..d818d8b 100644\n> > > > --- a/src/meson.build\n> > > > +++ b/src/meson.build\n> > > > @@ -10,3 +10,5 @@ subdir('qcam')\n> > > > if get_option('v4l2')\n> > > > subdir('v4l2')\n> > > > endif\n> > > > +\n> > > > +subdir('gstreamer')\n> > > \n> > > It feels good to see the skeleton, knowing there are 22 patches\n> > > following it :-)","headers":{"Return-Path":"","Received":["from mail-qk1-x744.google.com (mail-qk1-x744.google.com\n\t[IPv6:2607:f8b0:4864:20::744])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 647676042E\n\tfor ;\n\tTue, 25 Feb 2020 17:52:33 +0100 (CET)","by mail-qk1-x744.google.com with SMTP id 145so6344415qkl.2\n\tfor ;\n\tTue, 25 Feb 2020 08:52:33 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\ti14sm1111860qtm.60.2020.02.25.08.52.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 25 Feb 2020 08:52:31 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=message-id:subject:from:to:cc:date:in-reply-to:references\n\t:user-agent:mime-version:content-transfer-encoding;\n\tbh=XPdUgax1Xpm5lIZzjvE7bBvSJRGXTZPhLdD6kseIJb0=;\n\tb=zro2kney5XYwClAd2TGWLNoeL6E/ZKt4C0EVt5f70/yfMtcCKCoAFgRQ2VNBiRVP5h\n\t0XQzpkM8nO84Dk8Ih5gsmPbhnXsiqt0nZ91CbohB2EDxPfS/rGTa4Z7tab/rDflGekzN\n\tHE41JKc3aHotR6QqttctoqAzfvVFjzYflmnozg0pLR8uAaSNXGc7Kz0AuElGD0WqhvQT\n\texA/R7Q0ozQjo3xfouZPbaXILzzz8HH2eyjEFmcdl/TjRDdrmuT4GNwjxYEe2BpLUNQL\n\tXVZIeENi1g3RT8haDSsHicsUwukNtF+zbMagMkeU/NuZ/0tpRn/p+0m3s7KUPuow1sxJ\n\tFhKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to\n\t:references:user-agent:mime-version:content-transfer-encoding;\n\tbh=XPdUgax1Xpm5lIZzjvE7bBvSJRGXTZPhLdD6kseIJb0=;\n\tb=HS/9AoFqRERfy84Bctkt6srp3WrpQdAaT+KzGQXVlOsQxdbaU5a2SMBuqOq4TJjUE6\n\tE/Lut5hQm0u7fyuTne3DJ0E9DMikhj5olU2O+z5JMDgxKMwV3jM+kli4HgM+rFKm7icT\n\tbOK5/k+6dkXjKtVv0js6FFDvaSsEoLcGh5m08Q9HCdtMSCFuaqqSUOpYU4CL9vxMD34j\n\t6UGunBTAR8YAgVh2FMQzdudOdQ2OuUQ5RjUWGMjccr2c0MamUYe8uYkIDr9SVCCVg5bR\n\tvjtsnef65tdwr+iKd2K1WvN3eDOGswRaejBut6SMurV1m4J+Y2/QKZaF+S94pwHLcwdn\n\tD5qw==","X-Gm-Message-State":"APjAAAXov6uNNyNFx12xKljlpz6+8t+E6Xn3pCHV/KDZU2NX3fzwsjL2\n\t+exET4e6TllMEdk0jN+OeT49fw==","X-Google-Smtp-Source":"APXvYqxX7/7YFwHpdpRMD8exMmzynLsGeZTXxig491wRCJ7rGnmkY7L1lJg9CyWy2XqDjz6p5z87bw==","X-Received":"by 2002:a37:9b8b:: with SMTP id\n\td133mr56791260qke.147.1582649551875; \n\tTue, 25 Feb 2020 08:52:31 -0800 (PST)","Message-ID":"<55920119578dffa14bb45e5e4477da6ffcb2add4.camel@ndufresne.ca>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 25 Feb 2020 11:52:30 -0500","In-Reply-To":"<20200225162348.GM4764@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-2-nicolas@ndufresne.ca>\n\t<20200210232825.GB21893@pendragon.ideasonboard.com>\n\t<07a20741ed09085a3ee97331c4e8db83af5c9616.camel@ndufresne.ca>\n\t<20200225162348.GM4764@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 01/23] Add GStreamer plugin and\n\telement skeleton","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 25 Feb 2020 16:52:33 -0000"}}]