[libcamera-devel] [PATCH v1 03/23] gst: Add initial device provider

[Nicolas Dufresne]

Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit :
> Hi Nicolas,
> 
> Thank you for the patch.
> 
> On Tue, Jan 28, 2020 at 10:31:50PM -0500, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > 
> > This feature is used with GstDeviceMonitor in order to enumerate
> > and monitor devices to be used with the source element. The resulting
> > GstDevice implementation is also used by application to abstract the
> > configuration of the source element.
> > 
> > Implementations notes:
> 
> s/Implementations/Implementation/
> 
> >   - libcamera does not support polling yet
> >   - The device ID isn't unique in libcamera
> 
> yet :-)
> 
> >   - The "name" property does not yet exist in libcamerasrc
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > ---
> >  src/gstreamer/gstlibcamera.c           |  10 +-
> >  src/gstreamer/gstlibcameraprovider.cpp | 227 +++++++++++++++++++++++++
> >  src/gstreamer/gstlibcameraprovider.h   |  23 +++
> >  src/gstreamer/meson.build              |   1 +
> >  4 files changed, 259 insertions(+), 2 deletions(-)
> >  create mode 100644 src/gstreamer/gstlibcameraprovider.cpp
> >  create mode 100644 src/gstreamer/gstlibcameraprovider.h
> > 
> > diff --git a/src/gstreamer/gstlibcamera.c b/src/gstreamer/gstlibcamera.c
> > index 953fb29..7356374 100644
> > --- a/src/gstreamer/gstlibcamera.c
> > +++ b/src/gstreamer/gstlibcamera.c
> > @@ -7,12 +7,18 @@
> >   */
> >  
> >  #include "gstlibcamerasrc.h"
> > +#include "gstlibcameraprovider.h"
> 
> Could you please keep the headers alphabetically sorted ? Please see
> Documentation/coding-style.rst, section "Order of Includes".
> 
> >  
> >  static gboolean
> >  plugin_init(GstPlugin *plugin)
> >  {
> > -	return gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY,
> > -				    GST_TYPE_LIBCAMERA_SRC);
> > +	if (!gst_element_register(plugin, "libcamerasrc", GST_RANK_PRIMARY,
> > +				  GST_TYPE_LIBCAMERA_SRC) ||
> > +	    !gst_device_provider_register(plugin, "libcameraprovider",
> > +					  GST_RANK_PRIMARY,
> > +					  GST_TYPE_LIBCAMERA_PROVIDER))
> > +		return FALSE;
> > +
> >  	return TRUE;
> >  }
> >  
> > diff --git a/src/gstreamer/gstlibcameraprovider.cpp b/src/gstreamer/gstlibcameraprovider.cpp
> > new file mode 100644
> > index 0000000..3e27912
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraprovider.cpp
> > @@ -0,0 +1,227 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > + *
> > + * gstlibcameraprovider.c - GStreamer Device Provider
> > + */
> > +
> > +#include "gstlibcamera-utils.h"
> > +#include "gstlibcameraprovider.h"
> > +#include "gstlibcamerasrc.h"
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/camera_manager.h>
> > +
> > +using namespace libcamera;
> > +
> > +GST_DEBUG_CATEGORY_STATIC(provider_debug);
> > +#define GST_CAT_DEFAULT provider_debug
> > +
> > +/**************************************/
> > +/* GstLibcameraDevice internal Object */
> > +/**************************************/
> 
> Is this standard gstreamer coding style ? Would it make sense to instead
> use a doxygen-style comment with a bit of documentation to describe the
> object ?
> 
> /**
>  * \struct _GstLibcameraDevice
>  * \brief ...
>  *
>  * This structure describes ...
>  */
> 
> It could be useful for developers who are less familiar with gstreamer
> (that is, all the rest of us :-)).
> 
> > +
> > +enum {
> > +	PROP_DEVICE_NAME = 1,
> > +};
> > +
> > +#define GST_TYPE_LIBCAMERA_DEVICE gst_libcamera_device_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraDevice, gst_libcamera_device,
> > +		     GST_LIBCAMERA, DEVICE, GstDevice);
> > +
> > +struct _GstLibcameraDevice {
> > +	GstDevice parent;
> > +	gchar *name;
> > +};
> > +
> > +G_DEFINE_TYPE(GstLibcameraDevice, gst_libcamera_device, GST_TYPE_DEVICE);
> > +
> > +static GstElement *
> > +gst_libcamera_device_create_element(GstDevice *device, const gchar *name)
> > +{
> > +	GstElement *source;
> > +
> > +	source = gst_element_factory_make("libcamerasrc", name);
> > +	g_assert(source);
> > +
> > +	g_object_set(source, "camera-name", GST_LIBCAMERA_DEVICE(device)->name, NULL);
> > +
> > +	return source;
> > +}
> > +
> > +static gboolean
> > +gst_libcamera_device_reconfigure_element(GstDevice *device,
> > +					 GstElement *element)
> > +{
> > +	if (!GST_LIBCAMERA_IS_SRC(element))
> > +		return FALSE;
> > +
> > +	g_object_set(element, "camera-name", GST_LIBCAMERA_DEVICE(device)->name, NULL);
> > +
> > +	return TRUE;
> > +}
> > +
> > +static void
> > +gst_libcamera_device_set_property(GObject *object, guint prop_id,
> > +				  const GValue *value, GParamSpec *pspec)
> > +{
> > +	GstLibcameraDevice *device;
> > +
> > +	device = GST_LIBCAMERA_DEVICE(object);
> 
> You could merge the two lines:
> 
> 	GstLibcameraDevice *device = = GST_LIBCAMERA_DEVICE(object);
> 
> > +
> > +	switch (prop_id) {
> > +	case PROP_DEVICE_NAME:
> > +		device->name = g_value_dup_string(value);
> > +		break;
> > +	default:
> > +		G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > +		break;
> > +	}
> > +}
> > +
> > +static void
> > +gst_libcamera_device_init(GstLibcameraDevice *self)
> > +{
> > +}
> > +
> > +static void
> > +gst_libcamera_device_finalize(GObject *object)
> > +{
> > +	GstLibcameraDevice *self = GST_LIBCAMERA_DEVICE(object);
> > +	gpointer klass = gst_libcamera_device_parent_class;
> > +
> > +	g_free(self->name);
> > +
> > +	G_OBJECT_GET_CLASS(klass)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_device_class_init(GstLibcameraDeviceClass *klass)
> > +{
> > +	GParamSpec *pspec;
> 
> Can't this be named just "spec" ?
> 
> > +	GstDeviceClass *device_class = (GstDeviceClass *)klass;
> 
> Same question as for the previous patch, should this be deviceClass ?
> 
> > +	GObjectClass *object_class = (GObjectClass *)klass;
> > +
> > +	device_class->create_element = gst_libcamera_device_create_element;
> > +	device_class->reconfigure_element = gst_libcamera_device_reconfigure_element;
> > +
> > +	object_class->set_property = gst_libcamera_device_set_property;
> > +	object_class->finalize = gst_libcamera_device_finalize;
> > +
> > +	pspec = g_param_spec_string("name", "Name",
> 
> As this is C++, we usually don't declare all variables at the top of
> functions, so this would become
> 
> 	GParamSpec *pspec = g_param_spec_string("name", "Name",
> 
> Up to you.
> 
> > +				    "The name of the camera device", "",
> > +				    (GParamFlags)(G_PARAM_STATIC_STRINGS | G_PARAM_WRITABLE |
> > +						  G_PARAM_CONSTRUCT_ONLY));
> > +	g_object_class_install_property(object_class, PROP_DEVICE_NAME, pspec);
> > +}
> > +
> > +static GstDevice *
> > +gst_libcamera_device_new(const std::shared_ptr<Camera> &camera)
> > +{
> > +	g_autoptr(GstCaps) caps = gst_caps_new_empty();
> > +	g_autoptr(GstStructure) props = NULL;
> 
> As this is always null (as far as I can tell), does it need to be an
> auto pointer ?
> 
> > +	const gchar *name = camera->name().c_str();
> > +	StreamRoles roles;
> > +
> > +	roles.push_back(StreamRole::VideoRecording);
> > +	std::unique_ptr<CameraConfiguration> config = camera->generateConfiguration(roles);
> > +
> > +	for (const StreamConfiguration &stream_cfg : *config) {
> > +		GstCaps *sub_caps = gst_libcamera_stream_formats_to_caps(stream_cfg.formats());
> > +		if (sub_caps)
> > +			gst_caps_append(caps, sub_caps);
> > +	}
> > +
> > +	return (GstDevice *)g_object_new(GST_TYPE_LIBCAMERA_DEVICE,
> 
> C++ code should use C++ casts instead of C-style casts. This would use
> static_cast<GstDevice *> (assuming that the GObject is the first member
> of GstDevice).

It seems that static_cast don't work for void* to something. I could
use the type checking GST_LIBCAMERA_DEVICE() cast, it's overlay safe
though. In C with gcc/clang we instrument the g_object_new() function
so the compiler will warn if the implicit cast is wrong, but in C++ you
are forced to cast, which forces us to make it type unsafe (or use
runtime checks).

static_cast also don't generally work, since you cannot downcast a
type, and you cannot cast if the structure definition is not public.

> 
> > +					 /* FIXME not sure name is unique */
> > +					 "name", name,
> > +					 "display-name", name,
> > +					 "caps", caps,
> > +					 "device-class", "Source/Video",
> > +					 "properties", props,
> > +					 NULL);
> > +}
> > +
> > +/*************************************/
> > +/* GstLibcameraDeviceProvider Object */
> > +/*************************************/
> > +
> > +struct _GstLibcameraProvider {
> > +	GstDeviceProvider parent;
> > +	CameraManager *cm;
> > +};
> > +
> > +G_DEFINE_TYPE_WITH_CODE(GstLibcameraProvider, gst_libcamera_provider,
> > +			GST_TYPE_DEVICE_PROVIDER,
> > +			GST_DEBUG_CATEGORY_INIT(provider_debug, "libcamera-provider", 0,
> > +						"LibCamera Device Provider"));
> > +
> > +static GList *
> > +gst_libcamera_provider_probe(GstDeviceProvider *provider)
> > +{
> > +	GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(provider);
> > +	CameraManager *cm = self->cm;
> > +	GList *devices = NULL;
> > +	gint ret;
> > +
> > +	GST_INFO_OBJECT(self, "Probing cameras using LibCamera");
> > +
> > +	/* FIXME as long as the manager isn't able to handle hot-plug, we need to
> > +	 * cycle start/stop here in order to ensure wthat we have an up to date
> 
> s/wthat/that/
> 
> > +	 * list */
> 
> This should be
> 
> 	/*
> 	 * FIXME as long as the manager isn't able to handle hot-plug, we need to
> 	 * cycle start/stop here in order to ensure that we have an up to date
> 	 * list.
> 	 */
> 
> according to the coding style (period at the end, and opening and
> closing markers on a line of their own). Even better would be to replace
> FIXME with \todo as that's the marker we use through the library.
> 
> > +	ret = cm->start();
> > +	if (ret) {
> > +		GST_ERROR_OBJECT(self, "Failed to retrieve device list: %s",
> > +				 g_strerror(-ret));
> > +		return NULL;
> > +	}
> > +
> > +	for (const std::shared_ptr<Camera> &camera : cm->cameras()) {
> > +		GST_INFO_OBJECT(self, "Found camera '%s'", camera->name().c_str());
> > +		devices = g_list_append(devices,
> > +					g_object_ref_sink(gst_libcamera_device_new(camera)));
> > +	}
> > +
> > +	cm->stop();
> > +
> > +	return devices;
> > +}
> > +
> > +static void
> > +gst_libcamera_provider_init(GstLibcameraProvider *self)
> > +{
> > +	GstDeviceProvider *provider = GST_DEVICE_PROVIDER(self);
> > +
> > +	self->cm = new CameraManager();
> > +
> > +	/* Avoid devices being duplicated */
> 
> s/duplicated/duplicated./
> 
> (This applies to the rest of the series.)
> 
> > +	gst_device_provider_hide_provider(provider, "v4l2deviceprovider");
> > +}
> > +
> > +static void
> > +gst_libcamera_provider_finalize(GObject *object)
> > +{
> > +	GstLibcameraProvider *self = GST_LIBCAMERA_PROVIDER(object);
> > +	gpointer klass = gst_libcamera_provider_parent_class;
> > +
> > +	delete self->cm;
> > +
> > +	return G_OBJECT_GET_CLASS(klass)->finalize(object);
> > +}
> > +
> > +static void
> > +gst_libcamera_provider_class_init(GstLibcameraProviderClass *klass)
> > +{
> > +	GstDeviceProviderClass *provider_class = (GstDeviceProviderClass *)klass;
> > +	GObjectClass *object_class = (GObjectClass *)klass;
> > +
> > +	provider_class->probe = gst_libcamera_provider_probe;
> > +	object_class->finalize = gst_libcamera_provider_finalize;
> > +
> > +	gst_device_provider_class_set_metadata(provider_class,
> > +					       "LibCamera Device Provider",
> > +					       "Source/Video",
> > +					       "List camera device using LibCamera",
> > +					       "Nicolas Dufresne <nicolas.dufresne at collabora.com>");
> > +}
> > diff --git a/src/gstreamer/gstlibcameraprovider.h b/src/gstreamer/gstlibcameraprovider.h
> > new file mode 100644
> > index 0000000..6dd232d
> > --- /dev/null
> > +++ b/src/gstreamer/gstlibcameraprovider.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Collabora Ltd.
> > + *     Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > + *
> > + * gstlibcameraprovider.h - GStreamer Device Provider
> > + */
> > +
> > +#include <gst/gst.h>
> > +
> > +#ifndef __GST_LIBCAMERA_PROVIDER_H__
> > +#define __GST_LIBCAMERA_PROVIDER_H__
> > +
> > +G_BEGIN_DECLS
> > +
> > +#define GST_TYPE_LIBCAMERA_PROVIDER gst_libcamera_provider_get_type()
> > +G_DECLARE_FINAL_TYPE(GstLibcameraProvider, gst_libcamera_provider,
> > +		     GST_LIBCAMERA, PROVIDER, GstDeviceProvider)
> > +
> > +G_END_DECLS
> > +
> > +#endif /* __GST_LIBCAMERA_PROVIDER_H__ */
> > +
> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > index 39a34e7..7769b78 100644
> > --- a/src/gstreamer/meson.build
> > +++ b/src/gstreamer/meson.build
> > @@ -2,6 +2,7 @@ libcamera_gst_sources = [
> >      'gstlibcamera.c',
> >      'gstlibcamera-utils.cpp',
> >      'gstlibcamerasrc.cpp',
> > +    'gstlibcameraprovider.cpp',
> 
> Alphabetically sorted here too ?
> 
> >  ]
> >  
> >  libcamera_gst_c_args = [