[libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert StreamFormats to GstCaps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 11 17:34:10 CET 2020
Hi Nicolas,
On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote:
> Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit :
> > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:
> > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > >
> > > This transform the basic information found in StreamFormats to GstCaps.
> >
> > s/transform/transformts/
Without the last t of course :-)
> > > This can be handy to reply to early caps query or inside a device
> > > provider. Note that we ignored generated range as they are harmful to
> > > caps negotiation. We also don't simplify the caps for readability
> > > reasons, so some of the discrete value may be included in a range.
> > >
> > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > ---
> > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++
> > > src/gstreamer/gstlibcamera-utils.h | 18 +++++
> > > src/gstreamer/meson.build | 1 +
> > > 3 files changed, 121 insertions(+)
> > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp
> > > create mode 100644 src/gstreamer/gstlibcamera-utils.h
> > >
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > new file mode 100644
> > > index 0000000..2540be0
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > @@ -0,0 +1,102 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + * Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > + *
> > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function
> > > + */
> > > +
> > > +#include "gstlibcamera-utils.h"
> >
> > Could you add a blank line here, as per the coding style ?
> >
> > > +#include <linux/drm_fourcc.h>
> > > +
> > > +using namespace libcamera;
> > > +
> > > +static struct {
> > > + GstVideoFormat gst_format;
> > > + guint drm_fourcc;
> > > +} format_map[] = {
> >
> > Open question, should we use camelCase instead of snake_case ? I see
> > you're mixing the two, after a quick glance with snake_case used for
> > C-style functions and camelCase for C++ code. I don't mind that,
> > especially given that the GStreamer API is snake_case. Should we add a
> > README.md to the directory that documents this exception to the coding
> > style ?
>
> I've use camel only for c++ object method, everything else follow GST style
> for naming, types are Camel func and var lower case snake. When note, I
> made typo, that happens. It depends if you intend to keep the plugin
> forever or want in in GST long term.
I think it makes sense to plan for merging it in gstreamer, so a custom
coding style is fine by me, as long as we document it clearly.
> > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },
> > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },
> > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },
> > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },
> > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },
> > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },
> > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },
> > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },
> > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },
> > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */
> >
> > Can we try to avoid commented-out code ? Is this because
> > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?
>
> I can add it, that's usually what I do when. I'm bored and need something
> easy.
>
> > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },
> > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },
> > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },
> > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },
> > > +};
> > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)
> > > +
> > > +static inline GstVideoFormat
> >
> > The compiler should be smart enough to inline functions where needed,
> > you don't have to instruct it to do so specifically.
>
> Ok.
>
> > > +drm_to_gst_format(guint drm_fourcc)
> > > +{
> > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)
> > > + if (format_map[i].drm_fourcc == drm_fourcc)
> > > + return format_map[i].gst_format;
> >
> > If you want to make this more C++, you can define format_map as an
> > std::array, drop FORMAT_MAP_SIZE, and write
> >
> > for (const auto &format : format_map) {
> > if (format.drm_fourcc == drm_fourcc)
> > return format.gst_format;
> > }
> >
> > > + return GST_VIDEO_FORMAT_UNKNOWN;
> > > +}
> > > +
> > > +static GstStructure *
> > > +bare_structure_from_fourcc(guint fourcc)
> > > +{
> > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc);
> > > +
> > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)
> > > + return NULL;
> >
> > C++ uses nullptr instead of NULL. This comment applies to the whole
> > series.
>
> I know, annoying to switch habit. All the remaining are the one I forgot.
>
> > > +
> > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED)
> > > + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
> > > +
> > gst_video_format_to_string(gst_format), NULL);
> > > +
> > > + switch (fourcc) {
> > > + case DRM_FORMAT_MJPEG:
> > > + return gst_structure_new_empty("image/jpeg");
> > > + default:
> > > + break;
> >
> > Maybe
> >
> > return nullptr;
> >
> > here and dropping the return at the end of the function ?
> >
>
> I guess as I'm not using single return that could be better.
>
> > > + }
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +GstCaps *
> > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> > > +{
> > > + GstCaps *caps = gst_caps_new_empty();
> > > +
> > > + for (unsigned int fourcc : formats.pixelformats()) {
> > > + g_autoptr(GstStructure) bare_s =
> > bare_structure_from_fourcc(fourcc);
> >
> > g_autoptr is a really bad name when used in C++ code and the auto
> > keyword :-( Nothing we can do about it though, but I think you should
> > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use
> > unique_ptr extensively to convey ownership information.
>
> Do you have an example for that using a third party refcount system? If we
> don't use GStreamer / Glib recounts, we will break many assumption, and
> cause hard to catch errors. Each type have its own cleanup function, we
> know how to expend to that, but macros are needed anyway. I think your
> request is a lot of work, we need multiple different types of wrapper
> class, since this is not all about GObject (GstStructure is a red counted
> boxed type).
If I understand glib right (which is far from a given :-)), this would
become
static std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))>
bare_structure_from_fourcc(guint fourcc)
{
...
return std::unique_ptr<GstStructure, decltype(&_GLIB_AUTOPTR_FUNC_NAME(GstStructure))>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));
}
and here
auto bare_s = bare_structure_from_fourcc(fourcc);
You could simplify this with
template<typename T>
using gst_unique_ptr = std::unique_ptr<T, decltype(&_GLIB_AUTOPTR_FUNC_NAME(T))>;
static gst_unique_ptr<GstStructure> bare_structure_from_fourcc(guint fourcc)
{
...
return gst_unique_ptr<GstStructure>(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));
}
...
gst_unique_ptr<GstStructure> bare_s = bare_structure_from_fourcc(fourcc);
(not test-compiled though)
> > > +
> > > + if (!bare_s) {
> > > + GST_WARNING("Unsupported DRM format %"
> > GST_FOURCC_FORMAT,
> > > + GST_FOURCC_ARGS(fourcc));
> > > + continue;
> > > + }
> > > +
> > > + for (const Size &size : formats.sizes(fourcc)) {
> > > + GstStructure *s = gst_structure_copy(bare_s);
> > > + gst_structure_set(s,
> > > + "width", G_TYPE_INT, size.width,
> > > + "height", G_TYPE_INT,
> > size.height,
> > > + NULL);
> > > + gst_caps_append_structure(caps, s);
> >
> > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get
> > the raw pointer, and add bare_s.release() here to avoid the automatic
> > delete. This would avoid the need to call gst_structure_copy().
> >
> > And now that I've written this, I realise that unique_ptr will use
> > std::default_delete by default, while you need a specific cleanup
> > function. You could still use a unique_ptr with a custom deleter, but it
> > may not be worth it. Shame g_autptr doesn't support the equivalent of
> > std::unique_ptr::release() :-(
> >
> > > + }
> > > +
> > > + const SizeRange &range = formats.range(fourcc);
> > > + if (range.hStep && range.vStep) {
> > > + GstStructure *s = gst_structure_copy(bare_s);
> > > +
> > > + gst_structure_set(s,
> > > + "width", GST_TYPE_INT_RANGE,
> > range.min.width, range.max.width, range.hStep,
> > > + "height", GST_TYPE_INT_RANGE,
> > range.min.height, range.max.height, range.vStep,
> > > + NULL);
> >
> > It would be nice if the gstreamer element could use the same line
> > wrapping rules as the rest of the code.
> >
> > > + gst_caps_append_structure(caps, s);
> > > + }
> > > + }
> > > +
> > > + return caps;
> > > +}
> > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > > new file mode 100644
> > > index 0000000..4545512
> > > --- /dev/null
> > > +++ b/src/gstreamer/gstlibcamera-utils.h
> > > @@ -0,0 +1,18 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Collabora Ltd.
> > > + * Author: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > + *
> > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions
> > > + */
> > > +
> > > +#include <gst/gst.h>
> > > +#include <gst/video/video.h>
> > > +
> > > +#include <libcamera/stream.h>
> > > +
> >
> > These can go after the header guard too. This comment applies to the
> > rest of the series.
> >
> > > +#ifndef __GST_LIBCAMERA_UTILS_H_
> >
> > Missing
> >
> > #define __GST_LIBCAMERA_UTILS_H_
> >
> > The rest of the code base has two _ at the end of the header guards, how
> > about doing the same for all patches in this series ?
> >
> > > +
> > > +GstCaps *gst_libcamera_stream_formats_to_caps(const
> > libcamera::StreamFormats &formats);
> > > +
> > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */
> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build
> > > index 32d4233..39a34e7 100644
> > > --- a/src/gstreamer/meson.build
> > > +++ b/src/gstreamer/meson.build
> > > @@ -1,5 +1,6 @@
> > > libcamera_gst_sources = [
> > > 'gstlibcamera.c',
> > > + 'gstlibcamera-utils.cpp',
> >
> > Nitpicking, - comes before ., so this line should go first.
> >
> > > 'gstlibcamerasrc.cpp',
> > > ]
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list