[v4] gstreamer: Add sensor-config property
diff mbox series

Message ID 20260330101654.3332-1-david.plowman@raspberrypi.com
State New
Headers show
Series
  • [v4] gstreamer: Add sensor-config property
Related show

Commit Message

David Plowman March 30, 2026, 10:14 a.m. UTC
The sensor-config property may optionally be specified to give the
outputSize and bitDepth of the SensorConfiguration that is applied to
the camera configuration. For example, use

libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10"

to request the 10-bit 2304x1296 mode of a sensor.

All three parameters, width, height and bit depth, must be present, or
it will issue a warning and revert to automatic mode selection (as if
there were no sensor-config at all).

If a mode is requested that doesn't exist then camera configuration
will fail and the pipeline won't start.

As future-proofing, the SensorConfiguration's binning, increment and
analog-crop parameters may be specified, though libcamera currently
ignores them.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/300
---
 src/gstreamer/gstlibcamerasrc.cpp | 73 +++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

Comments

Nicolas Dufresne March 30, 2026, 3:40 p.m. UTC | #1
Hi,

Le lundi 30 mars 2026 à 11:14 +0100, David Plowman a écrit :
> +			/* Optional skipping (defaults to 1 for all increments). */
> +			gint xOddInc, xEvenInc, yOddInc, yEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
> +				sensorCfg.skipping.xOddInc = xOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
> +				sensorCfg.skipping.xEvenInc = xEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
> +				sensorCfg.skipping.yOddInc = yOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
> +				sensorCfg.skipping.yEvenInc = yEvenInc;
> +

Just wanted to let other reviewers know that I'm not very familiar this specific
feature. So I accepted that naming in the sense that "it matches mostly" the
libcamera API. But as a users, x-odd-inc and so on is not specially intuitive.
Would be nice with someone that knows this un-implemented design better can
review the names here.

Nicolas
Nicolas Dufresne March 30, 2026, 3:41 p.m. UTC | #2
Le lundi 30 mars 2026 à 11:14 +0100, David Plowman a écrit :
> The sensor-config property may optionally be specified to give the
> outputSize and bitDepth of the SensorConfiguration that is applied to
> the camera configuration. For example, use
> 
> libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10"
> 
> to request the 10-bit 2304x1296 mode of a sensor.
> 
> All three parameters, width, height and bit depth, must be present, or
> it will issue a warning and revert to automatic mode selection (as if
> there were no sensor-config at all).
> 
> If a mode is requested that doesn't exist then camera configuration
> will fail and the pipeline won't start.
> 
> As future-proofing, the SensorConfiguration's binning, increment and
> analog-crop parameters may be specified, though libcamera currently
> ignores them.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/300
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 73 +++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index a7241e9b..5d3ee3a2 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -141,6 +141,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> +	GstStructure *sensor_config;
>  
>  	std::atomic<GstEvent *> pending_eos;
>  
> @@ -152,6 +153,7 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> +	PROP_SENSOR_CONFIG,
>  	PROP_LAST
>  };
>  
> @@ -846,6 +848,55 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	}
>  	g_assert(state->config_->size() == state->srcpads_.size());
>  
> +	/* Apply optional sensor configuration. */
> +	if (self->sensor_config) {
> +		gint w = 0, h = 0, depth = 0;
> +		if (!gst_structure_get_int(self->sensor_config, "width", &w) ||
> +		    !gst_structure_get_int(self->sensor_config, "height", &h) ||
> +		    !gst_structure_get_int(self->sensor_config, "depth", &depth) ||
> +		    w <= 0 || h <= 0 || depth <= 0) {
> +			GST_ELEMENT_WARNING(self, RESOURCE, SETTINGS,
> +					    ("sensor-config requires non-zero width, height and depth"
> +					     " fields, ignoring"),
> +					    (nullptr));
> +		} else {
> +			SensorConfiguration sensorCfg;
> +			sensorCfg.outputSize = Size(w, h);
> +			sensorCfg.bitDepth = depth;
> +
> +			/* Optional binning (defaults to 1x1). */
> +			gint binX, binY;
> +			if (gst_structure_get_int(self->sensor_config, "bin-x", &binX) && binX > 0)
> +				sensorCfg.binning.binX = binX;
> +			if (gst_structure_get_int(self->sensor_config, "bin-y", &binY) && binY > 0)
> +				sensorCfg.binning.binY = binY;
> +
> +			/* Optional skipping (defaults to 1 for all increments). */
> +			gint xOddInc, xEvenInc, yOddInc, yEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
> +				sensorCfg.skipping.xOddInc = xOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
> +				sensorCfg.skipping.xEvenInc = xEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
> +				sensorCfg.skipping.yOddInc = yOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
> +				sensorCfg.skipping.yEvenInc = yEvenInc;
> +
> +			/* Optional analog crop (defaults to "unset"). */
> +			gint cropX, cropY, cropW, cropH;
> +			if (gst_structure_get_int(self->sensor_config, "analog-crop-x", &cropX) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-y", &cropY) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-width", &cropW) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-height", &cropH) &&
> +			    cropX >= 0 && cropY >= 0 && cropW > 0 && cropH > 0)
> +				sensorCfg.analogCrop = Rectangle(cropX, cropY,
> +								 static_cast<unsigned int>(cropW),
> +								 static_cast<unsigned int>(cropH));

Ack.

> +
> +			state->config_->sensorConfig = sensorCfg;
> +		}
> +	}
> +
>  	if (!gst_libcamera_src_negotiate(self)) {
>  		state->initControls_.clear();
>  		GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
> @@ -934,6 +985,10 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
>  		g_free(self->camera_name);
>  		self->camera_name = g_value_dup_string(value);
>  		break;
> +	case PROP_SENSOR_CONFIG:
> +		g_clear_pointer(&self->sensor_config, gst_structure_free);
> +		self->sensor_config = (GstStructure *)g_value_dup_boxed(value);
> +		break;
>  	default:
>  		if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -953,6 +1008,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
>  	case PROP_CAMERA_NAME:
>  		g_value_set_string(value, self->camera_name);
>  		break;
> +	case PROP_SENSOR_CONFIG:
> +		g_value_set_boxed(value, self->sensor_config);
> +		break;
>  	default:
>  		if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -1040,6 +1098,7 @@ gst_libcamera_src_finalize(GObject *object)
>  	g_clear_object(&self->task);
>  	g_mutex_clear(&self->state->lock_);
>  	g_free(self->camera_name);
> +	g_clear_pointer(&self->sensor_config, gst_structure_free);
>  	delete self->state;
>  
>  	return klass->finalize(object);
> @@ -1162,6 +1221,20 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  							     | G_PARAM_STATIC_STRINGS));
>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
>  
> +	spec = g_param_spec_boxed("sensor-config", "Sensor Config",
> +				  "Desired sensor configuration as a GstStructure with mandatory "
> +				  "fields width, height and depth, and optional fields bin-x, bin-y, "
> +				  "x-odd-inc, x-even-inc, y-odd-inc, y-even-inc, "
> +				  "analog-crop-x, analog-crop-y, analog-crop-width, analog-crop-height "
> +				  "(e.g. \"sensor/config,width=2304,height=1296,depth=10\"). "
> +				  "Leave unset to let the pipeline negotiate the sensor mode automatically.",
> +				  GST_TYPE_STRUCTURE,
> +				  (GParamFlags)(GST_PARAM_MUTABLE_READY
> +						| G_PARAM_CONSTRUCT
> +						| G_PARAM_READWRITE
> +						| G_PARAM_STATIC_STRINGS));
> +	g_object_class_install_property(object_class, PROP_SENSOR_CONFIG, spec);
> +
>  	GstCameraControls::installProperties(object_class, PROP_LAST);
>  }
>
Jacopo Mondi March 31, 2026, 3:54 p.m. UTC | #3
Hi Nicolas,

On Mon, Mar 30, 2026 at 11:40:45AM -0400, Nicolas Dufresne wrote:
> Hi,
>
> Le lundi 30 mars 2026 à 11:14 +0100, David Plowman a écrit :
> > +			/* Optional skipping (defaults to 1 for all increments). */
> > +			gint xOddInc, xEvenInc, yOddInc, yEvenInc;
> > +			if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
> > +				sensorCfg.skipping.xOddInc = xOddInc;
> > +			if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
> > +				sensorCfg.skipping.xEvenInc = xEvenInc;
> > +			if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
> > +				sensorCfg.skipping.yOddInc = yOddInc;
> > +			if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
> > +				sensorCfg.skipping.yEvenInc = yEvenInc;
> > +
>
> Just wanted to let other reviewers know that I'm not very familiar this specific
> feature. So I accepted that naming in the sense that "it matches mostly" the
> libcamera API. But as a users, x-odd-inc and so on is not specially intuitive.
> Would be nice with someone that knows this un-implemented design better can
> review the names here.

Unfortunately the concept itself ("pixel increment counter for odd rows")
is not easy to summarize with a nice name, if that's what concerns
you.

Anyone who's playing with sensor configurations and tries to configure
the skipping/binning modes should find those terms ... familiar (?) as
they're also used by the MIPI CCS specs as well (see 8.2.3
"Sub-Sampled Readout")

Of course, if there are better proposals I welcome them.



>
> Nicolas
Jacopo Mondi April 9, 2026, 10:22 a.m. UTC | #4
Hello
   sorry, I only replied to Nicolas' question but didn't fully review
the patch

On Mon, Mar 30, 2026 at 11:14:43AM +0100, David Plowman wrote:
> The sensor-config property may optionally be specified to give the
> outputSize and bitDepth of the SensorConfiguration that is applied to
> the camera configuration. For example, use
>
> libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10"
>
> to request the 10-bit 2304x1296 mode of a sensor.
>
> All three parameters, width, height and bit depth, must be present, or
> it will issue a warning and revert to automatic mode selection (as if
> there were no sensor-config at all).
>
> If a mode is requested that doesn't exist then camera configuration
> will fail and the pipeline won't start.
>
> As future-proofing, the SensorConfiguration's binning, increment and
> analog-crop parameters may be specified, though libcamera currently
> ignores them.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/300
> ---
>  src/gstreamer/gstlibcamerasrc.cpp | 73 +++++++++++++++++++++++++++++++
>  1 file changed, 73 insertions(+)
>
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index a7241e9b..5d3ee3a2 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -141,6 +141,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>
>  	gchar *camera_name;
> +	GstStructure *sensor_config;
>
>  	std::atomic<GstEvent *> pending_eos;
>
> @@ -152,6 +153,7 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> +	PROP_SENSOR_CONFIG,
>  	PROP_LAST
>  };
>
> @@ -846,6 +848,55 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
>  	}
>  	g_assert(state->config_->size() == state->srcpads_.size());
>
> +	/* Apply optional sensor configuration. */
> +	if (self->sensor_config) {
> +		gint w = 0, h = 0, depth = 0;
> +		if (!gst_structure_get_int(self->sensor_config, "width", &w) ||
> +		    !gst_structure_get_int(self->sensor_config, "height", &h) ||
> +		    !gst_structure_get_int(self->sensor_config, "depth", &depth) ||
> +		    w <= 0 || h <= 0 || depth <= 0) {

We mandate all fields to be populated in the documentation of
SensorConfig, but all other fields (skipping and binning) are
currently defaulted to 1 as long as we don't actually fix the
sensor mode selection on the Linux side.

I think it's fine to have the same constraint here.

> +			GST_ELEMENT_WARNING(self, RESOURCE, SETTINGS,
> +					    ("sensor-config requires non-zero width, height and depth"
> +					     " fields, ignoring"),
> +					    (nullptr));
> +		} else {
> +			SensorConfiguration sensorCfg;
> +			sensorCfg.outputSize = Size(w, h);
> +			sensorCfg.bitDepth = depth;
> +
> +			/* Optional binning (defaults to 1x1). */
> +			gint binX, binY;
> +			if (gst_structure_get_int(self->sensor_config, "bin-x", &binX) && binX > 0)
> +				sensorCfg.binning.binX = binX;
> +			if (gst_structure_get_int(self->sensor_config, "bin-y", &binY) && binY > 0)
> +				sensorCfg.binning.binY = binY;

Just wondering if we should enforce that if one of the two binning
directions is specified the other one should be specified as well, or
it's fine to rely on the fact we currently default both values to 1.

The above validation of self->sensor_config works on the assumption
that binning and skipping are default initialized to 1, hence I think
it's fine to make the same assumption here ?

> +
> +			/* Optional skipping (defaults to 1 for all increments). */
> +			gint xOddInc, xEvenInc, yOddInc, yEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
> +				sensorCfg.skipping.xOddInc = xOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
> +				sensorCfg.skipping.xEvenInc = xEvenInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
> +				sensorCfg.skipping.yOddInc = yOddInc;
> +			if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
> +				sensorCfg.skipping.yEvenInc = yEvenInc;

The same reasoning made for binning applies to skipping

> +
> +			/* Optional analog crop (defaults to "unset"). */
> +			gint cropX, cropY, cropW, cropH;
> +			if (gst_structure_get_int(self->sensor_config, "analog-crop-x", &cropX) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-y", &cropY) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-width", &cropW) &&
> +			    gst_structure_get_int(self->sensor_config, "analog-crop-height", &cropH) &&
> +			    cropX >= 0 && cropY >= 0 && cropW > 0 && cropH > 0)
> +				sensorCfg.analogCrop = Rectangle(cropX, cropY,
> +								 static_cast<unsigned int>(cropW),
> +								 static_cast<unsigned int>(cropH));
> +
> +			state->config_->sensorConfig = sensorCfg;

a little detail but it could be possible to avoid a copy by populating
the fileds of state->config_->sensorConfig. A detail though.

> +		}
> +	}
> +
>  	if (!gst_libcamera_src_negotiate(self)) {
>  		state->initControls_.clear();
>  		GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
> @@ -934,6 +985,10 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
>  		g_free(self->camera_name);
>  		self->camera_name = g_value_dup_string(value);
>  		break;
> +	case PROP_SENSOR_CONFIG:
> +		g_clear_pointer(&self->sensor_config, gst_structure_free);
> +		self->sensor_config = (GstStructure *)g_value_dup_boxed(value);
> +		break;
>  	default:
>  		if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -953,6 +1008,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
>  	case PROP_CAMERA_NAME:
>  		g_value_set_string(value, self->camera_name);
>  		break;
> +	case PROP_SENSOR_CONFIG:
> +		g_value_set_boxed(value, self->sensor_config);
> +		break;
>  	default:
>  		if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -1040,6 +1098,7 @@ gst_libcamera_src_finalize(GObject *object)
>  	g_clear_object(&self->task);
>  	g_mutex_clear(&self->state->lock_);
>  	g_free(self->camera_name);
> +	g_clear_pointer(&self->sensor_config, gst_structure_free);
>  	delete self->state;
>
>  	return klass->finalize(object);
> @@ -1162,6 +1221,20 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  							     | G_PARAM_STATIC_STRINGS));
>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
>
> +	spec = g_param_spec_boxed("sensor-config", "Sensor Config",
> +				  "Desired sensor configuration as a GstStructure with mandatory "
> +				  "fields width, height and depth, and optional fields bin-x, bin-y, "
> +				  "x-odd-inc, x-even-inc, y-odd-inc, y-even-inc, "
> +				  "analog-crop-x, analog-crop-y, analog-crop-width, analog-crop-height "
> +				  "(e.g. \"sensor/config,width=2304,height=1296,depth=10\"). "
> +				  "Leave unset to let the pipeline negotiate the sensor mode automatically.",
> +				  GST_TYPE_STRUCTURE,
> +				  (GParamFlags)(GST_PARAM_MUTABLE_READY
> +						| G_PARAM_CONSTRUCT
> +						| G_PARAM_READWRITE
> +						| G_PARAM_STATIC_STRINGS));
> +	g_object_class_install_property(object_class, PROP_SENSOR_CONFIG, spec);
> +

I'll let other comment on the gstreamer specific parts :)

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j
>  	GstCameraControls::installProperties(object_class, PROP_LAST);
>  }
>
> --
> 2.47.3
>
Fabien Danieau April 9, 2026, 3:37 p.m. UTC | #5
Tested on a Raspberry Pi 4 with Camera Module 3.

Without the patch, requesting a 1280x720 output results in a cropped
field of view, as the pipeline handler selects a cropped sensor mode.
With the patch, specifying
sensor-config="sensor/config,width=2304,height=1296,depth=10"
correctly forces the full sensor mode and preserves the full field of
view at 1280x720 output.

Test pipeline:

gst-launch-1.0 -e libcamerasrc \
  sensor-config="sensor/config,width=2304,height=1296,depth=10" ! \
  capsfilter caps=video/x-raw,width=1280,height=720,framerate=30/1,format=I420 ! \
  queue ! jpegenc ! avimux ! filesink location=output.avi

Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
David Plowman April 9, 2026, 3:55 p.m. UTC | #6
Hi Jacopo

Thanks very much for the review!

On Thu, 9 Apr 2026 at 11:23, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote:
>
> Hello
>    sorry, I only replied to Nicolas' question but didn't fully review
> the patch
>
> On Mon, Mar 30, 2026 at 11:14:43AM +0100, David Plowman wrote:
> > The sensor-config property may optionally be specified to give the
> > outputSize and bitDepth of the SensorConfiguration that is applied to
> > the camera configuration. For example, use
> >
> > libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10"
> >
> > to request the 10-bit 2304x1296 mode of a sensor.
> >
> > All three parameters, width, height and bit depth, must be present, or
> > it will issue a warning and revert to automatic mode selection (as if
> > there were no sensor-config at all).
> >
> > If a mode is requested that doesn't exist then camera configuration
> > will fail and the pipeline won't start.
> >
> > As future-proofing, the SensorConfiguration's binning, increment and
> > analog-crop parameters may be specified, though libcamera currently
> > ignores them.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/300
> > ---
> >  src/gstreamer/gstlibcamerasrc.cpp | 73 +++++++++++++++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index a7241e9b..5d3ee3a2 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -141,6 +141,7 @@ struct _GstLibcameraSrc {
> >       GstTask *task;
> >
> >       gchar *camera_name;
> > +     GstStructure *sensor_config;
> >
> >       std::atomic<GstEvent *> pending_eos;
> >
> > @@ -152,6 +153,7 @@ struct _GstLibcameraSrc {
> >  enum {
> >       PROP_0,
> >       PROP_CAMERA_NAME,
> > +     PROP_SENSOR_CONFIG,
> >       PROP_LAST
> >  };
> >
> > @@ -846,6 +848,55 @@ gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
> >       }
> >       g_assert(state->config_->size() == state->srcpads_.size());
> >
> > +     /* Apply optional sensor configuration. */
> > +     if (self->sensor_config) {
> > +             gint w = 0, h = 0, depth = 0;
> > +             if (!gst_structure_get_int(self->sensor_config, "width", &w) ||
> > +                 !gst_structure_get_int(self->sensor_config, "height", &h) ||
> > +                 !gst_structure_get_int(self->sensor_config, "depth", &depth) ||
> > +                 w <= 0 || h <= 0 || depth <= 0) {
>
> We mandate all fields to be populated in the documentation of
> SensorConfig, but all other fields (skipping and binning) are
> currently defaulted to 1 as long as we don't actually fix the
> sensor mode selection on the Linux side.
>
> I think it's fine to have the same constraint here.

Agree!

>
> > +                     GST_ELEMENT_WARNING(self, RESOURCE, SETTINGS,
> > +                                         ("sensor-config requires non-zero width, height and depth"
> > +                                          " fields, ignoring"),
> > +                                         (nullptr));
> > +             } else {
> > +                     SensorConfiguration sensorCfg;
> > +                     sensorCfg.outputSize = Size(w, h);
> > +                     sensorCfg.bitDepth = depth;
> > +
> > +                     /* Optional binning (defaults to 1x1). */
> > +                     gint binX, binY;
> > +                     if (gst_structure_get_int(self->sensor_config, "bin-x", &binX) && binX > 0)
> > +                             sensorCfg.binning.binX = binX;
> > +                     if (gst_structure_get_int(self->sensor_config, "bin-y", &binY) && binY > 0)
> > +                             sensorCfg.binning.binY = binY;
>
> Just wondering if we should enforce that if one of the two binning
> directions is specified the other one should be specified as well, or
> it's fine to rely on the fact we currently default both values to 1.
>
> The above validation of self->sensor_config works on the assumption
> that binning and skipping are default initialized to 1, hence I think
> it's fine to make the same assumption here ?

Yes, it all works because everything gets initialised to 1, so it
behaves just like the C++ API really.

There are certainly questions as to what further constraints might
make sense here - but as there aren't any defined in C++ yet, is it
worth doing something a bit speculative? I don't know. Actually I'd be
happy to drop all the bin/inc fields entirely, though having them does
mean that if they did start being used, then they're already
available, which could be a plus. Overall I feel I'm slightly inclined
to do nothing, mostly because I wouldn't know what to do instead, but
am certainly open to further persuasion...

>
> > +
> > +                     /* Optional skipping (defaults to 1 for all increments). */
> > +                     gint xOddInc, xEvenInc, yOddInc, yEvenInc;
> > +                     if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
> > +                             sensorCfg.skipping.xOddInc = xOddInc;
> > +                     if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
> > +                             sensorCfg.skipping.xEvenInc = xEvenInc;
> > +                     if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
> > +                             sensorCfg.skipping.yOddInc = yOddInc;
> > +                     if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
> > +                             sensorCfg.skipping.yEvenInc = yEvenInc;
>
> The same reasoning made for binning applies to skipping

Yes, same here! Not really sure what else would make sense instead...

>
> > +
> > +                     /* Optional analog crop (defaults to "unset"). */
> > +                     gint cropX, cropY, cropW, cropH;
> > +                     if (gst_structure_get_int(self->sensor_config, "analog-crop-x", &cropX) &&
> > +                         gst_structure_get_int(self->sensor_config, "analog-crop-y", &cropY) &&
> > +                         gst_structure_get_int(self->sensor_config, "analog-crop-width", &cropW) &&
> > +                         gst_structure_get_int(self->sensor_config, "analog-crop-height", &cropH) &&
> > +                         cropX >= 0 && cropY >= 0 && cropW > 0 && cropH > 0)
> > +                             sensorCfg.analogCrop = Rectangle(cropX, cropY,
> > +                                                              static_cast<unsigned int>(cropW),
> > +                                                              static_cast<unsigned int>(cropH));
> > +
> > +                     state->config_->sensorConfig = sensorCfg;
>
> a little detail but it could be possible to avoid a copy by populating
> the fileds of state->config_->sensorConfig. A detail though.

Yes, fair point. We could default-construct the
state->config_->sensorConfig and then populate the fields, though
(referring a bit to my hand-wringing above) I'm a bit nervous about a
future where we might change our minds and have code paths where
needed to remove it again! So I'm slightly inclined to stick with what
(arguably, of course) feels like the most obvious implementation,
given that the initialisation-time copy doesn't really seem of any
great consequence.

>
> > +             }
> > +     }
> > +
> >       if (!gst_libcamera_src_negotiate(self)) {
> >               state->initControls_.clear();
> >               GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
> > @@ -934,6 +985,10 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,
> >               g_free(self->camera_name);
> >               self->camera_name = g_value_dup_string(value);
> >               break;
> > +     case PROP_SENSOR_CONFIG:
> > +             g_clear_pointer(&self->sensor_config, gst_structure_free);
> > +             self->sensor_config = (GstStructure *)g_value_dup_boxed(value);
> > +             break;
> >       default:
> >               if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
> >                       G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > @@ -953,6 +1008,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
> >       case PROP_CAMERA_NAME:
> >               g_value_set_string(value, self->camera_name);
> >               break;
> > +     case PROP_SENSOR_CONFIG:
> > +             g_value_set_boxed(value, self->sensor_config);
> > +             break;
> >       default:
> >               if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
> >                       G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > @@ -1040,6 +1098,7 @@ gst_libcamera_src_finalize(GObject *object)
> >       g_clear_object(&self->task);
> >       g_mutex_clear(&self->state->lock_);
> >       g_free(self->camera_name);
> > +     g_clear_pointer(&self->sensor_config, gst_structure_free);
> >       delete self->state;
> >
> >       return klass->finalize(object);
> > @@ -1162,6 +1221,20 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >                                                            | G_PARAM_STATIC_STRINGS));
> >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> >
> > +     spec = g_param_spec_boxed("sensor-config", "Sensor Config",
> > +                               "Desired sensor configuration as a GstStructure with mandatory "
> > +                               "fields width, height and depth, and optional fields bin-x, bin-y, "
> > +                               "x-odd-inc, x-even-inc, y-odd-inc, y-even-inc, "
> > +                               "analog-crop-x, analog-crop-y, analog-crop-width, analog-crop-height "
> > +                               "(e.g. \"sensor/config,width=2304,height=1296,depth=10\"). "
> > +                               "Leave unset to let the pipeline negotiate the sensor mode automatically.",
> > +                               GST_TYPE_STRUCTURE,
> > +                               (GParamFlags)(GST_PARAM_MUTABLE_READY
> > +                                             | G_PARAM_CONSTRUCT
> > +                                             | G_PARAM_READWRITE
> > +                                             | G_PARAM_STATIC_STRINGS));
> > +     g_object_class_install_property(object_class, PROP_SENSOR_CONFIG, spec);
> > +
>
> I'll let other comment on the gstreamer specific parts :)
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks again!

David


David

>
> Thanks
>   j
> >       GstCameraControls::installProperties(object_class, PROP_LAST);
> >  }
> >
> > --
> > 2.47.3
> >
Laurent Pinchart April 9, 2026, 4:36 p.m. UTC | #7
Hello Fabien,

On Thu, Apr 09, 2026 at 05:37:25PM +0200, Fabien Danieau wrote:
> Tested on a Raspberry Pi 4 with Camera Module 3.
> 
> Without the patch, requesting a 1280x720 output results in a cropped
> field of view, as the pipeline handler selects a cropped sensor mode.
> With the patch, specifying
> sensor-config="sensor/config,width=2304,height=1296,depth=10"
> correctly forces the full sensor mode and preserves the full field of
> view at 1280x720 output.

Thank you for testing.

While configuring the sensor from GStreamer is a needed feature, it
sounds like there's also another issue somewhere. By default, libcamera
should not select a sensor configuration that will restrict the field of
view (at least not in both directions, if the requested aspect ratio
differs from the native aspect ratio, cropping in one direction is
expected), if not requested explicitly by the application.

David, any comment on this ?

> Test pipeline:
> 
> gst-launch-1.0 -e libcamerasrc \
>   sensor-config="sensor/config,width=2304,height=1296,depth=10" ! \
>   capsfilter caps=video/x-raw,width=1280,height=720,framerate=30/1,format=I420 ! \
>   queue ! jpegenc ! avimux ! filesink location=output.avi
> 
> Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
David Plowman April 10, 2026, 9:23 a.m. UTC | #8
Hi Laurent

On Thu, 9 Apr 2026 at 17:36, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello Fabien,
>
> On Thu, Apr 09, 2026 at 05:37:25PM +0200, Fabien Danieau wrote:
> > Tested on a Raspberry Pi 4 with Camera Module 3.
> >
> > Without the patch, requesting a 1280x720 output results in a cropped
> > field of view, as the pipeline handler selects a cropped sensor mode.
> > With the patch, specifying
> > sensor-config="sensor/config,width=2304,height=1296,depth=10"
> > correctly forces the full sensor mode and preserves the full field of
> > view at 1280x720 output.
>
> Thank you for testing.

Yes, thanks for doing that, Fabien!

>
> While configuring the sensor from GStreamer is a needed feature, it
> sounds like there's also another issue somewhere. By default, libcamera
> should not select a sensor configuration that will restrict the field of
> view (at least not in both directions, if the requested aspect ratio
> differs from the native aspect ratio, cropping in one direction is
> expected), if not requested explicitly by the application.
>
> David, any comment on this ?

Yes, fair question!

So the short answer (for those without the stamina to wade through the
rest of the discussion!!) is that, on the Pi at least, you have no
guarantees what the auto selection will choose. The only way to be
sure of the outcome is to specify the SensorConfiguration correctly.

The slightly longer answer is that our auto selection predates the
existence of the SensorConfiguration, so we made it possible for users
to get cropped modes with fast framerates (even where the aspect ratio
matches the full sensor resolution). The mechanism for that was by
choosing modes "closer" in resolution to the requested output
resolution; there wasn't really any other way. The case in point is
the 1536x864 120fps mode of the imx708 (full resolution 4608x2592). so
asking for an output "close" to 1536x864 would give you this mode.

Of course, you immediately get into questions of "how close" should
the resolutions be? "How similar" should aspect ratios be, how to
trade one against the other, and so on, leading us to (I won't deny
it!) a slightly unholy mixture of heuristics and fudge factors.

This all just reiterates why the SensorConfiguration is necessary, but
also leaves us in a position where it's hard to know what to do with
the auto selection. To be honest, I didn't even know about any rules
for the default behaviour but I think some of our users would be quite
irate if we changed anything that they've (for good or ill) come to
rely on, so we're probably stuck where we are.

I guess that's all largely orthogonal to the actual patch at hand, but
hopefully it does explain the situation at least a little bit...

Thanks (and congrats if you made it to the bottom!)

David

>
> > Test pipeline:
> >
> > gst-launch-1.0 -e libcamerasrc \
> >   sensor-config="sensor/config,width=2304,height=1296,depth=10" ! \
> >   capsfilter caps=video/x-raw,width=1280,height=720,framerate=30/1,format=I420 ! \
> >   queue ! jpegenc ! avimux ! filesink location=output.avi
> >
> > Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
>
> --
> Regards,
>
> Laurent Pinchart
Fabien Danieau April 14, 2026, 2:26 p.m. UTC | #9
Hi all,

Just a quick note on the tests with new high fps.

This doesn't work:
gst-launch-1.0 -e libcamerasrc sensor-
config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter
caps=video/x-raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc
! avimux ! filesink location=output.avi

while this does:
gst-launch-1.0 -e libcamerasrc ! capsfilter caps=video/x-
raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc ! avimux !
filesink location=output.avi

and going up to 56fps also works:
gst-launch-1.0 -e libcamerasrc sensor-
config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter
caps=video/x-raw,width=1280,height=720,framerate=56/1 ! queue ! jpegenc
! avimux ! filesink location=output.avi

Best,

Fabien

On Fri, 2026-04-10 at 10:23 +0100, David Plowman wrote:
> Hi Laurent
> 
> On Thu, 9 Apr 2026 at 17:36, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > 
> > Hello Fabien,
> > 
> > On Thu, Apr 09, 2026 at 05:37:25PM +0200, Fabien Danieau wrote:
> > > Tested on a Raspberry Pi 4 with Camera Module 3.
> > > 
> > > Without the patch, requesting a 1280x720 output results in a
> > > cropped
> > > field of view, as the pipeline handler selects a cropped sensor
> > > mode.
> > > With the patch, specifying
> > > sensor-config="sensor/config,width=2304,height=1296,depth=10"
> > > correctly forces the full sensor mode and preserves the full
> > > field of
> > > view at 1280x720 output.
> > 
> > Thank you for testing.
> 
> Yes, thanks for doing that, Fabien!
> 
> > 
> > While configuring the sensor from GStreamer is a needed feature, it
> > sounds like there's also another issue somewhere. By default,
> > libcamera
> > should not select a sensor configuration that will restrict the
> > field of
> > view (at least not in both directions, if the requested aspect
> > ratio
> > differs from the native aspect ratio, cropping in one direction is
> > expected), if not requested explicitly by the application.
> > 
> > David, any comment on this ?
> 
> Yes, fair question!
> 
> So the short answer (for those without the stamina to wade through
> the
> rest of the discussion!!) is that, on the Pi at least, you have no
> guarantees what the auto selection will choose. The only way to be
> sure of the outcome is to specify the SensorConfiguration correctly.
> 
> The slightly longer answer is that our auto selection predates the
> existence of the SensorConfiguration, so we made it possible for
> users
> to get cropped modes with fast framerates (even where the aspect
> ratio
> matches the full sensor resolution). The mechanism for that was by
> choosing modes "closer" in resolution to the requested output
> resolution; there wasn't really any other way. The case in point is
> the 1536x864 120fps mode of the imx708 (full resolution 4608x2592).
> so
> asking for an output "close" to 1536x864 would give you this mode.
> 
> Of course, you immediately get into questions of "how close" should
> the resolutions be? "How similar" should aspect ratios be, how to
> trade one against the other, and so on, leading us to (I won't deny
> it!) a slightly unholy mixture of heuristics and fudge factors.
> 
> This all just reiterates why the SensorConfiguration is necessary,
> but
> also leaves us in a position where it's hard to know what to do with
> the auto selection. To be honest, I didn't even know about any rules
> for the default behaviour but I think some of our users would be
> quite
> irate if we changed anything that they've (for good or ill) come to
> rely on, so we're probably stuck where we are.
> 
> I guess that's all largely orthogonal to the actual patch at hand,
> but
> hopefully it does explain the situation at least a little bit...
> 
> Thanks (and congrats if you made it to the bottom!)
> 
> David
> 
> > 
> > > Test pipeline:
> > > 
> > > gst-launch-1.0 -e libcamerasrc \
> > >   sensor-config="sensor/config,width=2304,height=1296,depth=10" !
> > > \
> > >   capsfilter caps=video/x-
> > > raw,width=1280,height=720,framerate=30/1,format=I420 ! \
> > >   queue ! jpegenc ! avimux ! filesink location=output.avi
> > > 
> > > Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
> > 
> > --
> > Regards,
> > 
> > Laurent Pinchart
David Plowman April 14, 2026, 3:20 p.m. UTC | #10
Hi Fabien

Thanks for the information. Yes, I think is the expected - intended,
even - behaviour:

Case 1: If you explicitly ask for the 2304x1296 mode, then you can't
have 60fps - it just won't go that fast!

Case 2: If you don't use the sensor-config you're at the mercy of the
auto mode selection, but which continues to do what it did before,
giving you the cropped mode with the fast framerate.

Case 3: Exactly so, the 2304x1296 mode goes up to 56fps but no more!

I think the moral of the story has always been that anyone who cares
strongly about the sensor mode should really consider saying exactly
what sensor-config they want (just as with the C++ API).

Thanks
David

On Tue, 14 Apr 2026 at 15:26, Fabien Danieau
<fabien.danieau@pollen-robotics.com> wrote:
>
> Hi all,
>
> Just a quick note on the tests with new high fps.
>
> This doesn't work:
> gst-launch-1.0 -e libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
>
> while this does:
> gst-launch-1.0 -e libcamerasrc ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
>
> and going up to 56fps also works:
> gst-launch-1.0 -e libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=56/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
>
> Best,
>
> Fabien
>
> On Fri, 2026-04-10 at 10:23 +0100, David Plowman wrote:
>
> Hi Laurent
>
> On Thu, 9 Apr 2026 at 17:36, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>
>
> Hello Fabien,
>
> On Thu, Apr 09, 2026 at 05:37:25PM +0200, Fabien Danieau wrote:
>
> Tested on a Raspberry Pi 4 with Camera Module 3.
>
> Without the patch, requesting a 1280x720 output results in a cropped
> field of view, as the pipeline handler selects a cropped sensor mode.
> With the patch, specifying
> sensor-config="sensor/config,width=2304,height=1296,depth=10"
> correctly forces the full sensor mode and preserves the full field of
> view at 1280x720 output.
>
>
> Thank you for testing.
>
>
> Yes, thanks for doing that, Fabien!
>
>
> While configuring the sensor from GStreamer is a needed feature, it
> sounds like there's also another issue somewhere. By default, libcamera
> should not select a sensor configuration that will restrict the field of
> view (at least not in both directions, if the requested aspect ratio
> differs from the native aspect ratio, cropping in one direction is
> expected), if not requested explicitly by the application.
>
> David, any comment on this ?
>
>
> Yes, fair question!
>
> So the short answer (for those without the stamina to wade through the
> rest of the discussion!!) is that, on the Pi at least, you have no
> guarantees what the auto selection will choose. The only way to be
> sure of the outcome is to specify the SensorConfiguration correctly.
>
> The slightly longer answer is that our auto selection predates the
> existence of the SensorConfiguration, so we made it possible for users
> to get cropped modes with fast framerates (even where the aspect ratio
> matches the full sensor resolution). The mechanism for that was by
> choosing modes "closer" in resolution to the requested output
> resolution; there wasn't really any other way. The case in point is
> the 1536x864 120fps mode of the imx708 (full resolution 4608x2592). so
> asking for an output "close" to 1536x864 would give you this mode.
>
> Of course, you immediately get into questions of "how close" should
> the resolutions be? "How similar" should aspect ratios be, how to
> trade one against the other, and so on, leading us to (I won't deny
> it!) a slightly unholy mixture of heuristics and fudge factors.
>
> This all just reiterates why the SensorConfiguration is necessary, but
> also leaves us in a position where it's hard to know what to do with
> the auto selection. To be honest, I didn't even know about any rules
> for the default behaviour but I think some of our users would be quite
> irate if we changed anything that they've (for good or ill) come to
> rely on, so we're probably stuck where we are.
>
> I guess that's all largely orthogonal to the actual patch at hand, but
> hopefully it does explain the situation at least a little bit...
>
> Thanks (and congrats if you made it to the bottom!)
>
> David
>
>
> Test pipeline:
>
> gst-launch-1.0 -e libcamerasrc \
>   sensor-config="sensor/config,width=2304,height=1296,depth=10" ! \
>   capsfilter caps=video/x-raw,width=1280,height=720,framerate=30/1,format=I420 ! \
>   queue ! jpegenc ! avimux ! filesink location=output.avi
>
> Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
>
>
> --
> Regards,
>
> Laurent Pinchart
Kieran Bingham April 14, 2026, 5:21 p.m. UTC | #11
Quoting David Plowman (2026-04-14 16:20:47)
> Hi Fabien
> 
> Thanks for the information. Yes, I think is the expected - intended,
> even - behaviour:
> 
> Case 1: If you explicitly ask for the 2304x1296 mode, then you can't
> have 60fps - it just won't go that fast!
> 
> Case 2: If you don't use the sensor-config you're at the mercy of the
> auto mode selection, but which continues to do what it did before,
> giving you the cropped mode with the fast framerate.
> 
> Case 3: Exactly so, the 2304x1296 mode goes up to 56fps but no more!
> 
> I think the moral of the story has always been that anyone who cares
> strongly about the sensor mode should really consider saying exactly
> what sensor-config they want (just as with the C++ API).

I know it's a long running/desired action, but still some how I think we
should really find a good way to convey the capabilities to the users.

I really like what the raspi-cam --list gives:

│ 0 : imx258 [4208x3120 10-bit RGGB]
│ (/base/axi/pcie@120000/rp1/i2c@88000/imx258@10)
│     Modes: 'SRGGB10_CSI2P' : 1048x780 [56.85 fps - (0, 0)/4208x3120 crop]
│                              2104x1560 [30.14 fps - (0, 0)/4208x3120 crop]
│                              4208x3120 [15.08 fps - (0, 0)/4208x3120 crop]

But we have to do a configuration dance to 'test' the camera driver
capabilities. Would be great to be able to avoid that.

And of course when a camera driver can be freely configured for cropping
and binning, the combinations just increase here...


Ultimately ... is this patch good to merge? It seems like it could be
good to go?

I see two RB tags anyway....


--
Kieran


> 
> Thanks
> David
> 
> On Tue, 14 Apr 2026 at 15:26, Fabien Danieau
> <fabien.danieau@pollen-robotics.com> wrote:
> >
> > Hi all,
> >
> > Just a quick note on the tests with new high fps.
> >
> > This doesn't work:
> > gst-launch-1.0 -e libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
> >
> > while this does:
> > gst-launch-1.0 -e libcamerasrc ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=60/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
> >
> > and going up to 56fps also works:
> > gst-launch-1.0 -e libcamerasrc sensor-config="sensor/config,width=2304,height=1296,depth=10" ! capsfilter caps=video/x-raw,width=1280,height=720,framerate=56/1 ! queue ! jpegenc ! avimux ! filesink location=output.avi
> >
> > Best,
> >
> > Fabien
> >
> > On Fri, 2026-04-10 at 10:23 +0100, David Plowman wrote:
> >
> > Hi Laurent
> >
> > On Thu, 9 Apr 2026 at 17:36, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> >
> >
> > Hello Fabien,
> >
> > On Thu, Apr 09, 2026 at 05:37:25PM +0200, Fabien Danieau wrote:
> >
> > Tested on a Raspberry Pi 4 with Camera Module 3.
> >
> > Without the patch, requesting a 1280x720 output results in a cropped
> > field of view, as the pipeline handler selects a cropped sensor mode.
> > With the patch, specifying
> > sensor-config="sensor/config,width=2304,height=1296,depth=10"
> > correctly forces the full sensor mode and preserves the full field of
> > view at 1280x720 output.
> >
> >
> > Thank you for testing.
> >
> >
> > Yes, thanks for doing that, Fabien!
> >
> >
> > While configuring the sensor from GStreamer is a needed feature, it
> > sounds like there's also another issue somewhere. By default, libcamera
> > should not select a sensor configuration that will restrict the field of
> > view (at least not in both directions, if the requested aspect ratio
> > differs from the native aspect ratio, cropping in one direction is
> > expected), if not requested explicitly by the application.
> >
> > David, any comment on this ?
> >
> >
> > Yes, fair question!
> >
> > So the short answer (for those without the stamina to wade through the
> > rest of the discussion!!) is that, on the Pi at least, you have no
> > guarantees what the auto selection will choose. The only way to be
> > sure of the outcome is to specify the SensorConfiguration correctly.
> >
> > The slightly longer answer is that our auto selection predates the
> > existence of the SensorConfiguration, so we made it possible for users
> > to get cropped modes with fast framerates (even where the aspect ratio
> > matches the full sensor resolution). The mechanism for that was by
> > choosing modes "closer" in resolution to the requested output
> > resolution; there wasn't really any other way. The case in point is
> > the 1536x864 120fps mode of the imx708 (full resolution 4608x2592). so
> > asking for an output "close" to 1536x864 would give you this mode.
> >
> > Of course, you immediately get into questions of "how close" should
> > the resolutions be? "How similar" should aspect ratios be, how to
> > trade one against the other, and so on, leading us to (I won't deny
> > it!) a slightly unholy mixture of heuristics and fudge factors.
> >
> > This all just reiterates why the SensorConfiguration is necessary, but
> > also leaves us in a position where it's hard to know what to do with
> > the auto selection. To be honest, I didn't even know about any rules
> > for the default behaviour but I think some of our users would be quite
> > irate if we changed anything that they've (for good or ill) come to
> > rely on, so we're probably stuck where we are.
> >
> > I guess that's all largely orthogonal to the actual patch at hand, but
> > hopefully it does explain the situation at least a little bit...
> >
> > Thanks (and congrats if you made it to the bottom!)
> >
> > David
> >
> >
> > Test pipeline:
> >
> > gst-launch-1.0 -e libcamerasrc \
> >   sensor-config="sensor/config,width=2304,height=1296,depth=10" ! \
> >   capsfilter caps=video/x-raw,width=1280,height=720,framerate=30/1,format=I420 ! \
> >   queue ! jpegenc ! avimux ! filesink location=output.avi
> >
> > Tested-by: Fabien Danieau <fabien.danieau@pollen-robotics.com>
> >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
Nicolas Dufresne April 14, 2026, 11:30 p.m. UTC | #12
Hi Kieran,

Le mardi 14 avril 2026 à 18:21 +0100, Kieran Bingham a écrit :
> Quoting David Plowman (2026-04-14 16:20:47)
> > Hi Fabien
> > 
> > Thanks for the information. Yes, I think is the expected - intended,
> > even - behaviour:
> > 
> > Case 1: If you explicitly ask for the 2304x1296 mode, then you can't
> > have 60fps - it just won't go that fast!
> > 
> > Case 2: If you don't use the sensor-config you're at the mercy of the
> > auto mode selection, but which continues to do what it did before,
> > giving you the cropped mode with the fast framerate.
> > 
> > Case 3: Exactly so, the 2304x1296 mode goes up to 56fps but no more!
> > 
> > I think the moral of the story has always been that anyone who cares
> > strongly about the sensor mode should really consider saying exactly
> > what sensor-config they want (just as with the C++ API).
> 
> I know it's a long running/desired action, but still some how I think we
> should really find a good way to convey the capabilities to the users.
> 
> I really like what the raspi-cam --list gives:
> 
> │ 0 : imx258 [4208x3120 10-bit RGGB]
> │ (/base/axi/pcie@120000/rp1/i2c@88000/imx258@10)
> │     Modes: 'SRGGB10_CSI2P' : 1048x780 [56.85 fps - (0, 0)/4208x3120 crop]
> │                              2104x1560 [30.14 fps - (0, 0)/4208x3120 crop]
> │                              4208x3120 [15.08 fps - (0, 0)/4208x3120 crop]
> 
> But we have to do a configuration dance to 'test' the camera driver
> capabilities. Would be great to be able to avoid that.
> 
> And of course when a camera driver can be freely configured for cropping
> and binning, the combinations just increase here...

I really like it to, but the dance was also very expensive and slow, and the
subset was artificial. The only way I can think of, would be to provide a list
of "well known modes" with the matching sensor configuration to achieve that.
But then, where do you maintain that list is something to think of, and how big
that list will be, what kind of rules we'd have. Seems like a project in itself.
UVC camera do pretty much that in their descriptor, but also offers nothing else
usually. This on libcamera would defeat the point of giving wider access to the
cameras.

> Ultimately ... is this patch good to merge? It seems like it could be
> good to go?
> 
> I see two RB tags anyway....

For me it is, there was discussion around the optional parameters, and while
valid question, no bug or violation of the API was found.

cheers,
Nicolas
David Plowman April 17, 2026, 7:35 a.m. UTC | #13
Hi again

Yes, it's certainly a thumbs up from me for merging this. I think as
it stands it captures the intention (as it currently stands) and
practice of the SensorConfiguration class pretty much directly.

This is probably the single biggest reason that I caution users about
using libcamera through gstreamer - because I know that when they
can't get the output resolution they want in the camera mode they
want, they're going to start complaining at me and it will all be my
fault. So it would be great finally to lance this boil, perhaps even
for the upcoming release? (Pretty please??)

Thanks
David

On Wed, 15 Apr 2026 at 00:30, Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Kieran,
>
> Le mardi 14 avril 2026 à 18:21 +0100, Kieran Bingham a écrit :
> > Quoting David Plowman (2026-04-14 16:20:47)
> > > Hi Fabien
> > >
> > > Thanks for the information. Yes, I think is the expected - intended,
> > > even - behaviour:
> > >
> > > Case 1: If you explicitly ask for the 2304x1296 mode, then you can't
> > > have 60fps - it just won't go that fast!
> > >
> > > Case 2: If you don't use the sensor-config you're at the mercy of the
> > > auto mode selection, but which continues to do what it did before,
> > > giving you the cropped mode with the fast framerate.
> > >
> > > Case 3: Exactly so, the 2304x1296 mode goes up to 56fps but no more!
> > >
> > > I think the moral of the story has always been that anyone who cares
> > > strongly about the sensor mode should really consider saying exactly
> > > what sensor-config they want (just as with the C++ API).
> >
> > I know it's a long running/desired action, but still some how I think we
> > should really find a good way to convey the capabilities to the users.
> >
> > I really like what the raspi-cam --list gives:
> >
> > │ 0 : imx258 [4208x3120 10-bit RGGB]
> > │ (/base/axi/pcie@120000/rp1/i2c@88000/imx258@10)
> > │     Modes: 'SRGGB10_CSI2P' : 1048x780 [56.85 fps - (0, 0)/4208x3120 crop]
> > │                              2104x1560 [30.14 fps - (0, 0)/4208x3120 crop]
> > │                              4208x3120 [15.08 fps - (0, 0)/4208x3120 crop]
> >
> > But we have to do a configuration dance to 'test' the camera driver
> > capabilities. Would be great to be able to avoid that.
> >
> > And of course when a camera driver can be freely configured for cropping
> > and binning, the combinations just increase here...
>
> I really like it to, but the dance was also very expensive and slow, and the
> subset was artificial. The only way I can think of, would be to provide a list
> of "well known modes" with the matching sensor configuration to achieve that.
> But then, where do you maintain that list is something to think of, and how big
> that list will be, what kind of rules we'd have. Seems like a project in itself.
> UVC camera do pretty much that in their descriptor, but also offers nothing else
> usually. This on libcamera would defeat the point of giving wider access to the
> cameras.
>
> > Ultimately ... is this patch good to merge? It seems like it could be
> > good to go?
> >
> > I see two RB tags anyway....
>
> For me it is, there was discussion around the optional parameters, and while
> valid question, no bug or violation of the API was found.
>
> cheers,
> Nicolas

Patch
diff mbox series

diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
index a7241e9b..5d3ee3a2 100644
--- a/src/gstreamer/gstlibcamerasrc.cpp
+++ b/src/gstreamer/gstlibcamerasrc.cpp
@@ -141,6 +141,7 @@  struct _GstLibcameraSrc {
 	GstTask *task;
 
 	gchar *camera_name;
+	GstStructure *sensor_config;
 
 	std::atomic<GstEvent *> pending_eos;
 
@@ -152,6 +153,7 @@  struct _GstLibcameraSrc {
 enum {
 	PROP_0,
 	PROP_CAMERA_NAME,
+	PROP_SENSOR_CONFIG,
 	PROP_LAST
 };
 
@@ -846,6 +848,55 @@  gst_libcamera_src_task_enter(GstTask *task, [[maybe_unused]] GThread *thread,
 	}
 	g_assert(state->config_->size() == state->srcpads_.size());
 
+	/* Apply optional sensor configuration. */
+	if (self->sensor_config) {
+		gint w = 0, h = 0, depth = 0;
+		if (!gst_structure_get_int(self->sensor_config, "width", &w) ||
+		    !gst_structure_get_int(self->sensor_config, "height", &h) ||
+		    !gst_structure_get_int(self->sensor_config, "depth", &depth) ||
+		    w <= 0 || h <= 0 || depth <= 0) {
+			GST_ELEMENT_WARNING(self, RESOURCE, SETTINGS,
+					    ("sensor-config requires non-zero width, height and depth"
+					     " fields, ignoring"),
+					    (nullptr));
+		} else {
+			SensorConfiguration sensorCfg;
+			sensorCfg.outputSize = Size(w, h);
+			sensorCfg.bitDepth = depth;
+
+			/* Optional binning (defaults to 1x1). */
+			gint binX, binY;
+			if (gst_structure_get_int(self->sensor_config, "bin-x", &binX) && binX > 0)
+				sensorCfg.binning.binX = binX;
+			if (gst_structure_get_int(self->sensor_config, "bin-y", &binY) && binY > 0)
+				sensorCfg.binning.binY = binY;
+
+			/* Optional skipping (defaults to 1 for all increments). */
+			gint xOddInc, xEvenInc, yOddInc, yEvenInc;
+			if (gst_structure_get_int(self->sensor_config, "x-odd-inc", &xOddInc) && xOddInc > 0)
+				sensorCfg.skipping.xOddInc = xOddInc;
+			if (gst_structure_get_int(self->sensor_config, "x-even-inc", &xEvenInc) && xEvenInc > 0)
+				sensorCfg.skipping.xEvenInc = xEvenInc;
+			if (gst_structure_get_int(self->sensor_config, "y-odd-inc", &yOddInc) && yOddInc > 0)
+				sensorCfg.skipping.yOddInc = yOddInc;
+			if (gst_structure_get_int(self->sensor_config, "y-even-inc", &yEvenInc) && yEvenInc > 0)
+				sensorCfg.skipping.yEvenInc = yEvenInc;
+
+			/* Optional analog crop (defaults to "unset"). */
+			gint cropX, cropY, cropW, cropH;
+			if (gst_structure_get_int(self->sensor_config, "analog-crop-x", &cropX) &&
+			    gst_structure_get_int(self->sensor_config, "analog-crop-y", &cropY) &&
+			    gst_structure_get_int(self->sensor_config, "analog-crop-width", &cropW) &&
+			    gst_structure_get_int(self->sensor_config, "analog-crop-height", &cropH) &&
+			    cropX >= 0 && cropY >= 0 && cropW > 0 && cropH > 0)
+				sensorCfg.analogCrop = Rectangle(cropX, cropY,
+								 static_cast<unsigned int>(cropW),
+								 static_cast<unsigned int>(cropH));
+
+			state->config_->sensorConfig = sensorCfg;
+		}
+	}
+
 	if (!gst_libcamera_src_negotiate(self)) {
 		state->initControls_.clear();
 		GST_ELEMENT_FLOW_ERROR(self, GST_FLOW_NOT_NEGOTIATED);
@@ -934,6 +985,10 @@  gst_libcamera_src_set_property(GObject *object, guint prop_id,
 		g_free(self->camera_name);
 		self->camera_name = g_value_dup_string(value);
 		break;
+	case PROP_SENSOR_CONFIG:
+		g_clear_pointer(&self->sensor_config, gst_structure_free);
+		self->sensor_config = (GstStructure *)g_value_dup_boxed(value);
+		break;
 	default:
 		if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
 			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
@@ -953,6 +1008,9 @@  gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,
 	case PROP_CAMERA_NAME:
 		g_value_set_string(value, self->camera_name);
 		break;
+	case PROP_SENSOR_CONFIG:
+		g_value_set_boxed(value, self->sensor_config);
+		break;
 	default:
 		if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
 			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
@@ -1040,6 +1098,7 @@  gst_libcamera_src_finalize(GObject *object)
 	g_clear_object(&self->task);
 	g_mutex_clear(&self->state->lock_);
 	g_free(self->camera_name);
+	g_clear_pointer(&self->sensor_config, gst_structure_free);
 	delete self->state;
 
 	return klass->finalize(object);
@@ -1162,6 +1221,20 @@  gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
 							     | G_PARAM_STATIC_STRINGS));
 	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
 
+	spec = g_param_spec_boxed("sensor-config", "Sensor Config",
+				  "Desired sensor configuration as a GstStructure with mandatory "
+				  "fields width, height and depth, and optional fields bin-x, bin-y, "
+				  "x-odd-inc, x-even-inc, y-odd-inc, y-even-inc, "
+				  "analog-crop-x, analog-crop-y, analog-crop-width, analog-crop-height "
+				  "(e.g. \"sensor/config,width=2304,height=1296,depth=10\"). "
+				  "Leave unset to let the pipeline negotiate the sensor mode automatically.",
+				  GST_TYPE_STRUCTURE,
+				  (GParamFlags)(GST_PARAM_MUTABLE_READY
+						| G_PARAM_CONSTRUCT
+						| G_PARAM_READWRITE
+						| G_PARAM_STATIC_STRINGS));
+	g_object_class_install_property(object_class, PROP_SENSOR_CONFIG, spec);
+
 	GstCameraControls::installProperties(object_class, PROP_LAST);
 }