[{"id":3680,"web_url":"https://patchwork.libcamera.org/comment/3680/","msgid":"<20200211233438.GN20823@pendragon.ideasonboard.com>","date":"2020-02-11T23:34:38","subject":"Re: [libcamera-devel] [PATCH v1 14/23] gst: utils: Add\n\tStreamConfiguration helpers","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:32:01PM -0500, Nicolas Dufresne wrote:\n> From: Nicolas Dufresne \n> \n> This add helpers to deal with the conversion from StreamConfiguration\n> to caps and vis-versa. This is needed to implement caps negotiation.\n> \n> Signed-off-by: Nicolas Dufresne \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 61 ++++++++++++++++++++++++++++\n> src/gstreamer/gstlibcamera-utils.h | 3 ++\n> 2 files changed, 64 insertions(+)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 2540be0..d9b217a 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -41,6 +41,18 @@ drm_to_gst_format(guint drm_fourcc)\n> \treturn GST_VIDEO_FORMAT_UNKNOWN;\n> }\n> \n> +static inline guint\n\nHere too I think you can let the compiler decide whether to inline this\nfunction or not.\n\n> +gst_format_to_drm(GstVideoFormat gst_format)\n> +{\n> +\tif (gst_format == GST_VIDEO_FORMAT_ENCODED)\n> +\t\treturn DRM_FORMAT_INVALID;\n> +\n> +\tfor (guint i = 0; i < FORMAT_MAP_SIZE; i++)\n> +\t\tif (format_map[i].gst_format == gst_format)\n> +\t\t\treturn format_map[i].drm_fourcc;\n\nAs commented on before for drm_to_gst_format(), you could use a\nrange-based for loop here too.\n\n> +\treturn DRM_FORMAT_INVALID;\n> +}\n> +\n> static GstStructure *\n> bare_structure_from_fourcc(guint fourcc)\n> {\n> @@ -100,3 +112,52 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n> \n> \treturn caps;\n> }\n> +\n> +GstCaps *\n> +gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)\n> +{\n> +\tGstCaps *caps = gst_caps_new_empty();\n> +\tGstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat);\n> +\n> +\tgst_structure_set(s,\n> +\t\t\t \"width\", G_TYPE_INT, stream_cfg.size.width,\n> +\t\t\t \"height\", G_TYPE_INT, stream_cfg.size.height,\n> +\t\t\t NULL);\n> +\tgst_caps_append_structure(caps, s);\n> +\n> +\treturn caps;\n> +}\n> +\n> +void\n> +gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> +\t\t\t\t\t GstCaps *caps)\n> +{\n> +\tGstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat);\n> +\n> +\t/* First fixate the caps using default configuration value */\n> +\tg_assert(gst_caps_is_writable(caps));\n> +\tcaps = gst_caps_truncate(caps);\n> +\tGstStructure *s = gst_caps_get_structure(caps, 0);\n> +\n> +\tgst_structure_fixate_field_nearest_int(s, \"width\", stream_cfg.size.width);\n> +\tgst_structure_fixate_field_nearest_int(s, \"height\", stream_cfg.size.height);\n> +\n> +\tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> +\t\tconst gchar *format = gst_video_format_to_string(gst_format);\n> +\t\tgst_structure_fixate_field_string(s, \"format\", format);\n> +\t}\n> +\n> +\t/* The configure the stream with the result. */\n\nDid you mean s/The/Then/ ?\n\n> +\tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> +\t\tconst gchar *format = gst_structure_get_string(s, \"format\");\n> +\t\tgst_format = gst_video_format_from_string(format);\n> +\t\tstream_cfg.pixelFormat = gst_format_to_drm(gst_format);\n> +\t} else if (gst_structure_has_name(s, \"image/jpeg\")) {\n> +\t\tstream_cfg.pixelFormat = DRM_FORMAT_MJPEG;\n> +\t} else {\n> +\t\tg_critical(\"Unsupported media type: %s\", gst_structure_get_name(s));\n> +\t}\n> +\n> +\tgst_structure_get_int(s, \"width\", (gint *)&stream_cfg.size.width);\n> +\tgst_structure_get_int(s, \"height\", (gint *)&stream_cfg.size.height);\n\nCould you use static_cast instead of C-style casts ? It will\ncause a compilation error if the types are not convertible, avoiding\nproblems if we later change the type of Size::width and Size::height.\n\nNow that I wrote this, I realize it will cause a compilation failure\nalready as width and height are unsigned int. Switching to\ngst_structure_get_uint() won't work unless I'm mistaken as the width and\nheight fields in the GstStructure are signed. I think the safest option\nis to use local variables.\n\n> +}\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index 528dc2c..91cfdcd 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -16,5 +16,8 @@\n> #define GST_OBJECT_LOCKER(obj) g_autoptr(GMutexLocker) locker = g_mutex_locker_new(GST_OBJECT_GET_LOCK(obj))\n> \n> GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);\n> +GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);\n> +void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,\n> +\t\t\t\t\t GstCaps *caps);\n> \n> #endif /* __GST_LIBCAMERA_UTILS_H_ */","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 5C9C460990\n\tfor ;\n\tWed, 12 Feb 2020 00:34:54 +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 6999F9DA;\n\tWed, 12 Feb 2020 00:34:53 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581464094;\n\tbh=6LaFkU3xdKrHJCVJ65RO/Sa9gMPDdRxXjojhixQbw6M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tm99CJ5tp+eu02NTqK1LwdL27Jds9NGfh5pA/HriK0A55p7Cwr6epHcXXeRzY+1uw\n\tEjgrBkcHdBoOL9zDnRVbhaDTwWc89mrOYK+GgpSByKLU9hpCB1hIvjXPZAcpswgTO/\n\tsXaNTiPHWM9CALvN8zyDzWkdX7Lp6uUllScyBq38=","Date":"Wed, 12 Feb 2020 01:34:38 +0200","From":"Laurent Pinchart ","To":"Nicolas Dufresne ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNicolas Dufresne ","Message-ID":"<20200211233438.GN20823@pendragon.ideasonboard.com>","References":"<20200129033210.278800-1-nicolas@ndufresne.ca>\n\t<20200129033210.278800-15-nicolas@ndufresne.ca>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200129033210.278800-15-nicolas@ndufresne.ca>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v1 14/23] gst: utils: Add\n\tStreamConfiguration helpers","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 23:34:54 -0000"}}]