[{"id":3649,"web_url":"https://patchwork.libcamera.org/comment/3649/","msgid":"<20200210232829.GC21893@pendragon.ideasonboard.com>","date":"2020-02-10T23:28:29","subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nThank you for the patch.\n\nOn Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> This transform the basic information found in StreamFormats to GstCaps.\n\ns/transform/transformts/\n\n> This can be handy to reply to early caps query or inside a device\n> provider. Note that we ignored generated range as they are harmful to\n> caps negotiation. We also don't simplify the caps for readability\n> reasons, so some of the discrete value may be included in a range.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++\n> src/gstreamer/gstlibcamera-utils.h | 18 +++++\n> src/gstreamer/meson.build | 1 +\n> 3 files changed, 121 insertions(+)\n> create mode 100644 src/gstreamer/gstlibcamera-utils.cpp\n> create mode 100644 src/gstreamer/gstlibcamera-utils.h\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> new file mode 100644\n> index 0000000..2540be0\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function\n> + */\n> +\n> +#include \"gstlibcamera-utils.h\"\n\nCould you add a blank line here, as per the coding style ?\n\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +static struct {\n> +\tGstVideoFormat gst_format;\n> +\tguint drm_fourcc;\n> +} format_map[] = {\n\nOpen question, should we use camelCase instead of snake_case ? I see\nyou're mixing the two, after a quick glance with snake_case used for\nC-style functions and camelCase for C++ code. I don't mind that,\nespecially given that the GStreamer API is snake_case. Should we add a\nREADME.md to the directory that documents this exception to the coding\nstyle ?\n\n> +\t{ GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },\n> +\t{ GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },\n> +\t{ GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },\n> +\t{ GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },\n> +\t{ GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },\n> +\t{ GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },\n> +\t{ GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },\n> +\t{ GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },\n> +\t{ GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },\n> +\t/* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */\n\nCan we try to avoid commented-out code ? Is this because\nGST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?\n\n> +\t{ GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },\n> +\t{ GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },\n> +\t{ GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },\n> +\t{ GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },\n> +};\n> +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)\n> +\n> +static inline GstVideoFormat\n\nThe compiler should be smart enough to inline functions where needed,\nyou don't have to instruct it to do so specifically.\n\n> +drm_to_gst_format(guint drm_fourcc)\n> +{\n> +\tfor (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> +\t\tif (format_map[i].drm_fourcc == drm_fourcc)\n> +\t\t\treturn format_map[i].gst_format;\n\nIf you want to make this more C++, you can define format_map as an\nstd::array, drop FORMAT_MAP_SIZE, and write\n\n\tfor (const auto &format : format_map) {\n\t\tif (format.drm_fourcc == drm_fourcc)\n\t\t\treturn format.gst_format;\n\t}\n\n> +\treturn GST_VIDEO_FORMAT_UNKNOWN;\n> +}\n> +\n> +static GstStructure *\n> +bare_structure_from_fourcc(guint fourcc)\n> +{\n> +\tGstVideoFormat gst_format = drm_to_gst_format(fourcc);\n> +\n> +\tif (gst_format == GST_VIDEO_FORMAT_UNKNOWN)\n> +\t\treturn NULL;\n\nC++ uses nullptr instead of NULL. This comment applies to the whole\nseries.\n\n> +\n> +\tif (gst_format != GST_VIDEO_FORMAT_ENCODED)\n> +\t\treturn gst_structure_new(\"video/x-raw\", \"format\", G_TYPE_STRING,\n> +\t\t\t\t\t gst_video_format_to_string(gst_format), NULL);\n> +\n> +\tswitch (fourcc) {\n> +\tcase DRM_FORMAT_MJPEG:\n> +\t\treturn gst_structure_new_empty(\"image/jpeg\");\n> +\tdefault:\n> +\t\tbreak;\n\nMaybe\n\n\t\treturn nullptr;\n\nhere and dropping the return at the end of the function ?\n\n> +\t}\n> +\n> +\treturn NULL;\n> +}\n> +\n> +GstCaps *\n> +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> +{\n> +\tGstCaps *caps = gst_caps_new_empty();\n> +\n> +\tfor (unsigned int fourcc : formats.pixelformats()) {\n> +\t\tg_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(fourcc);\n\ng_autoptr is a really bad name when used in C++ code and the auto\nkeyword :-( Nothing we can do about it though, but I think you should\nuse std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use\nunique_ptr extensively to convey ownership information.\n\n> +\n> +\t\tif (!bare_s) {\n> +\t\t\tGST_WARNING(\"Unsupported DRM format %\" GST_FOURCC_FORMAT,\n> +\t\t\t\t GST_FOURCC_ARGS(fourcc));\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tfor (const Size &size : formats.sizes(fourcc)) {\n> +\t\t\tGstStructure *s = gst_structure_copy(bare_s);\n> +\t\t\tgst_structure_set(s,\n> +\t\t\t\t\t \"width\", G_TYPE_INT, size.width,\n> +\t\t\t\t\t \"height\", G_TYPE_INT, size.height,\n> +\t\t\t\t\t NULL);\n> +\t\t\tgst_caps_append_structure(caps, s);\n\nUnless I'm mistaken, with unique_ptr, you could use bare_s.get() to get\nthe raw pointer, and add bare_s.release() here to avoid the automatic\ndelete. This would avoid the need to call gst_structure_copy().\n\nAnd now that I've written this, I realise that unique_ptr will use\nstd::default_delete by default, while you need a specific cleanup\nfunction. You could still use a unique_ptr with a custom deleter, but it\nmay not be worth it. Shame g_autptr doesn't support the equivalent of\nstd::unique_ptr::release() :-(\n\n> +\t\t}\n> +\n> +\t\tconst SizeRange &range = formats.range(fourcc);\n> +\t\tif (range.hStep && range.vStep) {\n> +\t\t\tGstStructure *s = gst_structure_copy(bare_s);\n> +\n> +\t\t\tgst_structure_set(s,\n> +\t\t\t\t\"width\", GST_TYPE_INT_RANGE, range.min.width, range.max.width, range.hStep,\n> +\t\t\t\t\"height\", GST_TYPE_INT_RANGE, range.min.height, range.max.height, range.vStep,\n> +\t\t\t\tNULL);\n\nIt would be nice if the gstreamer element could use the same line\nwrapping rules as the rest of the code.\n\n> +\t\t\tgst_caps_append_structure(caps, s);\n> +\t\t}\n> +\t}\n> +\n> +\treturn caps;\n> +}\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> new file mode 100644\n> index 0000000..4545512\n> --- /dev/null\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -0,0 +1,18 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Collabora Ltd.\n> + * Author: Nicolas Dufresne \n> + *\n> + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions\n> + */\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n\nThese can go after the header guard too. This comment applies to the\nrest of the series.\n\n> +#ifndef __GST_LIBCAMERA_UTILS_H_\n\nMissing\n\n#define __GST_LIBCAMERA_UTILS_H_\n\nThe rest of the code base has two _ at the end of the header guards, how\nabout doing the same for all patches in this series ?\n\n> +\n> +GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);\n> +\n> +#endif /* __GST_LIBCAMERA_UTILS_H_ */\n> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> index 32d4233..39a34e7 100644\n> --- a/src/gstreamer/meson.build\n> +++ b/src/gstreamer/meson.build\n> @@ -1,5 +1,6 @@\n> libcamera_gst_sources = [\n> 'gstlibcamera.c',\n> + 'gstlibcamera-utils.cpp',\n\nNitpicking, - comes before ., so this line should go first.\n\n> 'gstlibcamerasrc.cpp',\n> ]\n>","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A102609D7\n\tfor ;\n\tTue, 11 Feb 2020 00:28:45 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9EE2652A;\n\tTue, 11 Feb 2020 00:28:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581377324;\n\tbh=E+9ns4JVm/4SdwTySoHuPBixtMm65tdGdPlFzkUWJxQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EAPx2M93hQc4ZN58kPpQo7xIsegdNY53amJ5dWAWadogfJhdxipsh2wKN9BNe9mAj\n\tdHahS/eLUYhDheEMI06IvcPLqZxPzxMJJ/2jtjwpSQoPtbSQ+KtwPAQ9/oq1/yyeNP\n\tsbDmuROjq2f/8xsJJxk7PM+69ATK0kIOAZFqoer8=","Date":"Tue, 11 Feb 2020 01:28:29 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200210232829.GC21893@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-3-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-3-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Feb 2020 23:28:45 -0000"}},{"id":3650,"web_url":"https://patchwork.libcamera.org/comment/3650/","msgid":"","date":"2020-02-10T23:54:32","subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> a écrit :\n\n> Hi Nicolas,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:\n> > From: Nicolas Dufresne \n> >\n> > This transform the basic information found in StreamFormats to GstCaps.\n>\n> s/transform/transformts/\n>\n> > This can be handy to reply to early caps query or inside a device\n> > provider. Note that we ignored generated range as they are harmful to\n> > caps negotiation. We also don't simplify the caps for readability\n> > reasons, so some of the discrete value may be included in a range.\n> >\n> > Signed-off-by: Nicolas Dufresne \n> > ---\n> > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++\n> > src/gstreamer/gstlibcamera-utils.h | 18 +++++\n> > src/gstreamer/meson.build | 1 +\n> > 3 files changed, 121 insertions(+)\n> > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp\n> > create mode 100644 src/gstreamer/gstlibcamera-utils.h\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> b/src/gstreamer/gstlibcamera-utils.cpp\n> > new file mode 100644\n> > index 0000000..2540be0\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -0,0 +1,102 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function\n> > + */\n> > +\n> > +#include \"gstlibcamera-utils.h\"\n>\n> Could you add a blank line here, as per the coding style ?\n>\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +static struct {\n> > + GstVideoFormat gst_format;\n> > + guint drm_fourcc;\n> > +} format_map[] = {\n>\n> Open question, should we use camelCase instead of snake_case ? I see\n> you're mixing the two, after a quick glance with snake_case used for\n> C-style functions and camelCase for C++ code. I don't mind that,\n> especially given that the GStreamer API is snake_case. Should we add a\n> README.md to the directory that documents this exception to the coding\n> style ?\n>\n\nI've use camel only for c++ object method, everything else follow GST style\nfor naming, types are Camel func and var lower case snake. When note, I\nmade typo, that happens. It depends if you intend to keep the plugin\nforever or want in in GST long term.\n\n\n> > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },\n> > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },\n> > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },\n> > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },\n> > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },\n> > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },\n> > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },\n> > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },\n> > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },\n> > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */\n>\n> Can we try to avoid commented-out code ? Is this because\n> GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?\n>\n\nI can add it, that's usually what I do when. I'm bored and need something\neasy.\n\n\n> > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },\n> > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },\n> > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },\n> > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },\n> > +};\n> > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)\n> > +\n> > +static inline GstVideoFormat\n>\n> The compiler should be smart enough to inline functions where needed,\n> you don't have to instruct it to do so specifically.\n>\n\nOk.\n\n\n> > +drm_to_gst_format(guint drm_fourcc)\n> > +{\n> > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> > + if (format_map[i].drm_fourcc == drm_fourcc)\n> > + return format_map[i].gst_format;\n>\n> If you want to make this more C++, you can define format_map as an\n> std::array, drop FORMAT_MAP_SIZE, and write\n>\n> for (const auto &format : format_map) {\n> if (format.drm_fourcc == drm_fourcc)\n> return format.gst_format;\n> }\n>\n> > + return GST_VIDEO_FORMAT_UNKNOWN;\n> > +}\n> > +\n> > +static GstStructure *\n> > +bare_structure_from_fourcc(guint fourcc)\n> > +{\n> > + GstVideoFormat gst_format = drm_to_gst_format(fourcc);\n> > +\n> > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)\n> > + return NULL;\n>\n> C++ uses nullptr instead of NULL. This comment applies to the whole\n> series.\n>\n\nI know, annoying to switch habit. All the remaining are the one I forgot.\n\n\n> > +\n> > + if (gst_format != GST_VIDEO_FORMAT_ENCODED)\n> > + return gst_structure_new(\"video/x-raw\", \"format\",\n> G_TYPE_STRING,\n> > +\n> gst_video_format_to_string(gst_format), NULL);\n> > +\n> > + switch (fourcc) {\n> > + case DRM_FORMAT_MJPEG:\n> > + return gst_structure_new_empty(\"image/jpeg\");\n> > + default:\n> > + break;\n>\n> Maybe\n>\n> return nullptr;\n>\n> here and dropping the return at the end of the function ?\n>\n\nI guess as I'm not using single return that could be better.\n\n\n> > + }\n> > +\n> > + return NULL;\n> > +}\n> > +\n> > +GstCaps *\n> > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> > +{\n> > + GstCaps *caps = gst_caps_new_empty();\n> > +\n> > + for (unsigned int fourcc : formats.pixelformats()) {\n> > + g_autoptr(GstStructure) bare_s =\n> bare_structure_from_fourcc(fourcc);\n>\n> g_autoptr is a really bad name when used in C++ code and the auto\n> keyword :-( Nothing we can do about it though, but I think you should\n> use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use\n> unique_ptr extensively to convey ownership information.\n>\n\nDo you have an example for that using a third party refcount system? If we\ndon't use GStreamer / Glib recounts, we will break many assumption, and\ncause hard to catch errors. Each type have its own cleanup function, we\nknow how to expend to that, but macros are needed anyway. I think your\nrequest is a lot of work, we need multiple different types of wrapper\nclass, since this is not all about GObject (GstStructure is a red counted\nboxed type).\n\n\n> > +\n> > + if (!bare_s) {\n> > + GST_WARNING(\"Unsupported DRM format %\"\n> GST_FOURCC_FORMAT,\n> > + GST_FOURCC_ARGS(fourcc));\n> > + continue;\n> > + }\n> > +\n> > + for (const Size &size : formats.sizes(fourcc)) {\n> > + GstStructure *s = gst_structure_copy(bare_s);\n> > + gst_structure_set(s,\n> > + \"width\", G_TYPE_INT, size.width,\n> > + \"height\", G_TYPE_INT,\n> size.height,\n> > + NULL);\n> > + gst_caps_append_structure(caps, s);\n>\n> Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get\n> the raw pointer, and add bare_s.release() here to avoid the automatic\n> delete. This would avoid the need to call gst_structure_copy().\n>\n> And now that I've written this, I realise that unique_ptr will use\n> std::default_delete by default, while you need a specific cleanup\n> function. You could still use a unique_ptr with a custom deleter, but it\n> may not be worth it. Shame g_autptr doesn't support the equivalent of\n> std::unique_ptr::release() :-(\n>\n> > + }\n> > +\n> > + const SizeRange &range = formats.range(fourcc);\n> > + if (range.hStep && range.vStep) {\n> > + GstStructure *s = gst_structure_copy(bare_s);\n> > +\n> > + gst_structure_set(s,\n> > + \"width\", GST_TYPE_INT_RANGE,\n> range.min.width, range.max.width, range.hStep,\n> > + \"height\", GST_TYPE_INT_RANGE,\n> range.min.height, range.max.height, range.vStep,\n> > + NULL);\n>\n> It would be nice if the gstreamer element could use the same line\n> wrapping rules as the rest of the code.\n>\n> > + gst_caps_append_structure(caps, s);\n> > + }\n> > + }\n> > +\n> > + return caps;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> b/src/gstreamer/gstlibcamera-utils.h\n> > new file mode 100644\n> > index 0000000..4545512\n> > --- /dev/null\n> > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > @@ -0,0 +1,18 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Collabora Ltd.\n> > + * Author: Nicolas Dufresne \n> > + *\n> > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n>\n> These can go after the header guard too. This comment applies to the\n> rest of the series.\n>\n> > +#ifndef __GST_LIBCAMERA_UTILS_H_\n>\n> Missing\n>\n> #define __GST_LIBCAMERA_UTILS_H_\n>\n> The rest of the code base has two _ at the end of the header guards, how\n> about doing the same for all patches in this series ?\n>\n> > +\n> > +GstCaps *gst_libcamera_stream_formats_to_caps(const\n> libcamera::StreamFormats &formats);\n> > +\n> > +#endif /* __GST_LIBCAMERA_UTILS_H_ */\n> > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > index 32d4233..39a34e7 100644\n> > --- a/src/gstreamer/meson.build\n> > +++ b/src/gstreamer/meson.build\n> > @@ -1,5 +1,6 @@\n> > libcamera_gst_sources = [\n> > 'gstlibcamera.c',\n> > + 'gstlibcamera-utils.cpp',\n>\n> Nitpicking, - comes before ., so this line should go first.\n>\n> > 'gstlibcamerasrc.cpp',\n> > ]\n> >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","Received":["from mail-pg1-x541.google.com (mail-pg1-x541.google.com\n\t[IPv6:2607:f8b0:4864:20::541])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D0C8609D7\n\tfor ;\n\tTue, 11 Feb 2020 00:54:45 +0100 (CET)","by mail-pg1-x541.google.com with SMTP id b35so4706798pgm.13\n\tfor ;\n\tMon, 10 Feb 2020 15:54:45 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20150623.gappssmtp.com; s=20150623;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oFYFgnutN0fA3DDeIBGE3W2hTqa7oYnwDOoVF9NRaKc=;\n\tb=ekjiPP79wBcnF4jtNB48NlyMy1E1mEzp1pI8xUQA8TrfCI9/d6bQzcWYMfhuBEtseY\n\t09hEr16Quuccl99x1dpSFIawEjmPzxCqk79G4bJjqsDJsoiOLiAQ5coe1QT3fkafn+DR\n\tfp2IWrXdcEREGgfEU7Zg6j90ZQuE7VAiO132VnxM/1PiwgkAjJMhyXgZeSkBg6nz9Mvu\n\ttSDngAFkLvpsWIO8G15CRrg2/bla3HwDfBgQ+PT2Z1p2EQBIy2oft4HKMP0llj8g6ZJU\n\twfoGMP2WyLqMCFgOFUp3yFNgZvENaVcuBzcVsdnP2z7w6o1TFJRclsuRZkbMxmQe6VXq\n\tX8dA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oFYFgnutN0fA3DDeIBGE3W2hTqa7oYnwDOoVF9NRaKc=;\n\tb=lo9o8h7IJud43dJimvN3VyF0SDFPlU72+73Xa+EcDaYmv2nsuytHzWFgds/6LUwUTK\n\t96IyP7Lvvt2wwJ33ASGe/+2cye5bAuaS4y0iJjmwgRlaI9AT4YSQeTzR1YBzyat+5JtP\n\tNyf4VeQADFxqYuPdBE1sbl5lQl1poaK6GEhbTPodKOKIXT6I6CNukBh1IhfO/+0Mg5ah\n\tMgYb7PGpcYcsURUFB/t2MZP5FNM825yvNeLbarKcjbdqwRmaGaNfiBDTzU3GbrRZdIgk\n\tPBniPzjDy7Nmh49Zt/yhFLSreQ/6ga3tRq+Irh/Z+xgZqgIesoiHDXqpA4j/j7yJ5nwI\n\tb0eg==","X-Gm-Message-State":"APjAAAXWxzDYe2Xxke50B8h3rgPXlQt4wdVSn9k6SPkfgV+/SALWY9rW\n\tU/ouYtzuNeJlckG/2pVLeM7H6DnztlL/m6dwU4PlcA==","X-Google-Smtp-Source":"APXvYqwyNKN4ablhyu8vrsFEswAOlKqXTdWFN70YHq0x8rc1kzs+HiWmSC54IPpcIo86JSz6ATQ2zjHLOm4rueg+NAA=","X-Received":"by 2002:a63:8743:: with SMTP id i64mr285883pge.243.1581378883480;\n\tMon, 10 Feb 2020 15:54:43 -0800 (PST)","MIME-Version":"1.0","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-3-nicolas@ndufresne.ca>\n\t<20200210232829.GC21893@pendragon.ideasonboard.com>","In-Reply-To":"<20200210232829.GC21893@pendragon.ideasonboard.com>","From":"Nicolas Dufresne ","Date":"Mon, 10 Feb 2020 18:54:32 -0500","Message-ID":"","To":"Laurent Pinchart ","Cc":"LibCamera Devel , \n\tNicolas Dufresne ","Content-Type":"multipart/alternative; boundary=\"0000000000009b0197059e41765c\"","Subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 10 Feb 2020 23:54:45 -0000"}},{"id":3656,"web_url":"https://patchwork.libcamera.org/comment/3656/","msgid":"<20200211163410.GI22612@pendragon.ideasonboard.com>","date":"2020-02-11T16:34:10","subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote:\n> Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit :\n> > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:\n> > > From: Nicolas Dufresne \n> > >\n> > > This transform the basic information found in StreamFormats to GstCaps.\n> >\n> > s/transform/transformts/\n\nWithout the last t of course :-)\n\n> > > This can be handy to reply to early caps query or inside a device\n> > > provider. Note that we ignored generated range as they are harmful to\n> > > caps negotiation. We also don't simplify the caps for readability\n> > > reasons, so some of the discrete value may be included in a range.\n> > >\n> > > Signed-off-by: Nicolas Dufresne \n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++\n> > > src/gstreamer/gstlibcamera-utils.h | 18 +++++\n> > > src/gstreamer/meson.build | 1 +\n> > > 3 files changed, 121 insertions(+)\n> > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp\n> > > create mode 100644 src/gstreamer/gstlibcamera-utils.h\n> > >\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > new file mode 100644\n> > > index 0000000..2540be0\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > @@ -0,0 +1,102 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function\n> > > + */\n> > > +\n> > > +#include \"gstlibcamera-utils.h\"\n> >\n> > Could you add a blank line here, as per the coding style ?\n> >\n> > > +#include \n> > > +\n> > > +using namespace libcamera;\n> > > +\n> > > +static struct {\n> > > + GstVideoFormat gst_format;\n> > > + guint drm_fourcc;\n> > > +} format_map[] = {\n> >\n> > Open question, should we use camelCase instead of snake_case ? I see\n> > you're mixing the two, after a quick glance with snake_case used for\n> > C-style functions and camelCase for C++ code. I don't mind that,\n> > especially given that the GStreamer API is snake_case. Should we add a\n> > README.md to the directory that documents this exception to the coding\n> > style ?\n> \n> I've use camel only for c++ object method, everything else follow GST style\n> for naming, types are Camel func and var lower case snake. When note, I\n> made typo, that happens. It depends if you intend to keep the plugin\n> forever or want in in GST long term.\n\nI think it makes sense to plan for merging it in gstreamer, so a custom\ncoding style is fine by me, as long as we document it clearly.\n\n> > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },\n> > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },\n> > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },\n> > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },\n> > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },\n> > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },\n> > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },\n> > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },\n> > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },\n> > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */\n> >\n> > Can we try to avoid commented-out code ? Is this because\n> > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?\n> \n> I can add it, that's usually what I do when. I'm bored and need something\n> easy.\n> \n> > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },\n> > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },\n> > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },\n> > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },\n> > > +};\n> > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)\n> > > +\n> > > +static inline GstVideoFormat\n> >\n> > The compiler should be smart enough to inline functions where needed,\n> > you don't have to instruct it to do so specifically.\n> \n> Ok.\n> \n> > > +drm_to_gst_format(guint drm_fourcc)\n> > > +{\n> > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> > > + if (format_map[i].drm_fourcc == drm_fourcc)\n> > > + return format_map[i].gst_format;\n> >\n> > If you want to make this more C++, you can define format_map as an\n> > std::array, drop FORMAT_MAP_SIZE, and write\n> >\n> > for (const auto &format : format_map) {\n> > if (format.drm_fourcc == drm_fourcc)\n> > return format.gst_format;\n> > }\n> >\n> > > + return GST_VIDEO_FORMAT_UNKNOWN;\n> > > +}\n> > > +\n> > > +static GstStructure *\n> > > +bare_structure_from_fourcc(guint fourcc)\n> > > +{\n> > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc);\n> > > +\n> > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)\n> > > + return NULL;\n> >\n> > C++ uses nullptr instead of NULL. This comment applies to the whole\n> > series.\n> \n> I know, annoying to switch habit. All the remaining are the one I forgot.\n> \n> > > +\n> > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED)\n> > > + return gst_structure_new(\"video/x-raw\", \"format\", G_TYPE_STRING,\n> > > +\n> > gst_video_format_to_string(gst_format), NULL);\n> > > +\n> > > + switch (fourcc) {\n> > > + case DRM_FORMAT_MJPEG:\n> > > + return gst_structure_new_empty(\"image/jpeg\");\n> > > + default:\n> > > + break;\n> >\n> > Maybe\n> >\n> > return nullptr;\n> >\n> > here and dropping the return at the end of the function ?\n> >\n> \n> I guess as I'm not using single return that could be better.\n> \n> > > + }\n> > > +\n> > > + return NULL;\n> > > +}\n> > > +\n> > > +GstCaps *\n> > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> > > +{\n> > > + GstCaps *caps = gst_caps_new_empty();\n> > > +\n> > > + for (unsigned int fourcc : formats.pixelformats()) {\n> > > + g_autoptr(GstStructure) bare_s =\n> > bare_structure_from_fourcc(fourcc);\n> >\n> > g_autoptr is a really bad name when used in C++ code and the auto\n> > keyword :-( Nothing we can do about it though, but I think you should\n> > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use\n> > unique_ptr extensively to convey ownership information.\n> \n> Do you have an example for that using a third party refcount system? If we\n> don't use GStreamer / Glib recounts, we will break many assumption, and\n> cause hard to catch errors. Each type have its own cleanup function, we\n> know how to expend to that, but macros are needed anyway. I think your\n> request is a lot of work, we need multiple different types of wrapper\n> class, since this is not all about GObject (GstStructure is a red counted\n> boxed type).\n\nIf I understand glib right (which is far from a given :-)), this would\nbecome\n\nstatic std::unique_ptr\nbare_structure_from_fourcc(guint fourcc)\n{\n\t...\n\treturn std::unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n}\n\nand here\n\n\tauto bare_s = bare_structure_from_fourcc(fourcc);\n\nYou could simplify this with\n\ntemplate\nusing gst_unique_ptr = std::unique_ptr;\n\nstatic gst_unique_ptr bare_structure_from_fourcc(guint fourcc)\n{\n\t...\n\treturn gst_unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n}\n\n...\n\n\tgst_unique_ptr bare_s = bare_structure_from_fourcc(fourcc);\n\n(not test-compiled though)\n\n> > > +\n> > > + if (!bare_s) {\n> > > + GST_WARNING(\"Unsupported DRM format %\"\n> > GST_FOURCC_FORMAT,\n> > > + GST_FOURCC_ARGS(fourcc));\n> > > + continue;\n> > > + }\n> > > +\n> > > + for (const Size &size : formats.sizes(fourcc)) {\n> > > + GstStructure *s = gst_structure_copy(bare_s);\n> > > + gst_structure_set(s,\n> > > + \"width\", G_TYPE_INT, size.width,\n> > > + \"height\", G_TYPE_INT,\n> > size.height,\n> > > + NULL);\n> > > + gst_caps_append_structure(caps, s);\n> >\n> > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get\n> > the raw pointer, and add bare_s.release() here to avoid the automatic\n> > delete. This would avoid the need to call gst_structure_copy().\n> >\n> > And now that I've written this, I realise that unique_ptr will use\n> > std::default_delete by default, while you need a specific cleanup\n> > function. You could still use a unique_ptr with a custom deleter, but it\n> > may not be worth it. Shame g_autptr doesn't support the equivalent of\n> > std::unique_ptr::release() :-(\n> >\n> > > + }\n> > > +\n> > > + const SizeRange &range = formats.range(fourcc);\n> > > + if (range.hStep && range.vStep) {\n> > > + GstStructure *s = gst_structure_copy(bare_s);\n> > > +\n> > > + gst_structure_set(s,\n> > > + \"width\", GST_TYPE_INT_RANGE,\n> > range.min.width, range.max.width, range.hStep,\n> > > + \"height\", GST_TYPE_INT_RANGE,\n> > range.min.height, range.max.height, range.vStep,\n> > > + NULL);\n> >\n> > It would be nice if the gstreamer element could use the same line\n> > wrapping rules as the rest of the code.\n> >\n> > > + gst_caps_append_structure(caps, s);\n> > > + }\n> > > + }\n> > > +\n> > > + return caps;\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> > > new file mode 100644\n> > > index 0000000..4545512\n> > > --- /dev/null\n> > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > @@ -0,0 +1,18 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2020, Collabora Ltd.\n> > > + * Author: Nicolas Dufresne \n> > > + *\n> > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> >\n> > These can go after the header guard too. This comment applies to the\n> > rest of the series.\n> >\n> > > +#ifndef __GST_LIBCAMERA_UTILS_H_\n> >\n> > Missing\n> >\n> > #define __GST_LIBCAMERA_UTILS_H_\n> >\n> > The rest of the code base has two _ at the end of the header guards, how\n> > about doing the same for all patches in this series ?\n> >\n> > > +\n> > > +GstCaps *gst_libcamera_stream_formats_to_caps(const\n> > libcamera::StreamFormats &formats);\n> > > +\n> > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */\n> > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > index 32d4233..39a34e7 100644\n> > > --- a/src/gstreamer/meson.build\n> > > +++ b/src/gstreamer/meson.build\n> > > @@ -1,5 +1,6 @@\n> > > libcamera_gst_sources = [\n> > > 'gstlibcamera.c',\n> > > + 'gstlibcamera-utils.cpp',\n> >\n> > Nitpicking, - comes before ., so this line should go first.\n> >\n> > > 'gstlibcamerasrc.cpp',\n> > > ]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C03D660F3C\n\tfor ;\n\tTue, 11 Feb 2020 17:34:25 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3C9F5808;\n\tTue, 11 Feb 2020 17:34:25 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581438865;\n\tbh=aPMH6JG7dr/TgPtxDwTRmsmdYRHqoaZUV8MIZK9+f/U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TMlCnzcJfvxIgSlg0RibHoX3JWeOGK8SisgHPqiqc/KdmW0ZML5fnYC/ZVnaeROyG\n\tRBkBkJtnyc70aC8jjbucBl0JYyb+0q3wWtdxXw8kdBs57hnWgaP0Bim2YEJz9gC2Vd\n\tM46P2+epOB0Sndnt8m/vf1HyvLHnWsGoUY0xDx84=","Date":"Tue, 11 Feb 2020 18:34:10 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"LibCamera Devel ,\n\tNicolas Dufresne ","Message-ID":"<20200211163410.GI22612@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-3-nicolas@ndufresne.ca>\n\t<20200210232829.GC21893@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Feb 2020 16:34:25 -0000"}},{"id":3657,"web_url":"https://patchwork.libcamera.org/comment/3657/","msgid":"<8a5a1bb941b86bbaf49ab9c6b101247ebdf9d1df.camel@collabora.com>","date":"2020-02-11T16:52:48","subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","submitter":{"id":31,"url":"https://patchwork.libcamera.org/api/people/31/","name":"Nicolas Dufresne","email":"nicolas.dufresne@collabora.com"},"content":"On mar, 2020-02-11 at 18:34 +0200, Laurent Pinchart wrote:\n> Hi Nicolas,\n> \n> On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote:\n> > Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit :\n> > > On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:\n> > > > From: Nicolas Dufresne \n> > > > \n> > > > This transform the basic information found in StreamFormats to GstCaps.\n> > > \n> > > s/transform/transformts/\n> \n> Without the last t of course :-)\n> \n> > > > This can be handy to reply to early caps query or inside a device\n> > > > provider. Note that we ignored generated range as they are harmful to\n> > > > caps negotiation. We also don't simplify the caps for readability\n> > > > reasons, so some of the discrete value may be included in a range.\n> > > > \n> > > > Signed-off-by: Nicolas Dufresne \n> > > > ---\n> > > > src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++\n> > > > src/gstreamer/gstlibcamera-utils.h | 18 +++++\n> > > > src/gstreamer/meson.build | 1 +\n> > > > 3 files changed, 121 insertions(+)\n> > > > create mode 100644 src/gstreamer/gstlibcamera-utils.cpp\n> > > > create mode 100644 src/gstreamer/gstlibcamera-utils.h\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > new file mode 100644\n> > > > index 0000000..2540be0\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > @@ -0,0 +1,102 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Collabora Ltd.\n> > > > + * Author: Nicolas Dufresne \n> > > > + *\n> > > > + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function\n> > > > + */\n> > > > +\n> > > > +#include \"gstlibcamera-utils.h\"\n> > > \n> > > Could you add a blank line here, as per the coding style ?\n> > > \n> > > > +#include \n> > > > +\n> > > > +using namespace libcamera;\n> > > > +\n> > > > +static struct {\n> > > > + GstVideoFormat gst_format;\n> > > > + guint drm_fourcc;\n> > > > +} format_map[] = {\n> > > \n> > > Open question, should we use camelCase instead of snake_case ? I see\n> > > you're mixing the two, after a quick glance with snake_case used for\n> > > C-style functions and camelCase for C++ code. I don't mind that,\n> > > especially given that the GStreamer API is snake_case. Should we add a\n> > > README.md to the directory that documents this exception to the coding\n> > > style ?\n> > \n> > I've use camel only for c++ object method, everything else follow GST style\n> > for naming, types are Camel func and var lower case snake. When note, I\n> > made typo, that happens. It depends if you intend to keep the plugin\n> > forever or want in in GST long term.\n> \n> I think it makes sense to plan for merging it in gstreamer, so a custom\n> coding style is fine by me, as long as we document it clearly.\n> \n> > > > + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },\n> > > > + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },\n> > > > + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },\n> > > > + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },\n> > > > + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },\n> > > > + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },\n> > > > + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },\n> > > > + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },\n> > > > + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },\n> > > > + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */\n> > > \n> > > Can we try to avoid commented-out code ? Is this because\n> > > GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?\n> > \n> > I can add it, that's usually what I do when. I'm bored and need something\n> > easy.\n> > \n> > > > + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },\n> > > > + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },\n> > > > + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },\n> > > > + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },\n> > > > +};\n> > > > +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)\n> > > > +\n> > > > +static inline GstVideoFormat\n> > > \n> > > The compiler should be smart enough to inline functions where needed,\n> > > you don't have to instruct it to do so specifically.\n> > \n> > Ok.\n> > \n> > > > +drm_to_gst_format(guint drm_fourcc)\n> > > > +{\n> > > > + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> > > > + if (format_map[i].drm_fourcc == drm_fourcc)\n> > > > + return format_map[i].gst_format;\n> > > \n> > > If you want to make this more C++, you can define format_map as an\n> > > std::array, drop FORMAT_MAP_SIZE, and write\n> > > \n> > > for (const auto &format : format_map) {\n> > > if (format.drm_fourcc == drm_fourcc)\n> > > return format.gst_format;\n> > > }\n> > > \n> > > > + return GST_VIDEO_FORMAT_UNKNOWN;\n> > > > +}\n> > > > +\n> > > > +static GstStructure *\n> > > > +bare_structure_from_fourcc(guint fourcc)\n> > > > +{\n> > > > + GstVideoFormat gst_format = drm_to_gst_format(fourcc);\n> > > > +\n> > > > + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)\n> > > > + return NULL;\n> > > \n> > > C++ uses nullptr instead of NULL. This comment applies to the whole\n> > > series.\n> > \n> > I know, annoying to switch habit. All the remaining are the one I forgot.\n> > \n> > > > +\n> > > > + if (gst_format != GST_VIDEO_FORMAT_ENCODED)\n> > > > + return gst_structure_new(\"video/x-raw\", \"format\", G_TYPE_STRING,\n> > > > +\n> > > gst_video_format_to_string(gst_format), NULL);\n> > > > +\n> > > > + switch (fourcc) {\n> > > > + case DRM_FORMAT_MJPEG:\n> > > > + return gst_structure_new_empty(\"image/jpeg\");\n> > > > + default:\n> > > > + break;\n> > > \n> > > Maybe\n> > > \n> > > return nullptr;\n> > > \n> > > here and dropping the return at the end of the function ?\n> > > \n> > \n> > I guess as I'm not using single return that could be better.\n> > \n> > > > + }\n> > > > +\n> > > > + return NULL;\n> > > > +}\n> > > > +\n> > > > +GstCaps *\n> > > > +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> > > > +{\n> > > > + GstCaps *caps = gst_caps_new_empty();\n> > > > +\n> > > > + for (unsigned int fourcc : formats.pixelformats()) {\n> > > > + g_autoptr(GstStructure) bare_s =\n> > > bare_structure_from_fourcc(fourcc);\n> > > \n> > > g_autoptr is a really bad name when used in C++ code and the auto\n> > > keyword :-( Nothing we can do about it though, but I think you should\n> > > use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use\n> > > unique_ptr extensively to convey ownership information.\n> > \n> > Do you have an example for that using a third party refcount system? If we\n> > don't use GStreamer / Glib recounts, we will break many assumption, and\n> > cause hard to catch errors. Each type have its own cleanup function, we\n> > know how to expend to that, but macros are needed anyway. I think your\n> > request is a lot of work, we need multiple different types of wrapper\n> > class, since this is not all about GObject (GstStructure is a red counted\n> > boxed type).\n> \n> If I understand glib right (which is far from a given :-)), this would\n> become\n> \n> static std::unique_ptr\n\nThat is very slippery to call private macros. Private macros are not part of the\nAPI, if you use them, you are on your own. I'm really not convince by this.\n\n> bare_structure_from_fourcc(guint fourcc)\n> {\n> \t...\n> \treturn std::unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n> }\n> \n> and here\n> \n> \tauto bare_s = bare_structure_from_fourcc(fourcc);\n> \n> You could simplify this with\n> \n> template\n> using gst_unique_ptr = std::unique_ptr;\n> \n> static gst_unique_ptr bare_structure_from_fourcc(guint fourcc)\n> {\n> \t...\n> \treturn gst_unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n> }\n> \n> ...\n> \n> \tgst_unique_ptr bare_s = bare_structure_from_fourcc(fourcc);\n> \n> (not test-compiled though)\n> \n> > > > +\n> > > > + if (!bare_s) {\n> > > > + GST_WARNING(\"Unsupported DRM format %\"\n> > > GST_FOURCC_FORMAT,\n> > > > + GST_FOURCC_ARGS(fourcc));\n> > > > + continue;\n> > > > + }\n> > > > +\n> > > > + for (const Size &size : formats.sizes(fourcc)) {\n> > > > + GstStructure *s = gst_structure_copy(bare_s);\n> > > > + gst_structure_set(s,\n> > > > + \"width\", G_TYPE_INT, size.width,\n> > > > + \"height\", G_TYPE_INT,\n> > > size.height,\n> > > > + NULL);\n> > > > + gst_caps_append_structure(caps, s);\n> > > \n> > > Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get\n> > > the raw pointer, and add bare_s.release() here to avoid the automatic\n> > > delete. This would avoid the need to call gst_structure_copy().\n> > > \n> > > And now that I've written this, I realise that unique_ptr will use\n> > > std::default_delete by default, while you need a specific cleanup\n> > > function. You could still use a unique_ptr with a custom deleter, but it\n> > > may not be worth it. Shame g_autptr doesn't support the equivalent of\n> > > std::unique_ptr::release() :-(\n> > > \n> > > > + }\n> > > > +\n> > > > + const SizeRange &range = formats.range(fourcc);\n> > > > + if (range.hStep && range.vStep) {\n> > > > + GstStructure *s = gst_structure_copy(bare_s);\n> > > > +\n> > > > + gst_structure_set(s,\n> > > > + \"width\", GST_TYPE_INT_RANGE,\n> > > range.min.width, range.max.width, range.hStep,\n> > > > + \"height\", GST_TYPE_INT_RANGE,\n> > > range.min.height, range.max.height, range.vStep,\n> > > > + NULL);\n> > > \n> > > It would be nice if the gstreamer element could use the same line\n> > > wrapping rules as the rest of the code.\n> > > \n> > > > + gst_caps_append_structure(caps, s);\n> > > > + }\n> > > > + }\n> > > > +\n> > > > + return caps;\n> > > > +}\n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> > > > new file mode 100644\n> > > > index 0000000..4545512\n> > > > --- /dev/null\n> > > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > > @@ -0,0 +1,18 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2020, Collabora Ltd.\n> > > > + * Author: Nicolas Dufresne \n> > > > + *\n> > > > + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions\n> > > > + */\n> > > > +\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > > +\n> > > \n> > > These can go after the header guard too. This comment applies to the\n> > > rest of the series.\n> > > \n> > > > +#ifndef __GST_LIBCAMERA_UTILS_H_\n> > > \n> > > Missing\n> > > \n> > > #define __GST_LIBCAMERA_UTILS_H_\n> > > \n> > > The rest of the code base has two _ at the end of the header guards, how\n> > > about doing the same for all patches in this series ?\n> > > \n> > > > +\n> > > > +GstCaps *gst_libcamera_stream_formats_to_caps(const\n> > > libcamera::StreamFormats &formats);\n> > > > +\n> > > > +#endif /* __GST_LIBCAMERA_UTILS_H_ */\n> > > > diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> > > > index 32d4233..39a34e7 100644\n> > > > --- a/src/gstreamer/meson.build\n> > > > +++ b/src/gstreamer/meson.build\n> > > > @@ -1,5 +1,6 @@\n> > > > libcamera_gst_sources = [\n> > > > 'gstlibcamera.c',\n> > > > + 'gstlibcamera-utils.cpp',\n> > > \n> > > Nitpicking, - comes before ., so this line should go first.\n> > > \n> > > > 'gstlibcamerasrc.cpp',\n> > > > ]","headers":{"Return-Path":"","Received":["from bhuna.collabora.co.uk (bhuna.collabora.co.uk\n\t[IPv6:2a00:1098:0:82:1000:25:2eeb:e3e3])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4FB7160F3C\n\tfor ;\n\tTue, 11 Feb 2020 17:52:58 +0100 (CET)","from nicolas-tpx395.localdomain (unknown [IPv6:2610:98:8005::527])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits))\n\t(No client certificate requested) (Authenticated sender: nicolas)\n\tby bhuna.collabora.co.uk (Postfix) with ESMTPSA id A682629438E;\n\tTue, 11 Feb 2020 16:52:57 +0000 (GMT)"],"Message-ID":"<8a5a1bb941b86bbaf49ab9c6b101247ebdf9d1df.camel@collabora.com>","From":"Nicolas Dufresne ","To":"Laurent Pinchart ","Cc":"LibCamera Devel ","Date":"Tue, 11 Feb 2020 11:52:48 -0500","In-Reply-To":"<20200211163410.GI22612@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-3-nicolas@ndufresne.ca>\n\t<20200210232829.GC21893@pendragon.ideasonboard.com>\n\t\n\t<20200211163410.GI22612@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","User-Agent":"Evolution 3.34.3 (3.34.3-1.fc31) ","MIME-Version":"1.0","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Feb 2020 16:52:58 -0000"}},{"id":3658,"web_url":"https://patchwork.libcamera.org/comment/3658/","msgid":"<20200211175702.GM22612@pendragon.ideasonboard.com>","date":"2020-02-11T17:57:02","subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Nicolas,\n\nOn Tue, Feb 11, 2020 at 11:52:48AM -0500, Nicolas Dufresne wrote:\n> On mar, 2020-02-11 at 18:34 +0200, Laurent Pinchart wrote:\n> > On Mon, Feb 10, 2020 at 06:54:32PM -0500, Nicolas Dufresne wrote:\n> >> Le lun. 10 févr. 2020 18 h 28, Laurent Pinchart a écrit :\n> >>> On Tue, Jan 28, 2020 at 10:31:49PM -0500, Nicolas Dufresne wrote:\n> >>>> From: Nicolas Dufresne \n> >>>> \n> >>>> This transform the basic information found in StreamFormats to GstCaps.\n> >>> \n> >>> s/transform/transformts/\n> > \n> > Without the last t of course :-)\n> > \n> >>>> This can be handy to reply to early caps query or inside a device\n> >>>> provider. Note that we ignored generated range as they are harmful to\n> >>>> caps negotiation. We also don't simplify the caps for readability\n> >>>> reasons, so some of the discrete value may be included in a range.\n> >>>> \n> >>>> Signed-off-by: Nicolas Dufresne \n> >>>> ---\n> >>>> src/gstreamer/gstlibcamera-utils.cpp | 102 +++++++++++++++++++++++++++\n> >>>> src/gstreamer/gstlibcamera-utils.h | 18 +++++\n> >>>> src/gstreamer/meson.build | 1 +\n> >>>> 3 files changed, 121 insertions(+)\n> >>>> create mode 100644 src/gstreamer/gstlibcamera-utils.cpp\n> >>>> create mode 100644 src/gstreamer/gstlibcamera-utils.h\n> >>>> \n> >>>> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>> new file mode 100644\n> >>>> index 0000000..2540be0\n> >>>> --- /dev/null\n> >>>> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> >>>> @@ -0,0 +1,102 @@\n> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>> +/*\n> >>>> + * Copyright (C) 2020, Collabora Ltd.\n> >>>> + * Author: Nicolas Dufresne \n> >>>> + *\n> >>>> + * gstlibcamera-utils.c - GStreamer Libcamera Utility Function\n> >>>> + */\n> >>>> +\n> >>>> +#include \"gstlibcamera-utils.h\"\n> >>> \n> >>> Could you add a blank line here, as per the coding style ?\n> >>> \n> >>>> +#include \n> >>>> +\n> >>>> +using namespace libcamera;\n> >>>> +\n> >>>> +static struct {\n> >>>> + GstVideoFormat gst_format;\n> >>>> + guint drm_fourcc;\n> >>>> +} format_map[] = {\n> >>> \n> >>> Open question, should we use camelCase instead of snake_case ? I see\n> >>> you're mixing the two, after a quick glance with snake_case used for\n> >>> C-style functions and camelCase for C++ code. I don't mind that,\n> >>> especially given that the GStreamer API is snake_case. Should we add a\n> >>> README.md to the directory that documents this exception to the coding\n> >>> style ?\n> >> \n> >> I've use camel only for c++ object method, everything else follow GST style\n> >> for naming, types are Camel func and var lower case snake. When note, I\n> >> made typo, that happens. It depends if you intend to keep the plugin\n> >> forever or want in in GST long term.\n> > \n> > I think it makes sense to plan for merging it in gstreamer, so a custom\n> > coding style is fine by me, as long as we document it clearly.\n> > \n> >>>> + { GST_VIDEO_FORMAT_ENCODED, DRM_FORMAT_MJPEG },\n> >>>> + { GST_VIDEO_FORMAT_RGB, DRM_FORMAT_BGR888 },\n> >>>> + { GST_VIDEO_FORMAT_BGR, DRM_FORMAT_RGB888 },\n> >>>> + { GST_VIDEO_FORMAT_ARGB, DRM_FORMAT_BGRA8888 },\n> >>>> + { GST_VIDEO_FORMAT_NV12, DRM_FORMAT_NV12 },\n> >>>> + { GST_VIDEO_FORMAT_NV21, DRM_FORMAT_NV21 },\n> >>>> + { GST_VIDEO_FORMAT_NV16, DRM_FORMAT_NV16 },\n> >>>> + { GST_VIDEO_FORMAT_NV61, DRM_FORMAT_NV61 },\n> >>>> + { GST_VIDEO_FORMAT_NV24, DRM_FORMAT_NV24 },\n> >>>> + /* { GST_VIDEO_FORMAT_NV42, DRM_FORMAT_NV42 }, */\n> >>> \n> >>> Can we try to avoid commented-out code ? Is this because\n> >>> GST_VIDEO_FORMAT_NV42 is missing ? Are there plans to add it ?\n> >> \n> >> I can add it, that's usually what I do when. I'm bored and need something\n> >> easy.\n> >> \n> >>>> + { GST_VIDEO_FORMAT_UYVY, DRM_FORMAT_UYVY },\n> >>>> + { GST_VIDEO_FORMAT_VYUY, DRM_FORMAT_VYUY },\n> >>>> + { GST_VIDEO_FORMAT_YUY2, DRM_FORMAT_YUYV },\n> >>>> + { GST_VIDEO_FORMAT_YVYU, DRM_FORMAT_YVYU },\n> >>>> +};\n> >>>> +#define FORMAT_MAP_SIZE G_N_ELEMENTS(format_map)\n> >>>> +\n> >>>> +static inline GstVideoFormat\n> >>> \n> >>> The compiler should be smart enough to inline functions where needed,\n> >>> you don't have to instruct it to do so specifically.\n> >> \n> >> Ok.\n> >> \n> >>>> +drm_to_gst_format(guint drm_fourcc)\n> >>>> +{\n> >>>> + for (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> >>>> + if (format_map[i].drm_fourcc == drm_fourcc)\n> >>>> + return format_map[i].gst_format;\n> >>> \n> >>> If you want to make this more C++, you can define format_map as an\n> >>> std::array, drop FORMAT_MAP_SIZE, and write\n> >>> \n> >>> for (const auto &format : format_map) {\n> >>> if (format.drm_fourcc == drm_fourcc)\n> >>> return format.gst_format;\n> >>> }\n> >>> \n> >>>> + return GST_VIDEO_FORMAT_UNKNOWN;\n> >>>> +}\n> >>>> +\n> >>>> +static GstStructure *\n> >>>> +bare_structure_from_fourcc(guint fourcc)\n> >>>> +{\n> >>>> + GstVideoFormat gst_format = drm_to_gst_format(fourcc);\n> >>>> +\n> >>>> + if (gst_format == GST_VIDEO_FORMAT_UNKNOWN)\n> >>>> + return NULL;\n> >>> \n> >>> C++ uses nullptr instead of NULL. This comment applies to the whole\n> >>> series.\n> >> \n> >> I know, annoying to switch habit. All the remaining are the one I forgot.\n> >> \n> >>>> +\n> >>>> + if (gst_format != GST_VIDEO_FORMAT_ENCODED)\n> >>>> + return gst_structure_new(\"video/x-raw\", \"format\", G_TYPE_STRING,\n> >>>> +\n> >>> gst_video_format_to_string(gst_format), NULL);\n> >>>> +\n> >>>> + switch (fourcc) {\n> >>>> + case DRM_FORMAT_MJPEG:\n> >>>> + return gst_structure_new_empty(\"image/jpeg\");\n> >>>> + default:\n> >>>> + break;\n> >>> \n> >>> Maybe\n> >>> \n> >>> return nullptr;\n> >>> \n> >>> here and dropping the return at the end of the function ?\n> >>> \n> >> \n> >> I guess as I'm not using single return that could be better.\n> >> \n> >>>> + }\n> >>>> +\n> >>>> + return NULL;\n> >>>> +}\n> >>>> +\n> >>>> +GstCaps *\n> >>>> +gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> >>>> +{\n> >>>> + GstCaps *caps = gst_caps_new_empty();\n> >>>> +\n> >>>> + for (unsigned int fourcc : formats.pixelformats()) {\n> >>>> + g_autoptr(GstStructure) bare_s =\n> >>> bare_structure_from_fourcc(fourcc);\n> >>> \n> >>> g_autoptr is a really bad name when used in C++ code and the auto\n> >>> keyword :-( Nothing we can do about it though, but I think you should\n> >>> use std::unique_ptr<> instead of g_autoptr. This is C++ code, and we use\n> >>> unique_ptr extensively to convey ownership information.\n> >> \n> >> Do you have an example for that using a third party refcount system? If we\n> >> don't use GStreamer / Glib recounts, we will break many assumption, and\n> >> cause hard to catch errors. Each type have its own cleanup function, we\n> >> know how to expend to that, but macros are needed anyway. I think your\n> >> request is a lot of work, we need multiple different types of wrapper\n> >> class, since this is not all about GObject (GstStructure is a red counted\n> >> boxed type).\n> > \n> > If I understand glib right (which is far from a given :-)), this would\n> > become\n> > \n> > static std::unique_ptr\n> \n> That is very slippery to call private macros. Private macros are not part of the\n> API, if you use them, you are on your own. I'm really not convince by this.\n\nPlease note that the review comments should be treated as friendly\nsuggestions on how to improve the code :-) As with any suggestions, they\ncan sometimes be completely incorrect, or just not mandatory. As\ndiscussed separately on IRC, I think this case falls in the incorrect\ncategory given that switching to unique_ptr wouldn't remove the need for\ngst_structure_copy() as I initially thought.\n\n> > bare_structure_from_fourcc(guint fourcc)\n> > {\n> > \t...\n> > \treturn std::unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n> > }\n> > \n> > and here\n> > \n> > \tauto bare_s = bare_structure_from_fourcc(fourcc);\n> > \n> > You could simplify this with\n> > \n> > template\n> > using gst_unique_ptr = std::unique_ptr;\n> > \n> > static gst_unique_ptr bare_structure_from_fourcc(guint fourcc)\n> > {\n> > \t...\n> > \treturn gst_unique_ptr(foo, &_GLIB_AUTOPTR_FUNC_NAME(GstStructure));\n> > }\n> > \n> > ...\n> > \n> > \tgst_unique_ptr bare_s = bare_structure_from_fourcc(fourcc);\n> > \n> > (not test-compiled though)\n> > \n> >>>> +\n> >>>> + if (!bare_s) {\n> >>>> + GST_WARNING(\"Unsupported DRM format %\"\n> >>> GST_FOURCC_FORMAT,\n> >>>> + GST_FOURCC_ARGS(fourcc));\n> >>>> + continue;\n> >>>> + }\n> >>>> +\n> >>>> + for (const Size &size : formats.sizes(fourcc)) {\n> >>>> + GstStructure *s = gst_structure_copy(bare_s);\n> >>>> + gst_structure_set(s,\n> >>>> + \"width\", G_TYPE_INT, size.width,\n> >>>> + \"height\", G_TYPE_INT,\n> >>> size.height,\n> >>>> + NULL);\n> >>>> + gst_caps_append_structure(caps, s);\n> >>> \n> >>> Unless I'm mistaken, with unique_ptr, you could use bare_s.get() to get\n> >>> the raw pointer, and add bare_s.release() here to avoid the automatic\n> >>> delete. This would avoid the need to call gst_structure_copy().\n> >>> \n> >>> And now that I've written this, I realise that unique_ptr will use\n> >>> std::default_delete by default, while you need a specific cleanup\n> >>> function. You could still use a unique_ptr with a custom deleter, but it\n> >>> may not be worth it. Shame g_autptr doesn't support the equivalent of\n> >>> std::unique_ptr::release() :-(\n> >>> \n> >>>> + }\n> >>>> +\n> >>>> + const SizeRange &range = formats.range(fourcc);\n> >>>> + if (range.hStep && range.vStep) {\n> >>>> + GstStructure *s = gst_structure_copy(bare_s);\n> >>>> +\n> >>>> + gst_structure_set(s,\n> >>>> + \"width\", GST_TYPE_INT_RANGE,\n> >>> range.min.width, range.max.width, range.hStep,\n> >>>> + \"height\", GST_TYPE_INT_RANGE,\n> >>> range.min.height, range.max.height, range.vStep,\n> >>>> + NULL);\n> >>> \n> >>> It would be nice if the gstreamer element could use the same line\n> >>> wrapping rules as the rest of the code.\n> >>> \n> >>>> + gst_caps_append_structure(caps, s);\n> >>>> + }\n> >>>> + }\n> >>>> +\n> >>>> + return caps;\n> >>>> +}\n> >>>> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> >>>> new file mode 100644\n> >>>> index 0000000..4545512\n> >>>> --- /dev/null\n> >>>> +++ b/src/gstreamer/gstlibcamera-utils.h\n> >>>> @@ -0,0 +1,18 @@\n> >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> >>>> +/*\n> >>>> + * Copyright (C) 2020, Collabora Ltd.\n> >>>> + * Author: Nicolas Dufresne \n> >>>> + *\n> >>>> + * gstlibcamera-utils.h - GStreamer Libcamera Utility Functions\n> >>>> + */\n> >>>> +\n> >>>> +#include \n> >>>> +#include \n> >>>> +\n> >>>> +#include \n> >>>> +\n> >>> \n> >>> These can go after the header guard too. This comment applies to the\n> >>> rest of the series.\n> >>> \n> >>>> +#ifndef __GST_LIBCAMERA_UTILS_H_\n> >>> \n> >>> Missing\n> >>> \n> >>> #define __GST_LIBCAMERA_UTILS_H_\n> >>> \n> >>> The rest of the code base has two _ at the end of the header guards, how\n> >>> about doing the same for all patches in this series ?\n> >>> \n> >>>> +\n> >>>> +GstCaps *gst_libcamera_stream_formats_to_caps(const\n> >>> libcamera::StreamFormats &formats);\n> >>>> +\n> >>>> +#endif /* __GST_LIBCAMERA_UTILS_H_ */\n> >>>> diff --git a/src/gstreamer/meson.build b/src/gstreamer/meson.build\n> >>>> index 32d4233..39a34e7 100644\n> >>>> --- a/src/gstreamer/meson.build\n> >>>> +++ b/src/gstreamer/meson.build\n> >>>> @@ -1,5 +1,6 @@\n> >>>> libcamera_gst_sources = [\n> >>>> 'gstlibcamera.c',\n> >>>> + 'gstlibcamera-utils.cpp',\n> >>> \n> >>> Nitpicking, - comes before ., so this line should go first.\n> >>> \n> >>>> 'gstlibcamerasrc.cpp',\n> >>>> ]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3248460F3C\n\tfor ;\n\tTue, 11 Feb 2020 18:57:19 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0BAC99DA;\n\tTue, 11 Feb 2020 18:57:17 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581443838;\n\tbh=YwxF5p5iIki+5HKtEL+53J9vVpftIXkoZSWF4G0kVkI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LAP98vXHrFzdzSkj95v7BCN6d0NlpWw3+pKqYo1NVMFoIvUZhm+eWTA3BUcEx+6l2\n\tcNcqKID/oIPktAUlvJBhUXpbIDt2E3uKRmxpPUrNcKQ1SWrWZwFJT/FDX1HlJ0TcOE\n\tGFZr7rHz/2yIYfVkHGtRPPHG18nlt4Hq+UMemdpU=","Date":"Tue, 11 Feb 2020 19:57:02 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"LibCamera Devel ","Message-ID":"<20200211175702.GM22612@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-3-nicolas@ndufresne.ca>\n\t<20200210232829.GC21893@pendragon.ideasonboard.com>\n\t\n\t<20200211163410.GI22612@pendragon.ideasonboard.com>\n\t<8a5a1bb941b86bbaf49ab9c6b101247ebdf9d1df.camel@collabora.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8a5a1bb941b86bbaf49ab9c6b101247ebdf9d1df.camel@collabora.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 02/23] gst: Add utility to convert\n\tStreamFormats to GstCaps","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 11 Feb 2020 17:57:19 -0000"}}]