[libcamera-devel] [PATCH v1 14/23] gst: utils: Add StreamConfiguration helpers

[Laurent Pinchart]

Hi Nicolas,

Thank you for the patch.

On Tue, Jan 28, 2020 at 10:32:01PM -0500, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> 
> This add helpers to deal with the conversion from StreamConfiguration
> to caps and vis-versa. This is needed to implement caps negotiation.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 61 ++++++++++++++++++++++++++++
>  src/gstreamer/gstlibcamera-utils.h   |  3 ++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 2540be0..d9b217a 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -41,6 +41,18 @@ drm_to_gst_format(guint drm_fourcc)
>  	return GST_VIDEO_FORMAT_UNKNOWN;
>  }
>  
> +static inline guint

Here too I think you can let the compiler decide whether to inline this
function or not.

> +gst_format_to_drm(GstVideoFormat gst_format)
> +{
> +	if (gst_format == GST_VIDEO_FORMAT_ENCODED)
> +		return DRM_FORMAT_INVALID;
> +
> +	for (guint i = 0; i < FORMAT_MAP_SIZE; i++)
> +		if (format_map[i].gst_format == gst_format)
> +			return format_map[i].drm_fourcc;

As commented on before for drm_to_gst_format(), you could use a
range-based for loop here too.

> +	return DRM_FORMAT_INVALID;
> +}
> +
>  static GstStructure *
>  bare_structure_from_fourcc(guint fourcc)
>  {
> @@ -100,3 +112,52 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  
>  	return caps;
>  }
> +
> +GstCaps *
> +gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)
> +{
> +	GstCaps *caps = gst_caps_new_empty();
> +	GstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat);
> +
> +	gst_structure_set(s,
> +			  "width", G_TYPE_INT, stream_cfg.size.width,
> +			  "height", G_TYPE_INT, stream_cfg.size.height,
> +			  NULL);
> +	gst_caps_append_structure(caps, s);
> +
> +	return caps;
> +}
> +
> +void
> +gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> +					 GstCaps *caps)
> +{
> +	GstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat);
> +
> +	/* First fixate the caps using default configuration value */
> +	g_assert(gst_caps_is_writable(caps));
> +	caps = gst_caps_truncate(caps);
> +	GstStructure *s = gst_caps_get_structure(caps, 0);
> +
> +	gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
> +	gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
> +
> +	if (gst_structure_has_name(s, "video/x-raw")) {
> +		const gchar *format = gst_video_format_to_string(gst_format);
> +		gst_structure_fixate_field_string(s, "format", format);
> +	}
> +
> +	/* The configure the stream with the result. */

Did you mean s/The/Then/ ?

> +	if (gst_structure_has_name(s, "video/x-raw")) {
> +		const gchar *format = gst_structure_get_string(s, "format");
> +		gst_format = gst_video_format_from_string(format);
> +		stream_cfg.pixelFormat = gst_format_to_drm(gst_format);
> +	} else if (gst_structure_has_name(s, "image/jpeg")) {
> +		stream_cfg.pixelFormat = DRM_FORMAT_MJPEG;
> +	} else {
> +		g_critical("Unsupported media type: %s", gst_structure_get_name(s));
> +	}
> +
> +	gst_structure_get_int(s, "width", (gint *)&stream_cfg.size.width);
> +	gst_structure_get_int(s, "height", (gint *)&stream_cfg.size.height);

Could you use static_cast<gint *> instead of C-style casts ? It will
cause a compilation error if the types are not convertible, avoiding
problems if we later change the type of Size::width and Size::height.

Now that I wrote this, I realize it will cause a compilation failure
already as width and height are unsigned int. Switching to
gst_structure_get_uint() won't work unless I'm mistaken as the width and
height fields in the GstStructure are signed. I think the safest option
is to use local variables.

> +}
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 528dc2c..91cfdcd 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -16,5 +16,8 @@
>  #define GST_OBJECT_LOCKER(obj) g_autoptr(GMutexLocker) locker = g_mutex_locker_new(GST_OBJECT_GET_LOCK(obj))
>  
>  GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats);
> +GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg);
> +void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg,
> +					      GstCaps *caps);
>  
>  #endif /* __GST_LIBCAMERA_UTILS_H_ */

-- 
Regards,

Laurent Pinchart