[libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert StreamFormats to GstCaps
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Feb 11 00:28:29 CET 2020
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 ?
> + { 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 ?
> + { 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.
> +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.
> +
> + 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 ?
> + }
> +
> + 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.
> +
> + 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