[libcamera-devel] [PATCH v1 03/23] gst: Add initial device provider
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 11 00:28:16 CET 2020
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).
> + /* 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