[libcamera-devel] [PATCH v1 03/23] gst: Add initial device provider
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 25 18:41:57 CET 2020
Hi Nicolas,
On Tue, Feb 25, 2020 at 12:39:52PM -0500, Nicolas Dufresne wrote:
> Le mardi 11 février 2020 à 01:28 +0200, Laurent Pinchart a écrit :
> > On Tue, Jan 28, 2020 at 10:31: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.
static_cast indeed requires definitions of both types. You can use a
GStreamer- or glib-specific casting macro if you have one, or
alternatively use reinterpret_cast<GstDevice *> in this specific case.
> > > + /* 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 = [
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list