| Message ID | 20260803131616.11292-2-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Mon, Aug 03, 2026 at 02:07:47PM +0100, David Plowman wrote: > Existing code was preferring fixed size outputs over ones with ranges, > resulting in raw output formats being preferred even when applications > ("cheese" is one such example) aren't expecting them and subsequently > fail. > > The revised version still prefers fixed size outputs over ones with > ranges, but will prefer non-raw (meaning non-Bayer) formats where > these are available. This still allows applications that explicitly > request raw formats to get them, but other applications will normally > get non-raw formats which they handle better. > > Fixes, for example, "cheese" on Raspberry Pi. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> I can't really judge on the preferred gst negotiation part, but the code looks sane to me: Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- > src/gstreamer/gstlibcamera-utils.h | 2 +- > src/gstreamer/gstlibcamerasrc.cpp | 3 +- > 3 files changed, 87 insertions(+), 33 deletions(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 6541d478c..996467ef4 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -8,6 +8,7 @@ > > #include "gstlibcamera-utils.h" > > +#include <optional> > #include <string> > > #include <libcamera/control_ids.h> > @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > return caps; > } > > -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > +/* > + * We will want to distinguish between caps structures corresponding to > + * raw and non-raw (processed) output images, as this will help inform > + * our choice of preferred format. > + * > + * We identify Bayer as being the principle "raw" format here, but note > + * that we are considering greyscale (images with one component) to be > + * raw too (e.g. a raw monochrome sensor). > + */ > +static bool > +gst_libcamera_structure_is_raw_capture(const GstStructure *s) > +{ > + if (gst_structure_has_name(s, "video/x-bayer")) > + return true; > + > + if (gst_structure_has_name(s, "video/x-raw")) { > + const gchar *format = gst_structure_get_string(s, "format"); > + if (!format) > + return false; > + > + const GstVideoFormatInfo *finfo = > + gst_video_format_get_info(gst_video_format_from_string(format)); > + > + return finfo->n_components == 1; > + } > + > + return false; > +} > + > +/* > + * Data recorded for each caps structure candidate, used to rank them > + * against each other via operator<(). > + */ > +struct CapsCandidate { > + guint index; > + bool is_raw; > + bool is_fixed; > + guint delta; > + > + bool operator<(const CapsCandidate &other) const > + { > + /* > + * Raw formats are likely to be useful only to applications that > + * specifically ask for them. Other applications will typically be > + * unable to handle them and fail, so prefer non-raw candidates. > + */ > + if (is_raw != other.is_raw) > + return !is_raw; > + > + /* Prefer a reliable fixed value over a range. */ > + if (is_fixed != other.is_fixed) > + return is_fixed; > + > + /* Otherwise the closest size match wins. */ > + return delta < other.delta; > + } > +}; > + > +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > GstCaps *caps, GstVideoTransferFunction *transfer) > { > 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)); > > - /* Lookup the structure for a close match to the stream_cfg.size */ > + /* > + * Build a candidate for every available caps structure, and keep > + * track of the best one seen so far, as ranked by > + * CapsCandidate::operator<(). > + */ > + std::optional<CapsCandidate> best; > + > for (i = 0; i < gst_caps_get_size(caps); i++) { > s = gst_caps_get_structure(caps, i); > gint width, height; > - guint delta; > + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > + gst_structure_has_field_typed(s, "height", G_TYPE_INT); > > - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { > + if (is_fixed) { > 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); > + guint 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; > - } > - } > + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; > + > + if (!best || candidate < *best) > + best = candidate; > + } > + > + if (!best) { > + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); > + return false; > } > > - /* 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); > + s = gst_caps_get_structure(caps, best->index); > > if (gst_structure_has_name(s, "video/x-raw")) { > const gchar *format = gst_video_format_to_string(gst_format); > @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); > } > + > + return true; > } > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index 35df56fb9..5ce0e839c 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -18,7 +18,7 @@ > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, > GstVideoTransferFunction transfer); > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > GstCaps *caps, GstVideoTransferFunction *transfer); > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); > void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 9061f9163..66b31ce82 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > /* Fixate caps and configure the stream. */ > caps = gst_caps_make_writable(caps); > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); > + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) > + return false; > gst_libcamera_get_framerate_from_caps(caps, element_caps); > } > > -- > 2.47.3 >
Hi Nicolas, On Thu, Aug 06, 2026 at 10:07:17AM +0200, Jacopo Mondi wrote: > Hi David > > On Mon, Aug 03, 2026 at 02:07:47PM +0100, David Plowman wrote: > > Existing code was preferring fixed size outputs over ones with ranges, > > resulting in raw output formats being preferred even when applications > > ("cheese" is one such example) aren't expecting them and subsequently > > fail. > > > > The revised version still prefers fixed size outputs over ones with > > ranges, but will prefer non-raw (meaning non-Bayer) formats where > > these are available. This still allows applications that explicitly > > request raw formats to get them, but other applications will normally > > get non-raw formats which they handle better. > > > > Fixes, for example, "cheese" on Raspberry Pi. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > I can't really judge on the preferred gst negotiation part, but the Surely you can judge better than me the negotiation part :) Any thoughts ? Thanks j > code looks sane to me: > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- > > src/gstreamer/gstlibcamera-utils.h | 2 +- > > src/gstreamer/gstlibcamerasrc.cpp | 3 +- > > 3 files changed, 87 insertions(+), 33 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > index 6541d478c..996467ef4 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -8,6 +8,7 @@ > > > > #include "gstlibcamera-utils.h" > > > > +#include <optional> > > #include <string> > > > > #include <libcamera/control_ids.h> > > @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > > return caps; > > } > > > > -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > +/* > > + * We will want to distinguish between caps structures corresponding to > > + * raw and non-raw (processed) output images, as this will help inform > > + * our choice of preferred format. > > + * > > + * We identify Bayer as being the principle "raw" format here, but note > > + * that we are considering greyscale (images with one component) to be > > + * raw too (e.g. a raw monochrome sensor). > > + */ > > +static bool > > +gst_libcamera_structure_is_raw_capture(const GstStructure *s) > > +{ > > + if (gst_structure_has_name(s, "video/x-bayer")) > > + return true; > > + > > + if (gst_structure_has_name(s, "video/x-raw")) { > > + const gchar *format = gst_structure_get_string(s, "format"); > > + if (!format) > > + return false; > > + > > + const GstVideoFormatInfo *finfo = > > + gst_video_format_get_info(gst_video_format_from_string(format)); > > + > > + return finfo->n_components == 1; > > + } > > + > > + return false; > > +} > > + > > +/* > > + * Data recorded for each caps structure candidate, used to rank them > > + * against each other via operator<(). > > + */ > > +struct CapsCandidate { > > + guint index; > > + bool is_raw; > > + bool is_fixed; > > + guint delta; > > + > > + bool operator<(const CapsCandidate &other) const > > + { > > + /* > > + * Raw formats are likely to be useful only to applications that > > + * specifically ask for them. Other applications will typically be > > + * unable to handle them and fail, so prefer non-raw candidates. > > + */ > > + if (is_raw != other.is_raw) > > + return !is_raw; > > + > > + /* Prefer a reliable fixed value over a range. */ > > + if (is_fixed != other.is_fixed) > > + return is_fixed; > > + > > + /* Otherwise the closest size match wins. */ > > + return delta < other.delta; > > + } > > +}; > > + > > +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > GstCaps *caps, GstVideoTransferFunction *transfer) > > { > > 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)); > > > > - /* Lookup the structure for a close match to the stream_cfg.size */ > > + /* > > + * Build a candidate for every available caps structure, and keep > > + * track of the best one seen so far, as ranked by > > + * CapsCandidate::operator<(). > > + */ > > + std::optional<CapsCandidate> best; > > + > > for (i = 0; i < gst_caps_get_size(caps); i++) { > > s = gst_caps_get_structure(caps, i); > > gint width, height; > > - guint delta; > > + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > + gst_structure_has_field_typed(s, "height", G_TYPE_INT); > > > > - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { > > + if (is_fixed) { > > 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); > > + guint 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; > > - } > > - } > > + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; > > + > > + if (!best || candidate < *best) > > + best = candidate; > > + } > > + > > + if (!best) { > > + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); > > + return false; > > } > > > > - /* 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); > > + s = gst_caps_get_structure(caps, best->index); > > > > if (gst_structure_has_name(s, "video/x-raw")) { > > const gchar *format = gst_video_format_to_string(gst_format); > > @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); > > } > > + > > + return true; > > } > > > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > index 35df56fb9..5ce0e839c 100644 > > --- a/src/gstreamer/gstlibcamera-utils.h > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -18,7 +18,7 @@ > > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, > > GstVideoTransferFunction transfer); > > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > GstCaps *caps, GstVideoTransferFunction *transfer); > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); > > void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 9061f9163..66b31ce82 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > > > /* Fixate caps and configure the stream. */ > > caps = gst_caps_make_writable(caps); > > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); > > + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) > > + return false; > > gst_libcamera_get_framerate_from_caps(caps, element_caps); > > } > > > > -- > > 2.47.3 > >
Le lundi 03 août 2026 à 14:07 +0100, David Plowman a écrit : > Existing code was preferring fixed size outputs over ones with ranges, > resulting in raw output formats being preferred even when applications > ("cheese" is one such example) aren't expecting them and subsequently > fail. > > The revised version still prefers fixed size outputs over ones with > ranges, but will prefer non-raw (meaning non-Bayer) formats where > these are available. This still allows applications that explicitly > request raw formats to get them, but other applications will normally > get non-raw formats which they handle better. > > Fixes, for example, "cheese" on Raspberry Pi. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- > src/gstreamer/gstlibcamera-utils.h | 2 +- > src/gstreamer/gstlibcamerasrc.cpp | 3 +- > 3 files changed, 87 insertions(+), 33 deletions(-) > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > index 6541d478c..996467ef4 100644 > --- a/src/gstreamer/gstlibcamera-utils.cpp > +++ b/src/gstreamer/gstlibcamera-utils.cpp > @@ -8,6 +8,7 @@ > > #include "gstlibcamera-utils.h" > > +#include <optional> > #include <string> > > #include <libcamera/control_ids.h> > @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > return caps; > } > > -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > +/* > + * We will want to distinguish between caps structures corresponding to > + * raw and non-raw (processed) output images, as this will help inform > + * our choice of preferred format. > + * > + * We identify Bayer as being the principle "raw" format here, but note > + * that we are considering greyscale (images with one component) to be > + * raw too (e.g. a raw monochrome sensor). > + */ > +static bool > +gst_libcamera_structure_is_raw_capture(const GstStructure *s) > +{ > + if (gst_structure_has_name(s, "video/x-bayer")) > + return true; > + > + if (gst_structure_has_name(s, "video/x-raw")) { > + const gchar *format = gst_structure_get_string(s, "format"); > + if (!format) > + return false; > + > + const GstVideoFormatInfo *finfo = > + gst_video_format_get_info(gst_video_format_from_string(format)); > + > + return finfo->n_components == 1; I wonder if using the flag wouldn't be more accurate, see GST_VIDEO_FORMAT_FLAG_GRAY which you can read with the macro GST_VIDEO_FORMAT_INFO_IS_GRAY(finfo). Appart from this cosmetic suggestion: Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > + } > + > + return false; > +} > + > +/* > + * Data recorded for each caps structure candidate, used to rank them > + * against each other via operator<(). > + */ > +struct CapsCandidate { > + guint index; > + bool is_raw; > + bool is_fixed; > + guint delta; > + > + bool operator<(const CapsCandidate &other) const > + { > + /* > + * Raw formats are likely to be useful only to applications that > + * specifically ask for them. Other applications will typically be > + * unable to handle them and fail, so prefer non-raw candidates. > + */ > + if (is_raw != other.is_raw) > + return !is_raw; > + > + /* Prefer a reliable fixed value over a range. */ > + if (is_fixed != other.is_fixed) > + return is_fixed; > + > + /* Otherwise the closest size match wins. */ > + return delta < other.delta; > + } > +}; > + > +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > GstCaps *caps, GstVideoTransferFunction *transfer) > { > 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)); > > - /* Lookup the structure for a close match to the stream_cfg.size */ > + /* > + * Build a candidate for every available caps structure, and keep > + * track of the best one seen so far, as ranked by > + * CapsCandidate::operator<(). > + */ > + std::optional<CapsCandidate> best; > + > for (i = 0; i < gst_caps_get_size(caps); i++) { > s = gst_caps_get_structure(caps, i); > gint width, height; > - guint delta; > + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > + gst_structure_has_field_typed(s, "height", G_TYPE_INT); > > - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { > + if (is_fixed) { > 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); > + guint 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; > - } > - } > + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; > + > + if (!best || candidate < *best) > + best = candidate; > + } > + > + if (!best) { > + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); > + return false; > } > > - /* 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); > + s = gst_caps_get_structure(caps, best->index); > > if (gst_structure_has_name(s, "video/x-raw")) { > const gchar *format = gst_video_format_to_string(gst_format); > @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); > } > + > + return true; > } > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > index 35df56fb9..5ce0e839c 100644 > --- a/src/gstreamer/gstlibcamera-utils.h > +++ b/src/gstreamer/gstlibcamera-utils.h > @@ -18,7 +18,7 @@ > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, > GstVideoTransferFunction transfer); > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > GstCaps *caps, GstVideoTransferFunction *transfer); > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); > void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > index 9061f9163..66b31ce82 100644 > --- a/src/gstreamer/gstlibcamerasrc.cpp > +++ b/src/gstreamer/gstlibcamerasrc.cpp > @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > /* Fixate caps and configure the stream. */ > caps = gst_caps_make_writable(caps); > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); > + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) > + return false; > gst_libcamera_get_framerate_from_caps(caps, element_caps); > } >
Hi David On Mon, Aug 17, 2026 at 09:01:14AM -0400, Nicolas Dufresne wrote: > Le lundi 03 août 2026 à 14:07 +0100, David Plowman a écrit : > > Existing code was preferring fixed size outputs over ones with ranges, > > resulting in raw output formats being preferred even when applications > > ("cheese" is one such example) aren't expecting them and subsequently > > fail. > > > > The revised version still prefers fixed size outputs over ones with > > ranges, but will prefer non-raw (meaning non-Bayer) formats where > > these are available. This still allows applications that explicitly > > request raw formats to get them, but other applications will normally > > get non-raw formats which they handle better. > > > > Fixes, for example, "cheese" on Raspberry Pi. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- > > src/gstreamer/gstlibcamera-utils.h | 2 +- > > src/gstreamer/gstlibcamerasrc.cpp | 3 +- > > 3 files changed, 87 insertions(+), 33 deletions(-) > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > index 6541d478c..996467ef4 100644 > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > @@ -8,6 +8,7 @@ > > > > #include "gstlibcamera-utils.h" > > > > +#include <optional> > > #include <string> > > > > #include <libcamera/control_ids.h> > > @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > > return caps; > > } > > > > -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > +/* > > + * We will want to distinguish between caps structures corresponding to > > + * raw and non-raw (processed) output images, as this will help inform > > + * our choice of preferred format. > > + * > > + * We identify Bayer as being the principle "raw" format here, but note > > + * that we are considering greyscale (images with one component) to be > > + * raw too (e.g. a raw monochrome sensor). > > + */ > > +static bool > > +gst_libcamera_structure_is_raw_capture(const GstStructure *s) > > +{ > > + if (gst_structure_has_name(s, "video/x-bayer")) > > + return true; > > + > > + if (gst_structure_has_name(s, "video/x-raw")) { > > + const gchar *format = gst_structure_get_string(s, "format"); > > + if (!format) > > + return false; > > + > > + const GstVideoFormatInfo *finfo = > > + gst_video_format_get_info(gst_video_format_from_string(format)); > > + > > + return finfo->n_components == 1; > > I wonder if using the flag wouldn't be more accurate, see > GST_VIDEO_FORMAT_FLAG_GRAY which you can read with the macro > GST_VIDEO_FORMAT_INFO_IS_GRAY(finfo). > Are you planning to send a new version with the above suggestion or should we go ahead with this version ? (not asking you to re-send, but the macro looks nice :blink: :blink:) > Appart from this cosmetic suggestion: > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > + } > > + > > + return false; > > +} > > + > > +/* > > + * Data recorded for each caps structure candidate, used to rank them > > + * against each other via operator<(). > > + */ > > +struct CapsCandidate { > > + guint index; > > + bool is_raw; > > + bool is_fixed; > > + guint delta; > > + > > + bool operator<(const CapsCandidate &other) const > > + { > > + /* > > + * Raw formats are likely to be useful only to applications that > > + * specifically ask for them. Other applications will typically be > > + * unable to handle them and fail, so prefer non-raw candidates. > > + */ > > + if (is_raw != other.is_raw) > > + return !is_raw; > > + > > + /* Prefer a reliable fixed value over a range. */ > > + if (is_fixed != other.is_fixed) > > + return is_fixed; > > + > > + /* Otherwise the closest size match wins. */ > > + return delta < other.delta; > > + } > > +}; > > + > > +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > GstCaps *caps, GstVideoTransferFunction *transfer) > > { > > 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)); > > > > - /* Lookup the structure for a close match to the stream_cfg.size */ > > + /* > > + * Build a candidate for every available caps structure, and keep > > + * track of the best one seen so far, as ranked by > > + * CapsCandidate::operator<(). > > + */ > > + std::optional<CapsCandidate> best; > > + > > for (i = 0; i < gst_caps_get_size(caps); i++) { > > s = gst_caps_get_structure(caps, i); > > gint width, height; > > - guint delta; > > + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > + gst_structure_has_field_typed(s, "height", G_TYPE_INT); > > > > - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { > > + if (is_fixed) { > > 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); > > + guint 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; > > - } > > - } > > + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; > > + > > + if (!best || candidate < *best) > > + best = candidate; > > + } > > + > > + if (!best) { > > + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); > > + return false; > > } > > > > - /* 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); > > + s = gst_caps_get_structure(caps, best->index); > > > > if (gst_structure_has_name(s, "video/x-raw")) { > > const gchar *format = gst_video_format_to_string(gst_format); > > @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); > > } > > + > > + return true; > > } > > > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > index 35df56fb9..5ce0e839c 100644 > > --- a/src/gstreamer/gstlibcamera-utils.h > > +++ b/src/gstreamer/gstlibcamera-utils.h > > @@ -18,7 +18,7 @@ > > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, > > GstVideoTransferFunction transfer); > > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > GstCaps *caps, GstVideoTransferFunction *transfer); > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); > > void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > index 9061f9163..66b31ce82 100644 > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > > > /* Fixate caps and configure the stream. */ > > caps = gst_caps_make_writable(caps); > > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); > > + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) > > + return false; > > gst_libcamera_get_framerate_from_caps(caps, element_caps); > > } > >
Yes, sorry I didn't respond, but I will do that! Thanks David On Tue, 18 Aug 2026 at 11:08, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Mon, Aug 17, 2026 at 09:01:14AM -0400, Nicolas Dufresne wrote: > > Le lundi 03 août 2026 à 14:07 +0100, David Plowman a écrit : > > > Existing code was preferring fixed size outputs over ones with ranges, > > > resulting in raw output formats being preferred even when applications > > > ("cheese" is one such example) aren't expecting them and subsequently > > > fail. > > > > > > The revised version still prefers fixed size outputs over ones with > > > ranges, but will prefer non-raw (meaning non-Bayer) formats where > > > these are available. This still allows applications that explicitly > > > request raw formats to get them, but other applications will normally > > > get non-raw formats which they handle better. > > > > > > Fixes, for example, "cheese" on Raspberry Pi. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- > > > src/gstreamer/gstlibcamera-utils.h | 2 +- > > > src/gstreamer/gstlibcamerasrc.cpp | 3 +- > > > 3 files changed, 87 insertions(+), 33 deletions(-) > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp > > > index 6541d478c..996467ef4 100644 > > > --- a/src/gstreamer/gstlibcamera-utils.cpp > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp > > > @@ -8,6 +8,7 @@ > > > > > > #include "gstlibcamera-utils.h" > > > > > > +#include <optional> > > > #include <string> > > > > > > #include <libcamera/control_ids.h> > > > @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg > > > return caps; > > > } > > > > > > -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > > +/* > > > + * We will want to distinguish between caps structures corresponding to > > > + * raw and non-raw (processed) output images, as this will help inform > > > + * our choice of preferred format. > > > + * > > > + * We identify Bayer as being the principle "raw" format here, but note > > > + * that we are considering greyscale (images with one component) to be > > > + * raw too (e.g. a raw monochrome sensor). > > > + */ > > > +static bool > > > +gst_libcamera_structure_is_raw_capture(const GstStructure *s) > > > +{ > > > + if (gst_structure_has_name(s, "video/x-bayer")) > > > + return true; > > > + > > > + if (gst_structure_has_name(s, "video/x-raw")) { > > > + const gchar *format = gst_structure_get_string(s, "format"); > > > + if (!format) > > > + return false; > > > + > > > + const GstVideoFormatInfo *finfo = > > > + gst_video_format_get_info(gst_video_format_from_string(format)); > > > + > > > + return finfo->n_components == 1; > > > > I wonder if using the flag wouldn't be more accurate, see > > GST_VIDEO_FORMAT_FLAG_GRAY which you can read with the macro > > GST_VIDEO_FORMAT_INFO_IS_GRAY(finfo). > > > > Are you planning to send a new version with the above suggestion or > should we go ahead with this version ? > > (not asking you to re-send, but the macro looks nice :blink: :blink:) > > > > Appart from this cosmetic suggestion: > > > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com> > > > > > + } > > > + > > > + return false; > > > +} > > > + > > > +/* > > > + * Data recorded for each caps structure candidate, used to rank them > > > + * against each other via operator<(). > > > + */ > > > +struct CapsCandidate { > > > + guint index; > > > + bool is_raw; > > > + bool is_fixed; > > > + guint delta; > > > + > > > + bool operator<(const CapsCandidate &other) const > > > + { > > > + /* > > > + * Raw formats are likely to be useful only to applications that > > > + * specifically ask for them. Other applications will typically be > > > + * unable to handle them and fail, so prefer non-raw candidates. > > > + */ > > > + if (is_raw != other.is_raw) > > > + return !is_raw; > > > + > > > + /* Prefer a reliable fixed value over a range. */ > > > + if (is_fixed != other.is_fixed) > > > + return is_fixed; > > > + > > > + /* Otherwise the closest size match wins. */ > > > + return delta < other.delta; > > > + } > > > +}; > > > + > > > +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > > GstCaps *caps, GstVideoTransferFunction *transfer) > > > { > > > 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)); > > > > > > - /* Lookup the structure for a close match to the stream_cfg.size */ > > > + /* > > > + * Build a candidate for every available caps structure, and keep > > > + * track of the best one seen so far, as ranked by > > > + * CapsCandidate::operator<(). > > > + */ > > > + std::optional<CapsCandidate> best; > > > + > > > for (i = 0; i < gst_caps_get_size(caps); i++) { > > > s = gst_caps_get_structure(caps, i); > > > gint width, height; > > > - guint delta; > > > + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > > + gst_structure_has_field_typed(s, "height", G_TYPE_INT); > > > > > > - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && > > > - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { > > > + if (is_fixed) { > > > 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); > > > + guint 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; > > > - } > > > - } > > > + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; > > > + > > > + if (!best || candidate < *best) > > > + best = candidate; > > > + } > > > + > > > + if (!best) { > > > + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); > > > + return false; > > > } > > > > > > - /* 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); > > > + s = gst_caps_get_structure(caps, best->index); > > > > > > if (gst_structure_has_name(s, "video/x-raw")) { > > > const gchar *format = gst_video_format_to_string(gst_format); > > > @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, > > > > > > stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); > > > } > > > + > > > + return true; > > > } > > > > > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, > > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h > > > index 35df56fb9..5ce0e839c 100644 > > > --- a/src/gstreamer/gstlibcamera-utils.h > > > +++ b/src/gstreamer/gstlibcamera-utils.h > > > @@ -18,7 +18,7 @@ > > > GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); > > > GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, > > > GstVideoTransferFunction transfer); > > > -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > > +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, > > > GstCaps *caps, GstVideoTransferFunction *transfer); > > > void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); > > > void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp > > > index 9061f9163..66b31ce82 100644 > > > --- a/src/gstreamer/gstlibcamerasrc.cpp > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp > > > @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) > > > > > > /* Fixate caps and configure the stream. */ > > > caps = gst_caps_make_writable(caps); > > > - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); > > > + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) > > > + return false; > > > gst_libcamera_get_framerate_from_caps(caps, element_caps); > > > } > > > > >
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp index 6541d478c..996467ef4 100644 --- a/src/gstreamer/gstlibcamera-utils.cpp +++ b/src/gstreamer/gstlibcamera-utils.cpp @@ -8,6 +8,7 @@ #include "gstlibcamera-utils.h" +#include <optional> #include <string> #include <libcamera/control_ids.h> @@ -435,61 +436,111 @@ gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg return caps; } -void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, +/* + * We will want to distinguish between caps structures corresponding to + * raw and non-raw (processed) output images, as this will help inform + * our choice of preferred format. + * + * We identify Bayer as being the principle "raw" format here, but note + * that we are considering greyscale (images with one component) to be + * raw too (e.g. a raw monochrome sensor). + */ +static bool +gst_libcamera_structure_is_raw_capture(const GstStructure *s) +{ + if (gst_structure_has_name(s, "video/x-bayer")) + return true; + + if (gst_structure_has_name(s, "video/x-raw")) { + const gchar *format = gst_structure_get_string(s, "format"); + if (!format) + return false; + + const GstVideoFormatInfo *finfo = + gst_video_format_get_info(gst_video_format_from_string(format)); + + return finfo->n_components == 1; + } + + return false; +} + +/* + * Data recorded for each caps structure candidate, used to rank them + * against each other via operator<(). + */ +struct CapsCandidate { + guint index; + bool is_raw; + bool is_fixed; + guint delta; + + bool operator<(const CapsCandidate &other) const + { + /* + * Raw formats are likely to be useful only to applications that + * specifically ask for them. Other applications will typically be + * unable to handle them and fail, so prefer non-raw candidates. + */ + if (is_raw != other.is_raw) + return !is_raw; + + /* Prefer a reliable fixed value over a range. */ + if (is_fixed != other.is_fixed) + return is_fixed; + + /* Otherwise the closest size match wins. */ + return delta < other.delta; + } +}; + +bool gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, GstCaps *caps, GstVideoTransferFunction *transfer) { 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)); - /* Lookup the structure for a close match to the stream_cfg.size */ + /* + * Build a candidate for every available caps structure, and keep + * track of the best one seen so far, as ranked by + * CapsCandidate::operator<(). + */ + std::optional<CapsCandidate> best; + for (i = 0; i < gst_caps_get_size(caps); i++) { s = gst_caps_get_structure(caps, i); gint width, height; - guint delta; + bool is_fixed = gst_structure_has_field_typed(s, "width", G_TYPE_INT) && + gst_structure_has_field_typed(s, "height", G_TYPE_INT); - if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) && - gst_structure_has_field_typed(s, "height", G_TYPE_INT)) { + if (is_fixed) { 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); + guint 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; - } - } + CapsCandidate candidate{ i, gst_libcamera_structure_is_raw_capture(s), is_fixed, delta }; + + if (!best || candidate < *best) + best = candidate; + } + + if (!best) { + GST_WARNING("Failed to find a suitable caps structure to configure the stream"); + return false; } - /* 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); + s = gst_caps_get_structure(caps, best->index); if (gst_structure_has_name(s, "video/x-raw")) { const gchar *format = gst_video_format_to_string(gst_format); @@ -529,6 +580,8 @@ void gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg, stream_cfg.colorSpace = colorspace_from_colorimetry(colorimetry, transfer); } + + return true; } void gst_libcamera_get_framerate_from_caps(GstCaps *caps, diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h index 35df56fb9..5ce0e839c 100644 --- a/src/gstreamer/gstlibcamera-utils.h +++ b/src/gstreamer/gstlibcamera-utils.h @@ -18,7 +18,7 @@ GstCaps *gst_libcamera_stream_formats_to_caps(const libcamera::StreamFormats &formats); GstCaps *gst_libcamera_stream_configuration_to_caps(const libcamera::StreamConfiguration &stream_cfg, GstVideoTransferFunction transfer); -void gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, +bool gst_libcamera_configure_stream_from_caps(libcamera::StreamConfiguration &stream_cfg, GstCaps *caps, GstVideoTransferFunction *transfer); void gst_libcamera_get_framerate_from_caps(GstCaps *caps, GstStructure *element_caps); void gst_libcamera_clamp_and_set_frameduration(libcamera::ControlList &controls, diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp index 9061f9163..66b31ce82 100644 --- a/src/gstreamer/gstlibcamerasrc.cpp +++ b/src/gstreamer/gstlibcamerasrc.cpp @@ -605,7 +605,8 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self) /* Fixate caps and configure the stream. */ caps = gst_caps_make_writable(caps); - gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i]); + if (!gst_libcamera_configure_stream_from_caps(stream_cfg, caps, &transfer[i])) + return false; gst_libcamera_get_framerate_from_caps(caps, element_caps); }
Existing code was preferring fixed size outputs over ones with ranges, resulting in raw output formats being preferred even when applications ("cheese" is one such example) aren't expecting them and subsequently fail. The revised version still prefers fixed size outputs over ones with ranges, but will prefer non-raw (meaning non-Bayer) formats where these are available. This still allows applications that explicitly request raw formats to get them, but other applications will normally get non-raw formats which they handle better. Fixes, for example, "cheese" on Raspberry Pi. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/gstreamer/gstlibcamera-utils.cpp | 115 +++++++++++++++++++-------- src/gstreamer/gstlibcamera-utils.h | 2 +- src/gstreamer/gstlibcamerasrc.cpp | 3 +- 3 files changed, 87 insertions(+), 33 deletions(-)