[v5] gstreamer: Add support for Orientation

24615 diff mbox series

From: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>

Plumb the support for CameraConfiguration::orientation in libcamerasrc.
A new "orientation" property is introduced and mappings for
libcamera::Orientation <> GstVideoOrientationMethod are provided
with helpers.

Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
Co-developed-by: Umang Jain <uajain@igalia.com>
Signed-off-by: Umang Jain <uajain@igalia.com>
---
Changes in v5:
- patch scrubbing and cleanup
- If different orientation is returned than requested, update the
  property
- Minor string fixes, append appropriate tags

Link to v4: https://patchwork.libcamera.org/patch/23965/

Note: Checkstyle will complain on this patch on a hunk, but the existing format
is quite read-able IMO, hence ignored.
---
 src/gstreamer/gstlibcamera-utils.cpp | 36 ++++++++++++++++++++++++++++
 src/gstreamer/gstlibcamera-utils.h   |  3 +++
 src/gstreamer/gstlibcamerasrc.cpp    | 27 +++++++++++++++++++++
 3 files changed, 66 insertions(+)

Le mardi 14 octobre 2025 à 12:45 +0530, Umang Jain a écrit :
> From: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> 
> Plumb the support for CameraConfiguration::orientation in libcamerasrc.
> A new "orientation" property is introduced and mappings for
> libcamera::Orientation <> GstVideoOrientationMethod are provided
> with helpers.
> 
> Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> Co-developed-by: Umang Jain <uajain@igalia.com>
> Signed-off-by: Umang Jain <uajain@igalia.com>
> ---
> Changes in v5:
> - patch scrubbing and cleanup
> - If different orientation is returned than requested, update the
>   property
> - Minor string fixes, append appropriate tags
> 
> Link to v4: https://patchwork.libcamera.org/patch/23965/
> 
> Note: Checkstyle will complain on this patch on a hunk, but the existing format
> is quite read-able IMO, hence ignored.
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 36 ++++++++++++++++++++++++++++
>  src/gstreamer/gstlibcamera-utils.h   |  3 +++
>  src/gstreamer/gstlibcamerasrc.cpp    | 27 +++++++++++++++++++++
>  3 files changed, 66 insertions(+)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index bfb094c9..44050cbb 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -357,6 +357,42 @@ control_type_to_gtype(const ControlType &type)
>  	return G_TYPE_INVALID;
>  }
>  
> +static const struct {
> +	Orientation orientation;
> +	GstVideoOrientationMethod method;
> +} orientation_map[]{
> +	{ Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },
> +	{ Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },
> +	{ Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },
> +	{ Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },
> +	{ Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },
> +	{ Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },
> +	{ Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },
> +	{ Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },
> +};
> +
> +Orientation
> +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method)
> +{
> +	for (auto &b : orientation_map) {
> +		if (b.method == method)
> +			return b.orientation;
> +	}
> +
> +	return Orientation::Rotate0;
> +}
> +
> +GstVideoOrientationMethod
> +libcamera_orientation_to_gst_video_orientation(Orientation orientation)
> +{
> +	for (auto &a : orientation_map) {
> +		if (a.orientation == orientation)
> +			return a.method;
> +	}
> +
> +	return GST_VIDEO_ORIENTATION_IDENTITY;
> +}
> +
>  GstCaps *
>  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
>  {
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 35df56fb..4364811e 100644
> --- a/src/gstreamer/gstlibcamera-utils.h
> +++ b/src/gstreamer/gstlibcamera-utils.h
> @@ -10,6 +10,7 @@
>  
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/controls.h>
> +#include <libcamera/orientation.h>
>  #include <libcamera/stream.h>
>  
>  #include <gst/gst.h>
> @@ -32,6 +33,8 @@ libcamera::Rectangle gst_libcamera_gvalue_get_rectangle(const GValue *value);
>  int gst_libcamera_set_structure_field(GstStructure *structure,
>  				      const libcamera::ControlId *id,
>  				      const libcamera::ControlValue &value);
> +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
>  
>  #if !GST_CHECK_VERSION(1, 16, 0)
>  static inline void gst_clear_event(GstEvent **event_ptr)
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 79a025a5..2d5f0bf5 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> +	GstVideoOrientationMethod orientation;
>  
>  	std::atomic<GstEvent *> pending_eos;
>  
> @@ -157,6 +158,7 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> +	PROP_ORIENTATION,

The subject is ambiguous, I read this as libcamera would be providing the
orientation, and that this patch would simply translate it to a tag (image-
orientation) and push that in the pipeline. But in short, you are looking to
support sensor HFLIP and VFLIP, very rarely the other rotations are supported.

Please, update the subject and patch description so we can understand what this
patch is about.

>  	PROP_LAST
>  };
>  
> @@ -616,6 +618,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  		gst_libcamera_get_framerate_from_caps(caps, element_caps);
>  	}
>  
> +	/* Set orientation. */
> +	Orientation requestedOrientation = gst_video_orientation_to_libcamera_orientation(self->orientation);

Missing "GLibLocker lock(GST_OBJECT(object));" You can create a local get
function that does that.

> +	state->config_->orientation = requestedOrientation;
> +
>  	/* Validate the configuration. */
>  	CameraConfiguration::Status status = state->config_->validate();
>  	if (status == CameraConfiguration::Invalid)
> @@ -637,6 +643,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  	gst_libcamera_clamp_and_set_frameduration(state->initControls_,
>  						  state->cam_->controls(), element_caps);
>  
> +	/* If orientation is changed, update the property. */
> +	if (state->config_->orientation != requestedOrientation)
> +		g_object_set(G_OBJECT(self), "orientation", state->config_->orientation, nullptr);

Its always risky to call g_object_set() on self due to possible locking. This
case seem to work, but required me to think harder. Perhaps use the usual
method:

+	/* If orientation is changed, update the property. */
+	if (state->config_->orientation != requestedOrientation) {
+		{ // You can also make this a getter
+			GLibLocker lock(GST_OBJECT(object));
+			state->config_->orientation = state->config_-
>orientation;
+		}
+		g_object_notify(G_OBJECT(self), "orientation");
+	}

Personally, I would rather have the source push an image-orientation tag with
the missing operation needed to match the requested orientation. This this way
we won't need any code to offload the rest of the work to downstream elements.
Of course, you can't do that properly without also dealing with the sensor
rotation. And you can't do that if you merge this code as it will be impossible
to make something compatible.

> +
>  	/*
>  	 * Regardless if it has been modified, create clean caps and push the
>  	 * caps event. Downstream will decide if the caps are acceptable.
> @@ -936,6 +946,9 @@ 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_ORIENTATION:
> +		self->orientation = static_cast<GstVideoOrientationMethod>(g_value_get_enum(value));
> +		break;
>  	default:
>  		if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -955,6 +968,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_ORIENTATION:
> +		g_value_set_enum(value, static_cast<gint>(self->orientation));
> +		break;
>  	default:
>  		if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
>  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> @@ -1164,6 +1180,17 @@ 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_enum("orientation", "Orientation",
> +				 "Select the preferred image orientation.",

Not enough doc.

> +				 GST_TYPE_VIDEO_ORIENTATION_METHOD,
> +				 GST_VIDEO_ORIENTATION_IDENTITY,
> +				 (GParamFlags)(GST_PARAM_MUTABLE_READY
> +					       | G_PARAM_CONSTRUCT
> +					       | G_PARAM_READWRITE
> +					       | G_PARAM_STATIC_STRINGS));
> +	g_object_class_install_property(object_class, PROP_ORIENTATION, spec);
> +
>  	GstCameraControls::installProperties(object_class, PROP_LAST);
>  }
>

On Tue, Oct 14, 2025 at 08:53:04AM -0400, Nicolas Dufresne wrote:
> Le mardi 14 octobre 2025 à 12:45 +0530, Umang Jain a écrit :
> > From: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > 
> > Plumb the support for CameraConfiguration::orientation in libcamerasrc.
> > A new "orientation" property is introduced and mappings for
> > libcamera::Orientation <> GstVideoOrientationMethod are provided
> > with helpers.
> > 
> > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > Co-developed-by: Umang Jain <uajain@igalia.com>
> > Signed-off-by: Umang Jain <uajain@igalia.com>
> > ---
> > Changes in v5:
> > - patch scrubbing and cleanup
> > - If different orientation is returned than requested, update the
> >   property
> > - Minor string fixes, append appropriate tags
> > 
> > Link to v4: https://patchwork.libcamera.org/patch/23965/
> > 
> > Note: Checkstyle will complain on this patch on a hunk, but the existing format
> > is quite read-able IMO, hence ignored.
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 36 ++++++++++++++++++++++++++++
> >  src/gstreamer/gstlibcamera-utils.h   |  3 +++
> >  src/gstreamer/gstlibcamerasrc.cpp    | 27 +++++++++++++++++++++
> >  3 files changed, 66 insertions(+)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index bfb094c9..44050cbb 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -357,6 +357,42 @@ control_type_to_gtype(const ControlType &type)
> >  	return G_TYPE_INVALID;
> >  }
> >  
> > +static const struct {
> > +	Orientation orientation;
> > +	GstVideoOrientationMethod method;
> > +} orientation_map[]{
> > +	{ Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },
> > +	{ Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },
> > +	{ Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },
> > +	{ Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },
> > +	{ Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },
> > +	{ Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },
> > +	{ Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },
> > +	{ Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },
> > +};
> > +
> > +Orientation
> > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method)
> > +{
> > +	for (auto &b : orientation_map) {
> > +		if (b.method == method)
> > +			return b.orientation;
> > +	}
> > +
> > +	return Orientation::Rotate0;
> > +}
> > +
> > +GstVideoOrientationMethod
> > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)
> > +{
> > +	for (auto &a : orientation_map) {
> > +		if (a.orientation == orientation)
> > +			return a.method;
> > +	}
> > +
> > +	return GST_VIDEO_ORIENTATION_IDENTITY;
> > +}
> > +
> >  GstCaps *
> >  gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> >  {
> > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > index 35df56fb..4364811e 100644
> > --- a/src/gstreamer/gstlibcamera-utils.h
> > +++ b/src/gstreamer/gstlibcamera-utils.h
> > @@ -10,6 +10,7 @@
> >  
> >  #include <libcamera/camera_manager.h>
> >  #include <libcamera/controls.h>
> > +#include <libcamera/orientation.h>
> >  #include <libcamera/stream.h>
> >  
> >  #include <gst/gst.h>
> > @@ -32,6 +33,8 @@ libcamera::Rectangle gst_libcamera_gvalue_get_rectangle(const GValue *value);
> >  int gst_libcamera_set_structure_field(GstStructure *structure,
> >  				      const libcamera::ControlId *id,
> >  				      const libcamera::ControlValue &value);
> > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> >  
> >  #if !GST_CHECK_VERSION(1, 16, 0)
> >  static inline void gst_clear_event(GstEvent **event_ptr)
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 79a025a5..2d5f0bf5 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -146,6 +146,7 @@ struct _GstLibcameraSrc {
> >  	GstTask *task;
> >  
> >  	gchar *camera_name;
> > +	GstVideoOrientationMethod orientation;
> >  
> >  	std::atomic<GstEvent *> pending_eos;
> >  
> > @@ -157,6 +158,7 @@ struct _GstLibcameraSrc {
> >  enum {
> >  	PROP_0,
> >  	PROP_CAMERA_NAME,
> > +	PROP_ORIENTATION,
> 
> The subject is ambiguous, I read this as libcamera would be providing the
> orientation, and that this patch would simply translate it to a tag (image-
> orientation) and push that in the pipeline. But in short, you are looking to
> support sensor HFLIP and VFLIP, very rarely the other rotations are supported.
> 
> Please, update the subject and patch description so we can understand what this
> patch is about.

I assumed libcamera::orientation is well documented and the reader would
rather read that to gain more insight, since this is just a plumbing
patch. I can add a line or two to make more clearer.

> 
> >  	PROP_LAST
> >  };
> >  
> > @@ -616,6 +618,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >  		gst_libcamera_get_framerate_from_caps(caps, element_caps);
> >  	}
> >  
> > +	/* Set orientation. */
> > +	Orientation requestedOrientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> 
> Missing "GLibLocker lock(GST_OBJECT(object));" You can create a local get
> function that does that.
> 
> > +	state->config_->orientation = requestedOrientation;
> > +
> >  	/* Validate the configuration. */
> >  	CameraConfiguration::Status status = state->config_->validate();
> >  	if (status == CameraConfiguration::Invalid)
> > @@ -637,6 +643,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >  	gst_libcamera_clamp_and_set_frameduration(state->initControls_,
> >  						  state->cam_->controls(), element_caps);
> >  
> > +	/* If orientation is changed, update the property. */
> > +	if (state->config_->orientation != requestedOrientation)
> > +		g_object_set(G_OBJECT(self), "orientation", state->config_->orientation, nullptr);
> 
> Its always risky to call g_object_set() on self due to possible locking. This
> case seem to work, but required me to think harder. Perhaps use the usual
> method:
> 
> +	/* If orientation is changed, update the property. */
> +	if (state->config_->orientation != requestedOrientation) {
> +		{ // You can also make this a getter
> +			GLibLocker lock(GST_OBJECT(object));
> +			state->config_->orientation = state->config_-
> >orientation;
> +		}
> +		g_object_notify(G_OBJECT(self), "orientation");
> +	}
> 
> Personally, I would rather have the source push an image-orientation tag with
> the missing operation needed to match the requested orientation. This this way
> we won't need any code to offload the rest of the work to downstream elements.

I think there is no "missing" operation here because
CameraConfiguration::orientation would not apply anything if the
requested orientation cannot be satisfied and defaults to native image
orientation that the sensor will produce.

> Of course, you can't do that properly without also dealing with the sensor
> rotation. And you can't do that if you merge this code as it will be impossible
> to make something compatible.

So, I think you are asking for the "missing" operation to be computed (in order
to achieve the desired orientation) and push that down, via image-orientation tag
for downstream to compensate. I wonder why you say this is impossible if
this patch gets merged ? Rather, it should be built on top, no?

> 
> > +
> >  	/*
> >  	 * Regardless if it has been modified, create clean caps and push the
> >  	 * caps event. Downstream will decide if the caps are acceptable.
> > @@ -936,6 +946,9 @@ 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_ORIENTATION:
> > +		self->orientation = static_cast<GstVideoOrientationMethod>(g_value_get_enum(value));
> > +		break;
> >  	default:
> >  		if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))
> >  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > @@ -955,6 +968,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_ORIENTATION:
> > +		g_value_set_enum(value, static_cast<gint>(self->orientation));
> > +		break;
> >  	default:
> >  		if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))
> >  			G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);
> > @@ -1164,6 +1180,17 @@ 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_enum("orientation", "Orientation",
> > +				 "Select the preferred image orientation.",
> 
> Not enough doc.
> 
> > +				 GST_TYPE_VIDEO_ORIENTATION_METHOD,
> > +				 GST_VIDEO_ORIENTATION_IDENTITY,
> > +				 (GParamFlags)(GST_PARAM_MUTABLE_READY
> > +					       | G_PARAM_CONSTRUCT
> > +					       | G_PARAM_READWRITE
> > +					       | G_PARAM_STATIC_STRINGS));
> > +	g_object_class_install_property(object_class, PROP_ORIENTATION, spec);
> > +
> >  	GstCameraControls::installProperties(object_class, PROP_LAST);
> >  }
> >

Le mardi 14 octobre 2025 à 19:39 +0530, Umang Jain a écrit :
> On Tue, Oct 14, 2025 at 08:53:04AM -0400, Nicolas Dufresne wrote:
> > Le mardi 14 octobre 2025 à 12:45 +0530, Umang Jain a écrit :
> > > From: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > 
> > > 

[...]

> > >  
> > > @@ -157,6 +158,7 @@ struct _GstLibcameraSrc {
> > >  enum {
> > >  	PROP_0,
> > >  	PROP_CAMERA_NAME,
> > > +	PROP_ORIENTATION,
> > 
> > The subject is ambiguous, I read this as libcamera would be providing the
> > orientation, and that this patch would simply translate it to a tag (image-
> > orientation) and push that in the pipeline. But in short, you are looking to
> > support sensor HFLIP and VFLIP, very rarely the other rotations are supported.
> > 
> > Please, update the subject and patch description so we can understand what this
> > patch is about.
> 
> I assumed libcamera::orientation is well documented and the reader would
> rather read that to gain more insight, since this is just a plumbing
> patch. I can add a line or two to make more clearer.

This is the first source I'm aware that implements this operation. If you found
other sources doing so, let me know. This is normally found on filters and
display sink. A property that change its value from inside is also very uncommon
and must be documented thoroughly, GStreamer application developers are not use
to that.

Nicolas

Hi Umang,

Le mardi 14 octobre 2025 à 19:39 +0530, Umang Jain a écrit :
> > Personally, I would rather have the source push an image-orientation tag with
> > the missing operation needed to match the requested orientation. This this way
> > we won't need any code to offload the rest of the work to downstream elements.
> 
> I think there is no "missing" operation here because
> CameraConfiguration::orientation would not apply anything if the
> requested orientation cannot be satisfied and defaults to native image
> orientation that the sensor will produce.
> 
> > Of course, you can't do that properly without also dealing with the sensor
> > rotation. And you can't do that if you merge this code as it will be impossible
> > to make something compatible.
> 
> So, I think you are asking for the "missing" operation to be computed (in order
> to achieve the desired orientation) and push that down, via image-orientation tag
> for downstream to compensate. I wonder why you say this is impossible if
> this patch gets merged ? Rather, it should be built on top, no?

These are two different behaviour, which in my knowledge is an ABI break. I
never design toward a known ABI break.

Nicolas