[libcamera-devel,v2] gstreamer: Fix usage of default size for fixation
diff mbox series

Message ID 20210826141602.1243989-1-nicolas@ndufresne.ca
State Accepted
Commit be05f8d1dac23634ce38ba16a703d561e379788d
Headers show
Series
  • [libcamera-devel,v2] gstreamer: Fix usage of default size for fixation
Related show

Commit Message

Nicolas Dufresne Aug. 26, 2021, 2:16 p.m. UTC 
From: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Pipeline managers sets a default value to StreamConfiguration::size. The
original fixation code was attempting to use it, but as it was truncating
the caps to its first structure it would never actually find a best match.

In this patch, instead of truncating, we weight various matches using the
product of the width and height delta. We also split delta from ranges
apart and prefer fixed size over them as ranges are not reliable.

This patch also removes the related todo, as it seems that libcamera core
won't go further then providing this default value and won't be sorting the
format and size lists.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---
 src/gstreamer/gstlibcamerasrc.cpp    |  4 ---
 2 files changed, 47 insertions(+), 8 deletions(-)

Comments

Kieran Bingham Aug. 26, 2021, 2:33 p.m. UTC | #1 
On 26/08/2021 15:16, Nicolas Dufresne wrote:
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> Pipeline managers sets a default value to StreamConfiguration::size. 

If you're happy, I'd change this to

"Pipeline handlers provide a default StreamConfiguration when given a
StreamRole."

Eitherway, we can merge this now I think.
--
Kieran

> The
> original fixation code was attempting to use it, but as it was truncating
> the caps to its first structure it would never actually find a best match.
> 
> In this patch, instead of truncating, we weight various matches using the
> product of the width and height delta. We also split delta from ranges
> apart and prefer fixed size over them as ranges are not reliable.
> 
> This patch also removes the related todo, as it seems that libcamera core
> won't go further then providing this default value and won't be sorting the
> format and size lists.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---
>  src/gstreamer/gstlibcamerasrc.cpp    |  4 ---
>  2 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 61370d5f..723c4fa3 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>  					 GstCaps *caps)
>  {
>  	GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> +	guint i;
> +	gint best_fixed = -1, best_in_range = -1;
> +	GstStructure *s;
> +
> +	/*
> +	 * These are delta weight computed from:
> +	 *   ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)
> +	 */
> +	guint best_fixed_delta = G_MAXUINT;
> +	guint best_in_range_delta = G_MAXUINT;
>  
>  	/* 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);
> +	/* Lookup the structure for a close match to the stream_cfg.size */
> +	for (i = 0; i < gst_caps_get_size(caps); i++) {
> +		s = gst_caps_get_structure(caps, i);
> +		gint width, height;
> +		guint delta;
> +
> +		if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) &&
> +		    gst_structure_has_field_typed(s, "height", G_TYPE_INT)) {
> +			gst_structure_get_int(s, "width", &width);
> +			gst_structure_get_int(s, "height", &height);
> +
> +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> +
> +			if (delta < best_fixed_delta) {
> +				best_fixed_delta = delta;
> +				best_fixed = i;
> +			}
> +		} else {
> +			gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
> +			gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
> +			gst_structure_get_int(s, "width", &width);
> +			gst_structure_get_int(s, "height", &height);
> +
> +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> +
> +			if (delta < best_in_range_delta) {
> +				best_in_range_delta = delta;
> +				best_in_range = i;
> +			}
> +		}
> +	}
> +
> +	/* Prefer reliable fixed value over ranges */
> +	if (best_fixed >= 0)
> +		s = gst_caps_get_structure(caps, best_fixed);
> +	else
> +		s = gst_caps_get_structure(caps, best_in_range);
>  
>  	if (gst_structure_has_name(s, "video/x-raw")) {
>  		const gchar *format = gst_video_format_to_string(gst_format);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index b243aeb3..4c7b36ae 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -25,10 +25,6 @@
>   *  - Add timestamp support
>   *  - Use unique names to select the camera devices
>   *  - Add GstVideoMeta support (strides and offsets)
> - *
> - * \todo libcamera UVC drivers picks the lowest possible resolution first, this
> - * should be fixed so that we get a decent resolution and framerate for the
> - * role by default.
>   */
>  
>  #include "gstlibcamerasrc.h"
>
Nicolas Dufresne Aug. 26, 2021, 2:35 p.m. UTC | #2 
Le jeudi 26 août 2021 à 15:33 +0100, Kieran Bingham a écrit :
> On 26/08/2021 15:16, Nicolas Dufresne wrote:
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > Pipeline managers sets a default value to StreamConfiguration::size. 
> 
> If you're happy, I'd change this to
> 
> "Pipeline handlers provide a default StreamConfiguration when given a
> StreamRole."

That is fine with me. 

> 
> Eitherway, we can merge this now I think.
> --
> Kieran
> 
> > The
> > original fixation code was attempting to use it, but as it was truncating
> > the caps to its first structure it would never actually find a best match.
> > 
> > In this patch, instead of truncating, we weight various matches using the
> > product of the width and height delta. We also split delta from ranges
> > apart and prefer fixed size over them as ranges are not reliable.
> > 
> > This patch also removes the related todo, as it seems that libcamera core
> > won't go further then providing this default value and won't be sorting the
> > format and size lists.
> > 
> > Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---
> >  src/gstreamer/gstlibcamerasrc.cpp    |  4 ---
> >  2 files changed, 47 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index 61370d5f..723c4fa3 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> >  					 GstCaps *caps)
> >  {
> >  	GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> > +	guint i;
> > +	gint best_fixed = -1, best_in_range = -1;
> > +	GstStructure *s;
> > +
> > +	/*
> > +	 * These are delta weight computed from:
> > +	 *   ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)
> > +	 */
> > +	guint best_fixed_delta = G_MAXUINT;
> > +	guint best_in_range_delta = G_MAXUINT;
> >  
> >  	/* 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);
> > +	/* Lookup the structure for a close match to the stream_cfg.size */
> > +	for (i = 0; i < gst_caps_get_size(caps); i++) {
> > +		s = gst_caps_get_structure(caps, i);
> > +		gint width, height;
> > +		guint delta;
> > +
> > +		if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) &&
> > +		    gst_structure_has_field_typed(s, "height", G_TYPE_INT)) {
> > +			gst_structure_get_int(s, "width", &width);
> > +			gst_structure_get_int(s, "height", &height);
> > +
> > +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> > +
> > +			if (delta < best_fixed_delta) {
> > +				best_fixed_delta = delta;
> > +				best_fixed = i;
> > +			}
> > +		} else {
> > +			gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
> > +			gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
> > +			gst_structure_get_int(s, "width", &width);
> > +			gst_structure_get_int(s, "height", &height);
> > +
> > +			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> > +
> > +			if (delta < best_in_range_delta) {
> > +				best_in_range_delta = delta;
> > +				best_in_range = i;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* Prefer reliable fixed value over ranges */
> > +	if (best_fixed >= 0)
> > +		s = gst_caps_get_structure(caps, best_fixed);
> > +	else
> > +		s = gst_caps_get_structure(caps, best_in_range);
> >  
> >  	if (gst_structure_has_name(s, "video/x-raw")) {
> >  		const gchar *format = gst_video_format_to_string(gst_format);
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index b243aeb3..4c7b36ae 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -25,10 +25,6 @@
> >   *  - Add timestamp support
> >   *  - Use unique names to select the camera devices
> >   *  - Add GstVideoMeta support (strides and offsets)
> > - *
> > - * \todo libcamera UVC drivers picks the lowest possible resolution first, this
> > - * should be fixed so that we get a decent resolution and framerate for the
> > - * role by default.
> >   */
> >  
> >  #include "gstlibcamerasrc.h"
> >

Patch
diff mbox series 

diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
index 61370d5f..723c4fa3 100644
--- a/src/gstreamer/gstlibcamera-utils.cpp
+++ b/src/gstreamer/gstlibcamera-utils.cpp
@@ -136,14 +136,57 @@  gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
 					 GstCaps *caps)
 {
 	GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
+	guint i;
+	gint best_fixed = -1, best_in_range = -1;
+	GstStructure *s;
+
+	/*
+	 * These are delta weight computed from:
+	 *   ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)
+	 */
+	guint best_fixed_delta = G_MAXUINT;
+	guint best_in_range_delta = G_MAXUINT;
 
 	/* 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);
+	/* Lookup the structure for a close match to the stream_cfg.size */
+	for (i = 0; i < gst_caps_get_size(caps); i++) {
+		s = gst_caps_get_structure(caps, i);
+		gint width, height;
+		guint delta;
+
+		if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) &&
+		    gst_structure_has_field_typed(s, "height", G_TYPE_INT)) {
+			gst_structure_get_int(s, "width", &width);
+			gst_structure_get_int(s, "height", &height);
+
+			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
+
+			if (delta < best_fixed_delta) {
+				best_fixed_delta = delta;
+				best_fixed = i;
+			}
+		} else {
+			gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
+			gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
+			gst_structure_get_int(s, "width", &width);
+			gst_structure_get_int(s, "height", &height);
+
+			delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
+
+			if (delta < best_in_range_delta) {
+				best_in_range_delta = delta;
+				best_in_range = i;
+			}
+		}
+	}
+
+	/* Prefer reliable fixed value over ranges */
+	if (best_fixed >= 0)
+		s = gst_caps_get_structure(caps, best_fixed);
+	else
+		s = gst_caps_get_structure(caps, best_in_range);
 
 	if (gst_structure_has_name(s, "video/x-raw")) {
 		const gchar *format = gst_video_format_to_string(gst_format);
diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index b243aeb3..4c7b36ae 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -25,10 +25,6 @@ 
  *  - Add timestamp support
  *  - Use unique names to select the camera devices
  *  - Add GstVideoMeta support (strides and offsets)
- *
- * \todo libcamera UVC drivers picks the lowest possible resolution first, this
- * should be fixed so that we get a decent resolution and framerate for the
- * role by default.
  */
 
 #include "gstlibcamerasrc.h"