[{"id":3647,"web_url":"https://patchwork.libcamera.org/comment/3647/","msgid":"<20200210232816.GA21893@pendragon.ideasonboard.com>","date":"2020-02-10T23:28:16","subject":"Re: [libcamera-devel] [PATCH v1 03/23] gst: Add initial device\n\tprovider","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:50PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> This feature is used with GstDeviceMonitor in order to enumerate\n> and monitor devices to be used with the source element. The resulting\n> GstDevice implementation is also used by application to abstract the\n> configuration of the source element.\n> \n> Implementations notes:\n\ns/Implementations/Implementation/\n\n> - libcamera does not support polling yet\n> - The device ID isn't unique in libcamera\n\nyet :-)\n\n> - The \"name\" property does not yet exist in libcamerasrc\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamera.c | 10 +-\n> src/gstreamer/gstlibcameraprovider.cpp | 227 +++++++++++++++++++++++++\n> src/gstreamer/gstlibcameraprovider.h | 23 +++\n> src/gstreamer/meson.build | 1 +\n> 4 files changed, 259 insertions(+), 2 deletions(-)\n> create mode 100644 src/gstreamer/gstlibcameraprovider.cpp\n> create mode 100644 src/gstreamer/gstlibcameraprovider.h\n> \n> diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> index 953fb29..7356374 100644\n> --- a/src/gstreamer/gstlibcamera.c\n> +++ b/src/gstreamer/gstlibcamera.c\n> @@ -7,12 +7,18 @@\n> */\n> \n> #include \"gstlibcamerasrc.h\"\n> +#include \"gstlibcameraprovider.h\"\n\nCould you please keep the headers alphabetically sorted ? Please see\nDocumentation/coding-style.rst, section \"Order of Includes\".\n\n> \n> static gboolean\n> plugin_init(GstPlugin *plugin)\n> {\n> -\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> -\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> +\tif (!gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> +\t\t\t\t GST_TYPE_LIBCAMERA_SRC) ||\n> +\t !gst_device_provider_register(plugin, \"libcameraprovider\",\n> +\t\t\t\t\t GST_RANK_PRIMARY,\n> +\t\t\t\t\t GST_TYPE_LIBCAMERA_PROVIDER))\n> +\t\treturn FALSE;\n> +\n> \treturn TRUE;\n> }\n> \n> diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> new file mode 100644\n> index 0000000..3e27912\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> @@ -0,0 +1,227 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcameraprovider.c - GStreamer Device Provider\n> + */\n> +\n> +#include \"gstlibcamera-utils.h\"\n> +#include \"gstlibcameraprovider.h\"\n> +#include \"gstlibcamerasrc.h\"\n> +\n> +#include \n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> +#define GST_CAT_DEFAULT provider_debug\n> +\n> +/**************************************/\n> +/* GstLibcameraDevice internal Object */\n> +/**************************************/\n\nIs this standard gstreamer coding style ? Would it make sense to instead\nuse a doxygen-style comment with a bit of documentation to describe the\nobject ?\n\n/**\n * \\struct _GstLibcameraDevice\n * \\brief ...\n *\n * This structure describes ...\n */\n\nIt could be useful for developers who are less familiar with gstreamer\n(that is, all the rest of us :-)).\n\n> +\n> +enum {\n> +\tPROP_DEVICE_NAME = 1,\n> +};\n> +\n> +#define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> +G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> +\t\t GST_LIBCAMERA, DEVICE, GstDevice);\n> +\n> +struct _GstLibcameraDevice {\n> +\tGstDevice parent;\n> +\tgchar *name;\n> +};\n> +\n> +G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE);\n> +\n> +static GstElement *\n> +gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> +{\n> +\tGstElement *source;\n> +\n> +\tsource = gst_element_factory_make(\"libcamerasrc\", name);\n> +\tg_assert(source);\n> +\n> +\tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> +\n> +\treturn source;\n> +}\n> +\n> +static gboolean\n> +gst_libcamera_device_reconfigure_element(GstDevice *device,\n> +\t\t\t\t\t GstElement *element)\n> +{\n> +\tif (!GST_LIBCAMERA_IS_SRC(element))\n> +\t\treturn FALSE;\n> +\n> +\tg_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> +\n> +\treturn TRUE;\n> +}\n> +\n> +static void\n> +gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> +\t\t\t\t const GValue *value, GParamSpec *pspec)\n> +{\n> +\tGstLibcameraDevice *device;\n> +\n> +\tdevice = GST_LIBCAMERA_DEVICE(object);\n\nYou could merge the two lines:\n\n\tGstLibcameraDevice *device = = GST_LIBCAMERA_DEVICE(object);\n\n> +\n> +\tswitch (prop_id) {\n> +\tcase PROP_DEVICE_NAME:\n> +\t\tdevice->name = g_value_dup_string(value);\n> +\t\tbreak;\n> +\tdefault:\n> +\t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n> +static void\n> +gst_libcamera_device_init(GstLibcameraDevice *self)\n> +{\n> +}\n> +\n> +static void\n> +gst_libcamera_device_finalize(GObject *object)\n> +{\n> +\tGstLibcameraDevice *self = GST_LIBCAMERA_DEVICE(object);\n> +\tgpointer klass = gst_libcamera_device_parent_class;\n> +\n> +\tg_free(self->name);\n> +\n> +\tG_OBJECT_GET_CLASS(klass)->finalize(object);\n> +}\n> +\n> +static void\n> +gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> +{\n> +\tGParamSpec *pspec;\n\nCan't this be named just \"spec\" ?\n\n> +\tGstDeviceClass *device_class = (GstDeviceClass *)klass;\n\nSame question as for the previous patch, should this be deviceClass ?\n\n> +\tGObjectClass *object_class = (GObjectClass *)klass;\n> +\n> +\tdevice_class->create_element = gst_libcamera_device_create_element;\n> +\tdevice_class->reconfigure_element = gst_libcamera_device_reconfigure_element;\n> +\n> +\tobject_class->set_property = gst_libcamera_device_set_property;\n> +\tobject_class->finalize = gst_libcamera_device_finalize;\n> +\n> +\tpspec = g_param_spec_string(\"name\", \"Name\",\n\nAs this is C++, we usually don't declare all variables at the top of\nfunctions, so this would become\n\n\tGParamSpec *pspec = g_param_spec_string(\"name\", \"Name\",\n\nUp to you.\n\n> +\t\t\t\t \"The name of the camera device\", \"\",\n> +\t\t\t\t (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> +\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> +\tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> +}\n> +\n> +static GstDevice *\n> +gst_libcamera_device_new(const std::shared_ptr &camera)\n> +{\n> +\tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> +\tg_autoptr(GstStructure) props = NULL;\n\nAs this is always null (as far as I can tell), does it need to be an\nauto pointer ?\n\n> +\tconst gchar *name = camera->name().c_str();\n> +\tStreamRoles roles;\n> +\n> +\troles.push_back(StreamRole::VideoRecording);\n> +\tstd::unique_ptr config = camera->generateConfiguration(roles);\n> +\n> +\tfor (const StreamConfiguration &stream_cfg : *config) {\n> +\t\tGstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> +\t\tif (sub_caps)\n> +\t\t\tgst_caps_append(caps, sub_caps);\n> +\t}\n> +\n> +\treturn (GstDevice *)g_object_new(GST_TYPE_LIBCAMERA_DEVICE,\n\nC++ code should use C++ casts instead of C-style casts. This would use\nstatic_cast (assuming that the GObject is the first member\nof GstDevice).\n\n> +\t\t\t\t\t /* FIXME not sure name is unique */\n> +\t\t\t\t\t \"name\", name,\n> +\t\t\t\t\t \"display-name\", name,\n> +\t\t\t\t\t \"caps\", caps,\n> +\t\t\t\t\t \"device-class\", \"Source/Video\",\n> +\t\t\t\t\t \"properties\", props,\n> +\t\t\t\t\t NULL);\n> +}\n> +\n> +/*************************************/\n> +/* GstLibcameraDeviceProvider Object */\n> +/*************************************/\n> +\n> +struct _GstLibcameraProvider {\n> +\tGstDeviceProvider parent;\n> +\tCameraManager *cm;\n> +};\n> +\n> +G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,\n> +\t\t\tGST_TYPE_DEVICE_PROVIDER,\n> +\t\t\tGST_DEBUG_CATEGORY_INIT(provider_debug, \"libcamera-provider\", 0,\n> +\t\t\t\t\t\t\"LibCamera Device Provider\"));\n> +\n> +static GList *\n> +gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> +{\n> +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);\n> +\tCameraManager *cm = self->cm;\n> +\tGList *devices = NULL;\n> +\tgint ret;\n> +\n> +\tGST_INFO_OBJECT(self, \"Probing cameras using LibCamera\");\n> +\n> +\t/* FIXME as long as the manager isn't able to handle hot-plug, we need to\n> +\t * cycle start/stop here in order to ensure wthat we have an up to date\n\ns/wthat/that/\n\n> +\t * list */\n\nThis should be\n\n\t/*\n\t * FIXME as long as the manager isn't able to handle hot-plug, we need to\n\t * cycle start/stop here in order to ensure that we have an up to date\n\t * list.\n\t */\n\naccording to the coding style (period at the end, and opening and\nclosing markers on a line of their own). Even better would be to replace\nFIXME with \\todo as that's the marker we use through the library.\n\n> +\tret = cm->start();\n> +\tif (ret) {\n> +\t\tGST_ERROR_OBJECT(self, \"Failed to retrieve device list: %s\",\n> +\t\t\t\t g_strerror(-ret));\n> +\t\treturn NULL;\n> +\t}\n> +\n> +\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> +\t\tGST_INFO_OBJECT(self, \"Found camera '%s'\", camera->name().c_str());\n> +\t\tdevices = g_list_append(devices,\n> +\t\t\t\t\tg_object_ref_sink(gst_libcamera_device_new(camera)));\n> +\t}\n> +\n> +\tcm->stop();\n> +\n> +\treturn devices;\n> +}\n> +\n> +static void\n> +gst_libcamera_provider_init(GstLibcameraProvider *self)\n> +{\n> +\tGstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);\n> +\n> +\tself->cm = new CameraManager();\n> +\n> +\t/* Avoid devices being duplicated */\n\ns/duplicated/duplicated./\n\n(This applies to the rest of the series.)\n\n> +\tgst_device_provider_hide_provider(provider, \"v4l2deviceprovider\");\n> +}\n> +\n> +static void\n> +gst_libcamera_provider_finalize(GObject *object)\n> +{\n> +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);\n> +\tgpointer klass = gst_libcamera_provider_parent_class;\n> +\n> +\tdelete self->cm;\n> +\n> +\treturn G_OBJECT_GET_CLASS(klass)->finalize(object);\n> +}\n> +\n> +static void\n> +gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)\n> +{\n> +\tGstDeviceProviderClass *provider_class = (GstDeviceProviderClass *)klass;\n> +\tGObjectClass *object_class = (GObjectClass *)klass;\n> +\n> +\tprovider_class->probe = gst_libcamera_provider_probe;\n> +\tobject_class->finalize = gst_libcamera_provider_finalize;\n> +\n> +\tgst_device_provider_class_set_metadata(provider_class,\n> +\t\t\t\t\t \"LibCamera Device Provider\",\n> +\t\t\t\t\t \"Source/Video\",\n> +\t\t\t\t\t \"List camera device using LibCamera\",\n> +\t\t\t\t\t \"Nicolas Dufresne \");\n> +}\n> diff --git a/src/gstreamer/gstlibcameraprovider.h b/src/gstreamer/gstlibcameraprovider.h\n> new file mode 100644\n> index 0000000..6dd232d\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcameraprovider.h\n> @@ -0,0 +1,23 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcameraprovider.h - GStreamer Device Provider\n> + */\n> +\n> +#include \n> +\n> +#ifndef __GST_LIBCAMERA_PROVIDER_H__\n> +#define __GST_LIBCAMERA_PROVIDER_H__\n> +\n> +G_BEGIN_DECLS\n> +\n> +#define GST_TYPE_LIBCAMERA_PROVIDER gst_libcamera_provider_get_type()\n> +G_DECLARE_FINAL_TYPE(GstLibcameraProvider, gst_libcamera_provider,\n> +\t\t GST_LIBCAMERA, PROVIDER, GstDeviceProvider)\n> +\n> +G_END_DECLS\n> +\n> +#endif /* __GST_LIBCAMERA_PROVIDER_H__ */\n> +\n> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> index 39a34e7..7769b78 100644\n> --- a/src/gstreamer/meson.build\n> +++ b/src/gstreamer/meson.build\n> @@ -2,6 +2,7 @@ libcamera_gst_sources = [\n> 'gstlibcamera.c',\n> 'gstlibcamera-utils.cpp',\n> 'gstlibcamerasrc.cpp',\n> + 'gstlibcameraprovider.cpp',\n\nAlphabetically sorted here too ?\n\n> ]\n> \n> libcamera_gst_c_args = [","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 90D97609D7\n\tfor ;\n\tTue, 11 Feb 2020 00:28:32 +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 1636052A;\n\tTue, 11 Feb 2020 00:28:31 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581377312;\n\tbh=nywfpp//cZUHnAKZk4poZX5d7WuKM20+p+MQGYWwDNA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=BxoJXL4PiEx5dqhhvgcUNAYzGMu07M/62HE2AoTvRpyixDUCmOtEVaRbOd6/giG7E\n\tNpM4bY7QPE6aJMiadAV3WdlADppJEM0HGd3k7fj99zkr4so8l7GuYt9TqsF1pYwycQ\n\tmo8iVvNBSsUHGatftUJFzJzn7LMwlfZG/uiDdCM0=","Date":"Tue, 11 Feb 2020 01:28:16 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200210232816.GA21893@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-4-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-4-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 03/23] gst: Add initial device\n\tprovider","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:32 -0000"}},{"id":3844,"web_url":"https://patchwork.libcamera.org/comment/3844/","msgid":"","date":"2020-02-25T17:39:52","subject":"Re: [libcamera-devel] [PATCH v1 03/23] gst: Add initial device\n\tprovider","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:50PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> > \n> > This feature is used with GstDeviceMonitor in order to enumerate\n> > and monitor devices to be used with the source element. The resulting\n> > GstDevice implementation is also used by application to abstract the\n> > configuration of the source element.\n> > \n> > Implementations notes:\n> \n> s/Implementations/Implementation/\n> \n> > - libcamera does not support polling yet\n> > - The device ID isn't unique in libcamera\n> \n> yet :-)\n> \n> > - The \"name\" property does not yet exist in libcamerasrc\n> > \n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamera.c | 10 +-\n> > src/gstreamer/gstlibcameraprovider.cpp | 227 +++++++++++++++++++++++++\n> > src/gstreamer/gstlibcameraprovider.h | 23 +++\n> > src/gstreamer/meson.build | 1 +\n> > 4 files changed, 259 insertions(+), 2 deletions(-)\n> > create mode 100644 src/gstreamer/gstlibcameraprovider.cpp\n> > create mode 100644 src/gstreamer/gstlibcameraprovider.h\n> > \n> > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> > index 953fb29..7356374 100644\n> > --- a/src/gstreamer/gstlibcamera.c\n> > +++ b/src/gstreamer/gstlibcamera.c\n> > @@ -7,12 +7,18 @@\n> > */\n> > \n> > #include \"gstlibcamerasrc.h\"\n> > +#include \"gstlibcameraprovider.h\"\n> \n> Could you please keep the headers alphabetically sorted ? Please see\n> Documentation/coding-style.rst, section \"Order of Includes\".\n> \n> > \n> > static gboolean\n> > plugin_init(GstPlugin *plugin)\n> > {\n> > -\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > -\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> > +\tif (!gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > +\t\t\t\t GST_TYPE_LIBCAMERA_SRC) ||\n> > +\t !gst_device_provider_register(plugin, \"libcameraprovider\",\n> > +\t\t\t\t\t GST_RANK_PRIMARY,\n> > +\t\t\t\t\t GST_TYPE_LIBCAMERA_PROVIDER))\n> > +\t\treturn FALSE;\n> > +\n> > \treturn TRUE;\n> > }\n> > \n> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > new file mode 100644\n> > index 0000000..3e27912\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > @@ -0,0 +1,227 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraprovider.c - GStreamer Device Provider\n> > + */\n> > +\n> > +#include \"gstlibcamera-utils.h\"\n> > +#include \"gstlibcameraprovider.h\"\n> > +#include \"gstlibcamerasrc.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > +#define GST_CAT_DEFAULT provider_debug\n> > +\n> > +/**************************************/\n> > +/* GstLibcameraDevice internal Object */\n> > +/**************************************/\n> \n> Is this standard gstreamer coding style ? Would it make sense to instead\n> use a doxygen-style comment with a bit of documentation to describe the\n> object ?\n> \n> /**\n> * \\struct _GstLibcameraDevice\n> * \\brief ...\n> *\n> * This structure describes ...\n> */\n> \n> It could be useful for developers who are less familiar with gstreamer\n> (that is, all the rest of us :-)).\n> \n> > +\n> > +enum {\n> > +\tPROP_DEVICE_NAME = 1,\n> > +};\n> > +\n> > +#define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > +\t\t GST_LIBCAMERA, DEVICE, GstDevice);\n> > +\n> > +struct _GstLibcameraDevice {\n> > +\tGstDevice parent;\n> > +\tgchar *name;\n> > +};\n> > +\n> > +G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE);\n> > +\n> > +static GstElement *\n> > +gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > +{\n> > +\tGstElement *source;\n> > +\n> > +\tsource = gst_element_factory_make(\"libcamerasrc\", name);\n> > +\tg_assert(source);\n> > +\n> > +\tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> > +\n> > +\treturn source;\n> > +}\n> > +\n> > +static gboolean\n> > +gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > +\t\t\t\t\t GstElement *element)\n> > +{\n> > +\tif (!GST_LIBCAMERA_IS_SRC(element))\n> > +\t\treturn FALSE;\n> > +\n> > +\tg_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> > +\n> > +\treturn TRUE;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > +\t\t\t\t const GValue *value, GParamSpec *pspec)\n> > +{\n> > +\tGstLibcameraDevice *device;\n> > +\n> > +\tdevice = GST_LIBCAMERA_DEVICE(object);\n> \n> You could merge the two lines:\n> \n> \tGstLibcameraDevice *device = = GST_LIBCAMERA_DEVICE(object);\n> \n> > +\n> > +\tswitch (prop_id) {\n> > +\tcase PROP_DEVICE_NAME:\n> > +\t\tdevice->name = g_value_dup_string(value);\n> > +\t\tbreak;\n> > +\tdefault:\n> > +\t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > +\t\tbreak;\n> > +\t}\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_device_init(GstLibcameraDevice *self)\n> > +{\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_device_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraDevice *self = GST_LIBCAMERA_DEVICE(object);\n> > +\tgpointer klass = gst_libcamera_device_parent_class;\n> > +\n> > +\tg_free(self->name);\n> > +\n> > +\tG_OBJECT_GET_CLASS(klass)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > +{\n> > +\tGParamSpec *pspec;\n> \n> Can't this be named just \"spec\" ?\n> \n> > +\tGstDeviceClass *device_class = (GstDeviceClass *)klass;\n> \n> Same question as for the previous patch, should this be deviceClass ?\n> \n> > +\tGObjectClass *object_class = (GObjectClass *)klass;\n> > +\n> > +\tdevice_class->create_element = gst_libcamera_device_create_element;\n> > +\tdevice_class->reconfigure_element = gst_libcamera_device_reconfigure_element;\n> > +\n> > +\tobject_class->set_property = gst_libcamera_device_set_property;\n> > +\tobject_class->finalize = gst_libcamera_device_finalize;\n> > +\n> > +\tpspec = g_param_spec_string(\"name\", \"Name\",\n> \n> As this is C++, we usually don't declare all variables at the top of\n> functions, so this would become\n> \n> \tGParamSpec *pspec = g_param_spec_string(\"name\", \"Name\",\n> \n> Up to you.\n> \n> > +\t\t\t\t \"The name of the camera device\", \"\",\n> > +\t\t\t\t (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > +\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > +\tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > +}\n> > +\n> > +static GstDevice *\n> > +gst_libcamera_device_new(const std::shared_ptr &camera)\n> > +{\n> > +\tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> > +\tg_autoptr(GstStructure) props = NULL;\n> \n> As this is always null (as far as I can tell), does it need to be an\n> auto pointer ?\n> \n> > +\tconst gchar *name = camera->name().c_str();\n> > +\tStreamRoles roles;\n> > +\n> > +\troles.push_back(StreamRole::VideoRecording);\n> > +\tstd::unique_ptr config = camera->generateConfiguration(roles);\n> > +\n> > +\tfor (const StreamConfiguration &stream_cfg : *config) {\n> > +\t\tGstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > +\t\tif (sub_caps)\n> > +\t\t\tgst_caps_append(caps, sub_caps);\n> > +\t}\n> > +\n> > +\treturn (GstDevice *)g_object_new(GST_TYPE_LIBCAMERA_DEVICE,\n> \n> C++ code should use C++ casts instead of C-style casts. This would use\n> static_cast (assuming that the GObject is the first member\n> of GstDevice).\n\nIt seems that static_cast don't work for void* to something. I could\nuse the type checking GST_LIBCAMERA_DEVICE() cast, it's overlay safe\nthough. In C with gcc/clang we instrument the g_object_new() function\nso the compiler will warn if the implicit cast is wrong, but in C++ you\nare forced to cast, which forces us to make it type unsafe (or use\nruntime checks).\n\nstatic_cast also don't generally work, since you cannot downcast a\ntype, and you cannot cast if the structure definition is not public.\n\n> \n> > +\t\t\t\t\t /* FIXME not sure name is unique */\n> > +\t\t\t\t\t \"name\", name,\n> > +\t\t\t\t\t \"display-name\", name,\n> > +\t\t\t\t\t \"caps\", caps,\n> > +\t\t\t\t\t \"device-class\", \"Source/Video\",\n> > +\t\t\t\t\t \"properties\", props,\n> > +\t\t\t\t\t NULL);\n> > +}\n> > +\n> > +/*************************************/\n> > +/* GstLibcameraDeviceProvider Object */\n> > +/*************************************/\n> > +\n> > +struct _GstLibcameraProvider {\n> > +\tGstDeviceProvider parent;\n> > +\tCameraManager *cm;\n> > +};\n> > +\n> > +G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,\n> > +\t\t\tGST_TYPE_DEVICE_PROVIDER,\n> > +\t\t\tGST_DEBUG_CATEGORY_INIT(provider_debug, \"libcamera-provider\", 0,\n> > +\t\t\t\t\t\t\"LibCamera Device Provider\"));\n> > +\n> > +static GList *\n> > +gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> > +{\n> > +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);\n> > +\tCameraManager *cm = self->cm;\n> > +\tGList *devices = NULL;\n> > +\tgint ret;\n> > +\n> > +\tGST_INFO_OBJECT(self, \"Probing cameras using LibCamera\");\n> > +\n> > +\t/* FIXME as long as the manager isn't able to handle hot-plug, we need to\n> > +\t * cycle start/stop here in order to ensure wthat we have an up to date\n> \n> s/wthat/that/\n> \n> > +\t * list */\n> \n> This should be\n> \n> \t/*\n> \t * FIXME as long as the manager isn't able to handle hot-plug, we need to\n> \t * cycle start/stop here in order to ensure that we have an up to date\n> \t * list.\n> \t */\n> \n> according to the coding style (period at the end, and opening and\n> closing markers on a line of their own). Even better would be to replace\n> FIXME with \\todo as that's the marker we use through the library.\n> \n> > +\tret = cm->start();\n> > +\tif (ret) {\n> > +\t\tGST_ERROR_OBJECT(self, \"Failed to retrieve device list: %s\",\n> > +\t\t\t\t g_strerror(-ret));\n> > +\t\treturn NULL;\n> > +\t}\n> > +\n> > +\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> > +\t\tGST_INFO_OBJECT(self, \"Found camera '%s'\", camera->name().c_str());\n> > +\t\tdevices = g_list_append(devices,\n> > +\t\t\t\t\tg_object_ref_sink(gst_libcamera_device_new(camera)));\n> > +\t}\n> > +\n> > +\tcm->stop();\n> > +\n> > +\treturn devices;\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_provider_init(GstLibcameraProvider *self)\n> > +{\n> > +\tGstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);\n> > +\n> > +\tself->cm = new CameraManager();\n> > +\n> > +\t/* Avoid devices being duplicated */\n> \n> s/duplicated/duplicated./\n> \n> (This applies to the rest of the series.)\n> \n> > +\tgst_device_provider_hide_provider(provider, \"v4l2deviceprovider\");\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_provider_finalize(GObject *object)\n> > +{\n> > +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);\n> > +\tgpointer klass = gst_libcamera_provider_parent_class;\n> > +\n> > +\tdelete self->cm;\n> > +\n> > +\treturn G_OBJECT_GET_CLASS(klass)->finalize(object);\n> > +}\n> > +\n> > +static void\n> > +gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)\n> > +{\n> > +\tGstDeviceProviderClass *provider_class = (GstDeviceProviderClass *)klass;\n> > +\tGObjectClass *object_class = (GObjectClass *)klass;\n> > +\n> > +\tprovider_class->probe = gst_libcamera_provider_probe;\n> > +\tobject_class->finalize = gst_libcamera_provider_finalize;\n> > +\n> > +\tgst_device_provider_class_set_metadata(provider_class,\n> > +\t\t\t\t\t \"LibCamera Device Provider\",\n> > +\t\t\t\t\t \"Source/Video\",\n> > +\t\t\t\t\t \"List camera device using LibCamera\",\n> > +\t\t\t\t\t \"Nicolas Dufresne \");\n> > +}\n> > diff --git a/src/gstreamer/gstlibcameraprovider.h b/src/gstreamer/gstlibcameraprovider.h\n> > new file mode 100644\n> > index 0000000..6dd232d\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcameraprovider.h\n> > @@ -0,0 +1,23 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcameraprovider.h - GStreamer Device Provider\n> > + */\n> > +\n> > +#include \n> > +\n> > +#ifndef __GST_LIBCAMERA_PROVIDER_H__\n> > +#define __GST_LIBCAMERA_PROVIDER_H__\n> > +\n> > +G_BEGIN_DECLS\n> > +\n> > +#define GST_TYPE_LIBCAMERA_PROVIDER gst_libcamera_provider_get_type()\n> > +G_DECLARE_FINAL_TYPE(GstLibcameraProvider, gst_libcamera_provider,\n> > +\t\t GST_LIBCAMERA, PROVIDER, GstDeviceProvider)\n> > +\n> > +G_END_DECLS\n> > +\n> > +#endif /* __GST_LIBCAMERA_PROVIDER_H__ */\n> > +\n> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > index 39a34e7..7769b78 100644\n> > --- a/src/gstreamer/meson.build\n> > +++ b/src/gstreamer/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_gst_sources = [\n> > 'gstlibcamera.c',\n> > 'gstlibcamera-utils.cpp',\n> > 'gstlibcamerasrc.cpp',\n> > + 'gstlibcameraprovider.cpp',\n> \n> Alphabetically sorted here too ?\n> \n> > ]\n> > \n> > libcamera_gst_c_args = [","headers":{"Return-Path":"","Received":["from mail-qv1-xf33.google.com (mail-qv1-xf33.google.com\n\t[IPv6:2607:f8b0:4864:20::f33])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D32B86042E\n\tfor ;\n\tTue, 25 Feb 2020 18:39:55 +0100 (CET)","by mail-qv1-xf33.google.com with SMTP id o18so61278qvf.1\n\tfor ;\n\tTue, 25 Feb 2020 09:39:55 -0800 (PST)","from skullcanyon ([192.222.193.21])\n\tby smtp.gmail.com with ESMTPSA id\n\ty91sm7970303qtd.13.2020.02.25.09.39.53\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 25 Feb 2020 09:39:53 -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=N2GvXPzbJOJzWgCVO3/DU9LQp/5fpw4ZLGK+JKz9P70=;\n\tb=YII1KoFXtTnkBVOZNa/BbVxsqkS6sufHM+OrCW1wHsnR/zhAAXKnEuZ5rS6KseRDhM\n\t0jP/s7BhMhYz1R8gA4eJsSlTI9RknvC6Uil+VAQNtLuJ6ZY7IpswpUsukV0wBzZgtfwZ\n\tErYZNrxf4roHR2OBzzQUOggf2otbn5F5NsELhre0/Ps/PUqOMQMrIPnh9NFkoQxNEWC3\n\tuQZf6NDTKdEfT2Szgnn6QpOukakszBxXJQZWqplTlXqM5Hs8haopoSQrj4FfDtTu+cQd\n\tH7Sd17vz3E7DvWrHWGzeqNNCpyZZftxHduVrehgKzU3Zy7LvW3JzbYyfDz+AsjCq8DHC\n\tKvbQ==","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=N2GvXPzbJOJzWgCVO3/DU9LQp/5fpw4ZLGK+JKz9P70=;\n\tb=qDw2z7aeV/AAabtooVGMpc7PvVsl+GVkrUiJFzbn8bGUyZjw4Vic3QkzKd2zLaZpZN\n\tQsa7m3q7LBMlQEeTeSmIa+GNZZSoTqaXJfrEfT+m7o5/+cD68y6kKYTOe6rQac7ycbMd\n\tyAiEPwxRWlqqmETMYNdfWJ/MuweLCLw7xHTqePKjkfG5ejbP2slpfMTC5Lna8pKY7/fR\n\tviMAbU3ydIXi2/FsLRtGxA2vkXkoYOAPJoB1s2H0T6arKC/ESyR54D9CWjaVHgsg+TXA\n\tQHEI7m4eNJ+UuS/rFs91XL4UPh90vn18Q6VIEepYA+yO73zYPFXAzYsvmjiuWb9XTWcx\n\thtwQ==","X-Gm-Message-State":"APjAAAUsgHepSMh2AlL8TcklmSNle/GM5m8pRZZFl53Pwsv7QkM5vi+l\n\tnvanOyAvfEe0P36ZxxYKNycXzA+m7IE3tw==","X-Google-Smtp-Source":"APXvYqx9g6fXYUU8oxH3rkltaUxMlgiSYwWh4JNzoRO1eVGhIArgpcjS9dXYQZPWCHqg0N7/gAWdNg==","X-Received":"by 2002:a0c:bf05:: with SMTP id m5mr16508qvi.26.1582652394509;\n\tTue, 25 Feb 2020 09:39:54 -0800 (PST)","Message-ID":"","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Tue, 25 Feb 2020 12:39:52 -0500","In-Reply-To":"<20200210232816.GA21893@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-4-nicolas@ndufresne.ca>\n\t<20200210232816.GA21893@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 03/23] gst: Add initial device\n\tprovider","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 17:39:56 -0000"}},{"id":3845,"web_url":"https://patchwork.libcamera.org/comment/3845/","msgid":"<20200225174157.GR4764@pendragon.ideasonboard.com>","date":"2020-02-25T17:41:57","subject":"Re: [libcamera-devel] [PATCH v1 03/23] gst: Add initial device\n\tprovider","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 12:39:52PM -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:50PM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne \n> > > \n> > > This feature is used with GstDeviceMonitor in order to enumerate\n> > > and monitor devices to be used with the source element. The resulting\n> > > GstDevice implementation is also used by application to abstract the\n> > > configuration of the source element.\n> > > \n> > > Implementations notes:\n> > \n> > s/Implementations/Implementation/\n> > \n> > > - libcamera does not support polling yet\n> > > - The device ID isn't unique in libcamera\n> > \n> > yet :-)\n> > \n> > > - The \"name\" property does not yet exist in libcamerasrc\n> > > \n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > src/gstreamer/gstlibcamera.c | 10 +-\n> > > src/gstreamer/gstlibcameraprovider.cpp | 227 +++++++++++++++++++++++++\n> > > src/gstreamer/gstlibcameraprovider.h | 23 +++\n> > > src/gstreamer/meson.build | 1 +\n> > > 4 files changed, 259 insertions(+), 2 deletions(-)\n> > > create mode 100644 src/gstreamer/gstlibcameraprovider.cpp\n> > > create mode 100644 src/gstreamer/gstlibcameraprovider.h\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c\n> > > index 953fb29..7356374 100644\n> > > --- a/src/gstreamer/gstlibcamera.c\n> > > +++ b/src/gstreamer/gstlibcamera.c\n> > > @@ -7,12 +7,18 @@\n> > > */\n> > > \n> > > #include \"gstlibcamerasrc.h\"\n> > > +#include \"gstlibcameraprovider.h\"\n> > \n> > Could you please keep the headers alphabetically sorted ? Please see\n> > Documentation/coding-style.rst, section \"Order of Includes\".\n> > \n> > > \n> > > static gboolean\n> > > plugin_init(GstPlugin *plugin)\n> > > {\n> > > -\treturn gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > > -\t\t\t\t GST_TYPE_LIBCAMERA_SRC);\n> > > +\tif (!gst_element_register(plugin, \"libcamerasrc\", GST_RANK_PRIMARY,\n> > > +\t\t\t\t GST_TYPE_LIBCAMERA_SRC) ||\n> > > +\t !gst_device_provider_register(plugin, \"libcameraprovider\",\n> > > +\t\t\t\t\t GST_RANK_PRIMARY,\n> > > +\t\t\t\t\t GST_TYPE_LIBCAMERA_PROVIDER))\n> > > +\t\treturn FALSE;\n> > > +\n> > > \treturn TRUE;\n> > > }\n> > > \n> > > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp\n> > > new file mode 100644\n> > > index 0000000..3e27912\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcameraprovider.cpp\n> > > @@ -0,0 +1,227 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcameraprovider.c - GStreamer Device Provider\n> > > + */\n> > > +\n> > > +#include \"gstlibcamera-utils.h\"\n> > > +#include \"gstlibcameraprovider.h\"\n> > > +#include \"gstlibcamerasrc.h\"\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +GST_DEBUG_CATEGORY_STATIC(provider_debug);\n> > > +#define GST_CAT_DEFAULT provider_debug\n> > > +\n> > > +/**************************************/\n> > > +/* GstLibcameraDevice internal Object */\n> > > +/**************************************/\n> > \n> > Is this standard gstreamer coding style ? Would it make sense to instead\n> > use a doxygen-style comment with a bit of documentation to describe the\n> > object ?\n> > \n> > /**\n> > * \\struct _GstLibcameraDevice\n> > * \\brief ...\n> > *\n> > * This structure describes ...\n> > */\n> > \n> > It could be useful for developers who are less familiar with gstreamer\n> > (that is, all the rest of us :-)).\n> > \n> > > +\n> > > +enum {\n> > > +\tPROP_DEVICE_NAME = 1,\n> > > +};\n> > > +\n> > > +#define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()\n> > > +G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,\n> > > +\t\t GST_LIBCAMERA, DEVICE, GstDevice);\n> > > +\n> > > +struct _GstLibcameraDevice {\n> > > +\tGstDevice parent;\n> > > +\tgchar *name;\n> > > +};\n> > > +\n> > > +G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE);\n> > > +\n> > > +static GstElement *\n> > > +gst_libcamera_device_create_element(GstDevice *device, const gchar *name)\n> > > +{\n> > > +\tGstElement *source;\n> > > +\n> > > +\tsource = gst_element_factory_make(\"libcamerasrc\", name);\n> > > +\tg_assert(source);\n> > > +\n> > > +\tg_object_set(source, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> > > +\n> > > +\treturn source;\n> > > +}\n> > > +\n> > > +static gboolean\n> > > +gst_libcamera_device_reconfigure_element(GstDevice *device,\n> > > +\t\t\t\t\t GstElement *element)\n> > > +{\n> > > +\tif (!GST_LIBCAMERA_IS_SRC(element))\n> > > +\t\treturn FALSE;\n> > > +\n> > > +\tg_object_set(element, \"camera-name\", GST_LIBCAMERA_DEVICE(device)->name, NULL);\n> > > +\n> > > +\treturn TRUE;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_device_set_property(GObject *object, guint prop_id,\n> > > +\t\t\t\t const GValue *value, GParamSpec *pspec)\n> > > +{\n> > > +\tGstLibcameraDevice *device;\n> > > +\n> > > +\tdevice = GST_LIBCAMERA_DEVICE(object);\n> > \n> > You could merge the two lines:\n> > \n> > \tGstLibcameraDevice *device = = GST_LIBCAMERA_DEVICE(object);\n> > \n> > > +\n> > > +\tswitch (prop_id) {\n> > > +\tcase PROP_DEVICE_NAME:\n> > > +\t\tdevice->name = g_value_dup_string(value);\n> > > +\t\tbreak;\n> > > +\tdefault:\n> > > +\t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > +\t\tbreak;\n> > > +\t}\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_device_init(GstLibcameraDevice *self)\n> > > +{\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_device_finalize(GObject *object)\n> > > +{\n> > > +\tGstLibcameraDevice *self = GST_LIBCAMERA_DEVICE(object);\n> > > +\tgpointer klass = gst_libcamera_device_parent_class;\n> > > +\n> > > +\tg_free(self->name);\n> > > +\n> > > +\tG_OBJECT_GET_CLASS(klass)->finalize(object);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)\n> > > +{\n> > > +\tGParamSpec *pspec;\n> > \n> > Can't this be named just \"spec\" ?\n> > \n> > > +\tGstDeviceClass *device_class = (GstDeviceClass *)klass;\n> > \n> > Same question as for the previous patch, should this be deviceClass ?\n> > \n> > > +\tGObjectClass *object_class = (GObjectClass *)klass;\n> > > +\n> > > +\tdevice_class->create_element = gst_libcamera_device_create_element;\n> > > +\tdevice_class->reconfigure_element = gst_libcamera_device_reconfigure_element;\n> > > +\n> > > +\tobject_class->set_property = gst_libcamera_device_set_property;\n> > > +\tobject_class->finalize = gst_libcamera_device_finalize;\n> > > +\n> > > +\tpspec = g_param_spec_string(\"name\", \"Name\",\n> > \n> > As this is C++, we usually don't declare all variables at the top of\n> > functions, so this would become\n> > \n> > \tGParamSpec *pspec = g_param_spec_string(\"name\", \"Name\",\n> > \n> > Up to you.\n> > \n> > > +\t\t\t\t \"The name of the camera device\", \"\",\n> > > +\t\t\t\t (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |\n> > > +\t\t\t\t\t\t G_PARAM_CONSTRUCT_ONLY));\n> > > +\tg_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);\n> > > +}\n> > > +\n> > > +static GstDevice *\n> > > +gst_libcamera_device_new(const std::shared_ptr &camera)\n> > > +{\n> > > +\tg_autoptr(GstCaps) caps = gst_caps_new_empty();\n> > > +\tg_autoptr(GstStructure) props = NULL;\n> > \n> > As this is always null (as far as I can tell), does it need to be an\n> > auto pointer ?\n> > \n> > > +\tconst gchar *name = camera->name().c_str();\n> > > +\tStreamRoles roles;\n> > > +\n> > > +\troles.push_back(StreamRole::VideoRecording);\n> > > +\tstd::unique_ptr config = camera->generateConfiguration(roles);\n> > > +\n> > > +\tfor (const StreamConfiguration &stream_cfg : *config) {\n> > > +\t\tGstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());\n> > > +\t\tif (sub_caps)\n> > > +\t\t\tgst_caps_append(caps, sub_caps);\n> > > +\t}\n> > > +\n> > > +\treturn (GstDevice *)g_object_new(GST_TYPE_LIBCAMERA_DEVICE,\n> > \n> > C++ code should use C++ casts instead of C-style casts. This would use\n> > static_cast (assuming that the GObject is the first member\n> > of GstDevice).\n> \n> It seems that static_cast don't work for void* to something. I could\n> use the type checking GST_LIBCAMERA_DEVICE() cast, it's overlay safe\n> though. In C with gcc/clang we instrument the g_object_new() function\n> so the compiler will warn if the implicit cast is wrong, but in C++ you\n> are forced to cast, which forces us to make it type unsafe (or use\n> runtime checks).\n> \n> static_cast also don't generally work, since you cannot downcast a\n> type, and you cannot cast if the structure definition is not public.\n\nstatic_cast indeed requires definitions of both types. You can use a\nGStreamer- or glib-specific casting macro if you have one, or\nalternatively use reinterpret_cast in this specific case.\n\n> > > +\t\t\t\t\t /* FIXME not sure name is unique */\n> > > +\t\t\t\t\t \"name\", name,\n> > > +\t\t\t\t\t \"display-name\", name,\n> > > +\t\t\t\t\t \"caps\", caps,\n> > > +\t\t\t\t\t \"device-class\", \"Source/Video\",\n> > > +\t\t\t\t\t \"properties\", props,\n> > > +\t\t\t\t\t NULL);\n> > > +}\n> > > +\n> > > +/*************************************/\n> > > +/* GstLibcameraDeviceProvider Object */\n> > > +/*************************************/\n> > > +\n> > > +struct _GstLibcameraProvider {\n> > > +\tGstDeviceProvider parent;\n> > > +\tCameraManager *cm;\n> > > +};\n> > > +\n> > > +G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,\n> > > +\t\t\tGST_TYPE_DEVICE_PROVIDER,\n> > > +\t\t\tGST_DEBUG_CATEGORY_INIT(provider_debug, \"libcamera-provider\", 0,\n> > > +\t\t\t\t\t\t\"LibCamera Device Provider\"));\n> > > +\n> > > +static GList *\n> > > +gst_libcamera_provider_probe(GstDeviceProvider *provider)\n> > > +{\n> > > +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);\n> > > +\tCameraManager *cm = self->cm;\n> > > +\tGList *devices = NULL;\n> > > +\tgint ret;\n> > > +\n> > > +\tGST_INFO_OBJECT(self, \"Probing cameras using LibCamera\");\n> > > +\n> > > +\t/* FIXME as long as the manager isn't able to handle hot-plug, we need to\n> > > +\t * cycle start/stop here in order to ensure wthat we have an up to date\n> > \n> > s/wthat/that/\n> > \n> > > +\t * list */\n> > \n> > This should be\n> > \n> > \t/*\n> > \t * FIXME as long as the manager isn't able to handle hot-plug, we need to\n> > \t * cycle start/stop here in order to ensure that we have an up to date\n> > \t * list.\n> > \t */\n> > \n> > according to the coding style (period at the end, and opening and\n> > closing markers on a line of their own). Even better would be to replace\n> > FIXME with \\todo as that's the marker we use through the library.\n> > \n> > > +\tret = cm->start();\n> > > +\tif (ret) {\n> > > +\t\tGST_ERROR_OBJECT(self, \"Failed to retrieve device list: %s\",\n> > > +\t\t\t\t g_strerror(-ret));\n> > > +\t\treturn NULL;\n> > > +\t}\n> > > +\n> > > +\tfor (const std::shared_ptr &camera : cm->cameras()) {\n> > > +\t\tGST_INFO_OBJECT(self, \"Found camera '%s'\", camera->name().c_str());\n> > > +\t\tdevices = g_list_append(devices,\n> > > +\t\t\t\t\tg_object_ref_sink(gst_libcamera_device_new(camera)));\n> > > +\t}\n> > > +\n> > > +\tcm->stop();\n> > > +\n> > > +\treturn devices;\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_provider_init(GstLibcameraProvider *self)\n> > > +{\n> > > +\tGstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);\n> > > +\n> > > +\tself->cm = new CameraManager();\n> > > +\n> > > +\t/* Avoid devices being duplicated */\n> > \n> > s/duplicated/duplicated./\n> > \n> > (This applies to the rest of the series.)\n> > \n> > > +\tgst_device_provider_hide_provider(provider, \"v4l2deviceprovider\");\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_provider_finalize(GObject *object)\n> > > +{\n> > > +\tGstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);\n> > > +\tgpointer klass = gst_libcamera_provider_parent_class;\n> > > +\n> > > +\tdelete self->cm;\n> > > +\n> > > +\treturn G_OBJECT_GET_CLASS(klass)->finalize(object);\n> > > +}\n> > > +\n> > > +static void\n> > > +gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)\n> > > +{\n> > > +\tGstDeviceProviderClass *provider_class = (GstDeviceProviderClass *)klass;\n> > > +\tGObjectClass *object_class = (GObjectClass *)klass;\n> > > +\n> > > +\tprovider_class->probe = gst_libcamera_provider_probe;\n> > > +\tobject_class->finalize = gst_libcamera_provider_finalize;\n> > > +\n> > > +\tgst_device_provider_class_set_metadata(provider_class,\n> > > +\t\t\t\t\t \"LibCamera Device Provider\",\n> > > +\t\t\t\t\t \"Source/Video\",\n> > > +\t\t\t\t\t \"List camera device using LibCamera\",\n> > > +\t\t\t\t\t \"Nicolas Dufresne \");\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcameraprovider.h b/src/gstreamer/gstlibcameraprovider.h\n> > > new file mode 100644\n> > > index 0000000..6dd232d\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcameraprovider.h\n> > > @@ -0,0 +1,23 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcameraprovider.h - GStreamer Device Provider\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +#ifndef __GST_LIBCAMERA_PROVIDER_H__\n> > > +#define __GST_LIBCAMERA_PROVIDER_H__\n> > > +\n> > > +G_BEGIN_DECLS\n> > > +\n> > > +#define GST_TYPE_LIBCAMERA_PROVIDER gst_libcamera_provider_get_type()\n> > > +G_DECLARE_FINAL_TYPE(GstLibcameraProvider, gst_libcamera_provider,\n> > > +\t\t GST_LIBCAMERA, PROVIDER, GstDeviceProvider)\n> > > +\n> > > +G_END_DECLS\n> > > +\n> > > +#endif /* __GST_LIBCAMERA_PROVIDER_H__ */\n> > > +\n> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > index 39a34e7..7769b78 100644\n> > > --- a/src/gstreamer/meson.build\n> > > +++ b/src/gstreamer/meson.build\n> > > @@ -2,6 +2,7 @@ libcamera_gst_sources = [\n> > > 'gstlibcamera.c',\n> > > 'gstlibcamera-utils.cpp',\n> > > 'gstlibcamerasrc.cpp',\n> > > + 'gstlibcameraprovider.cpp',\n> > \n> > Alphabetically sorted here too ?\n> > \n> > > ]\n> > > \n> > > libcamera_gst_c_args = [","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 3E2796042E\n\tfor ;\n\tTue, 25 Feb 2020 18:42:19 +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 A24BB43F;\n\tTue, 25 Feb 2020 18:42:18 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582652538;\n\tbh=vV09fKGI+esaiUJ8QBStLRx4OFecXYGHInf5ire75Ec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QfXYtU+ikFOnHQh+WCBEVo+ElH3/PRvAuAcDw83lwzwL2edmwSFohpcWkIjw3vnWr\n\tRYOSMPWeczCVzFegy2cVFufwDDyv6fEYf6RsttHx9+TmZeP3ZHPNIiKa7gqWBaUaC/\n\tnRFgabeycfY9XMAejuUBMSA7JAaMbVp4shxsVSck=","Date":"Tue, 25 Feb 2020 19:41:57 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200225174157.GR4764@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-4-nicolas@ndufresne.ca>\n\t<20200210232816.GA21893@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 03/23] gst: Add initial device\n\tprovider","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 17:42:19 -0000"}}]