[libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert StreamFormats to GstCaps
Nicolas Dufresne
nicolas at ndufresne.ca
Tue Feb 11 00:54:32 CET 2020
Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> a écrit :
> Hi Nicolas,
>
> Thank you for the patch.
>
> 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/
>
> > 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.
> > + { 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 (!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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20200210/9bc98a56/attachment-0001.htm>
More information about the libcamera-devel
mailing list