<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Nicolas,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:<br>
> From: Nicolas Dufresne <<a href="mailto:nicolas.dufresne@collabora.com" target="_blank" rel="noreferrer">nicolas.dufresne@collabora.com</a>><br>
> <br>
> This transform the basic information found in StreamFormats to GstCaps.<br>
<br>
s/transform/transformts/<br>
<br>
> This can be handy to reply to early caps query or inside a device<br>
> provider. Note that we ignored generated range as they are harmful to<br>
> caps negotiation. We also don't simplify the caps for readability<br>
> reasons, so some of the discrete value may be included in a range.<br>
> <br>
> Signed-off-by: Nicolas Dufresne <<a href="mailto:nicolas.dufresne@collabora.com" target="_blank" rel="noreferrer">nicolas.dufresne@collabora.com</a>><br>
> ---<br>
> src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++<br>
> src/gstreamer/gstlibcamera-utils.h | 18 +++++<br>
> src/gstreamer/meson.build | 1 +<br>
> 3 files changed, 121 insertions(+)<br>
> create mode 100644 src/gstreamer/gstlibcamera-utils.cpp<br>
> create mode 100644 src/gstreamer/gstlibcamera-utils.h<br>
> <br>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp<br>
> new file mode 100644<br>
> index 0000000..2540be0<br>
> --- /dev/null<br>
> +++ b/src/gstreamer/gstlibcamera-utils.cpp<br>
> @@ -0,0 +1,102 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2020, Collabora Ltd.<br>
> + * Author: Nicolas Dufresne <<a href="mailto:nicolas.dufresne@collabora.com" target="_blank" rel="noreferrer">nicolas.dufresne@collabora.com</a>><br>
> + *<br>
> + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function<br>
> + */<br>
> +<br>
> +#include "gstlibcamera-utils.h"<br>
<br>
Could you add a blank line here, as per the coding style ?<br>
<br>
> +#include <linux/drm_fourcc.h><br>
> +<br>
> +using namespace libcamera;<br>
> +<br>
> +static struct {<br>
> + GstVideoFormat gst_format;<br>
> + guint drm_fourcc;<br>
> +} format_map[] = {<br>
<br>
Open question, should we use camelCase instead of snake_case ? I see<br>
you're mixing the two, after a quick glance with snake_case used for<br>
C-style functions and camelCase for C++ code. I don't mind that,<br>
especially given that the GStreamer API is snake_case. Should we add a<br>
README.md to the directory that documents this exception to the coding<br>
style ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },<br>
> + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },<br>
> + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },<br>
> + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },<br>
> + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },<br>
> + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },<br>
> + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },<br>
> + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },<br>
> + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },<br>
> + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */<br>
<br>
Can we try to avoid commented-out code ? Is this because<br>
GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I can add it, that's usually what I do when. I'm bored and need something easy.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },<br>
> + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },<br>
> + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },<br>
> + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },<br>
> +};<br>
> +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)<br>
> +<br>
> +static inline GstVideoFormat<br>
<br>
The compiler should be smart enough to inline functions where needed,<br>
you don't have to instruct it to do so specifically.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ok.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +drm_to_gst_format(guint drm_fourcc)<br>
> +{<br>
> + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)<br>
> + if (format_map[i].drm_fourcc == drm_fourcc)<br>
> + return format_map[i].gst_format;<br>
<br>
If you want to make this more C++, you can define format_map as an<br>
std::array, drop FORMAT_MAP_SIZE, and write<br>
<br>
for (const auto &format : format_map) {<br>
if (format.drm_fourcc == drm_fourcc)<br>
return format.gst_format;<br>
}<br>
<br>
> + return GST_VIDEO_FORMAT_UNKNOWN;<br>
> +}<br>
> +<br>
> +static GstStructure *<br>
> +bare_structure_from_fourcc(guint fourcc)<br>
> +{<br>
> + GstVideoFormat gst_format = drm_to_gst_format(fourcc);<br>
> +<br>
> + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)<br>
> + return NULL;<br>
<br>
C++ uses nullptr instead of NULL. This comment applies to the whole<br>
series.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I know, annoying to switch habit. All the remaining are the one I forgot.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + if (gst_format != GST_VIDEO_FORMAT_ENCODED)<br>
> + return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,<br>
> + gst_video_format_to_string(gst_format), NULL);<br>
> +<br>
> + switch (fourcc) {<br>
> + case DRM_FORMAT_MJPEG:<br>
> + return gst_structure_new_empty("image/jpeg");<br>
> + default:<br>
> + break;<br>
<br>
Maybe<br>
<br>
return nullptr;<br>
<br>
here and dropping the return at the end of the function ?<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I guess as I'm not using single return that could be better.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + }<br>
> +<br>
> + return NULL;<br>
> +}<br>
> +<br>
> +GstCaps *<br>
> +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)<br>
> +{<br>
> + GstCaps *caps = gst_caps_new_empty();<br>
> +<br>
> + for (unsigned int fourcc : formats.pixelformats()) {<br>
> + g_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(fourcc);<br>
<br>
g_autoptr is a really bad name when used in C++ code and the auto<br>
keyword :-( Nothing we can do about it though, but I think you should<br>
use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use<br>
unique_ptr extensively to convey ownership information.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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).</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + if (!bare_s) {<br>
> + GST_WARNING("Unsupported DRM format %" GST_FOURCC_FORMAT,<br>
> + GST_FOURCC_ARGS(fourcc));<br>
> + continue;<br>
> + }<br>
> +<br>
> + for (const Size &size : formats.sizes(fourcc)) {<br>
> + GstStructure *s = gst_structure_copy(bare_s);<br>
> + gst_structure_set(s,<br>
> + "width", G_TYPE_INT, size.width,<br>
> + "height", G_TYPE_INT, size.height,<br>
> + NULL);<br>
> + gst_caps_append_structure(caps, s);<br>
<br>
Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get<br>
the raw pointer, and add bare_s.release() here to avoid the automatic<br>
delete. This would avoid the need to call gst_structure_copy().<br>
<br>
And now that I've written this, I realise that unique_ptr will use<br>
std::default_delete by default, while you need a specific cleanup<br>
function. You could still use a unique_ptr with a custom deleter, but it<br>
may not be worth it. Shame g_autptr doesn't support the equivalent of<br>
std::unique_ptr::release() :-(<br>
<br>
> + }<br>
> +<br>
> + const SizeRange &range = formats.range(fourcc);<br>
> + if (range.hStep && range.vStep) {<br>
> + GstStructure *s = gst_structure_copy(bare_s);<br>
> +<br>
> + gst_structure_set(s,<br>
> + "width", GST_TYPE_INT_RANGE, range.min.width, range.max.width, range.hStep,<br>
> + "height", GST_TYPE_INT_RANGE, range.min.height, range.max.height, range.vStep,<br>
> + NULL);<br>
<br>
It would be nice if the gstreamer element could use the same line<br>
wrapping rules as the rest of the code.<br>
<br>
> + gst_caps_append_structure(caps, s);<br>
> + }<br>
> + }<br>
> +<br>
> + return caps;<br>
> +}<br>
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h<br>
> new file mode 100644<br>
> index 0000000..4545512<br>
> --- /dev/null<br>
> +++ b/src/gstreamer/gstlibcamera-utils.h<br>
> @@ -0,0 +1,18 @@<br>
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */<br>
> +/*<br>
> + * Copyright (C) 2020, Collabora Ltd.<br>
> + * Author: Nicolas Dufresne <<a href="mailto:nicolas.dufresne@collabora.com" target="_blank" rel="noreferrer">nicolas.dufresne@collabora.com</a>><br>
> + *<br>
> + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions<br>
> + */<br>
> +<br>
> +#include <gst/gst.h><br>
> +#include <gst/video/video.h><br>
> +<br>
> +#include <libcamera/stream.h><br>
> +<br>
<br>
These can go after the header guard too. This comment applies to the<br>
rest of the series.<br>
<br>
> +#ifndef __GST_LIBCAMERA_UTILS_H_<br>
<br>
Missing<br>
<br>
#define __GST_LIBCAMERA_UTILS_H_<br>
<br>
The rest of the code base has two _ at the end of the header guards, how<br>
about doing the same for all patches in this series ?<br>
<br>
> +<br>
> +GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);<br>
> +<br>
> +#endif /* __GST_LIBCAMERA_UTILS_H_ */<br>
> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build<br>
> index 32d4233..39a34e7 100644<br>
> --- a/src/gstreamer/meson.build<br>
> +++ b/src/gstreamer/meson.build<br>
> @@ -1,5 +1,6 @@<br>
> libcamera_gst_sources = [<br>
> 'gstlibcamera.c',<br>
> + 'gstlibcamera-utils.cpp',<br>
<br>
Nitpicking, - comes before ., so this line should go first.<br>
<br>
> 'gstlibcamerasrc.cpp',<br>
> ]<br>
> <br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div></div>