[v4,1/1] gstreamer: Add support for Orientation

23965 diff mbox series

libcamera allows to control the images orientation through the
CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
control it. Parameter is mapped internally to libcamera::Orientation via
new gstlibcamera-utils functions:
- gst_video_orientation_to_libcamera_orientation
- libcamera_orientation_to_gst_video_orientation

Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
---
 src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
 src/gstreamer/gstlibcamera-utils.h   |  4 ++
 src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
 3 files changed, 86 insertions(+), 22 deletions(-)

Hi Giacomo,

There is one issue with the patch format. I think you have picked
up Jaslo's series (right thing to do) but, you have squashed his
commit+orientation patch into one. This is not right thing to do
and only makes reviews/merging almost impossible.

What is generally done in cases like this:
Step 1: Download the patch series which you want to base you work on
Step 2: apply to a local git tree
Step 3: Base your work on top of that tree with separate patches
	authored by you, build+test+send.

After Step 3:, you should only send out the patches authored by you
to the mailing list. And in the cover/patch comment, mention you have
based your work based on XYZ series (in this case Jaslo's
https://patchwork.libcamera.org/project/libcamera/list/?series=5311).

For Step 1 & 2, if it is unclear - You can simply download the patch
series by clicking on "series" on right hand side and apply locally
with `git am path_to_file.patch`. Alternatively, if you want to go
advanced simply use the `git-pw` utility
(https://github.com/getpatchwork/git-pw) which simplies this process
even further.

I took a cursory look at the orientation related LOC, and it has started
to look in the correct direction to me.

On Fri, Jul 25, 2025 at 01:30:46PM +0200, Giacomo Cappellini wrote:
> libcamera allows to control the images orientation through the
> CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
> parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
> control it. Parameter is mapped internally to libcamera::Orientation via
> new gstlibcamera-utils functions:
> - gst_video_orientation_to_libcamera_orientation
> - libcamera_orientation_to_gst_video_orientation
> 
> Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
>  src/gstreamer/gstlibcamera-utils.h   |  4 ++
>  src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
>  3 files changed, 86 insertions(+), 22 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index a548b0c1..15069f98 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -10,6 +10,9 @@
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/orientation.h>
> +
> +#include <gst/video/video.h>
>  
>  using namespace libcamera;
>  
> @@ -20,7 +23,7 @@ static struct {
>  	/* Compressed */
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::MJPEG },
>  
> -	/* Bayer formats, gstreamer only supports 8-bit */
> +	/* Bayer formats */
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SBGGR8 },
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SGBRG8 },
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SGRBG8 },
> @@ -317,20 +320,15 @@ bare_structure_from_format(const PixelFormat &format)
>  		return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
>  					 gst_video_format_to_string(gst_format), nullptr);
>  
> -	switch (format) {
> -	case formats::MJPEG:
> +	if (format == formats::MJPEG)
>  		return gst_structure_new_empty("image/jpeg");
>  
> -	case formats::SBGGR8:
> -	case formats::SGBRG8:
> -	case formats::SGRBG8:
> -	case formats::SRGGB8:
> -		return gst_structure_new("video/x-bayer", "format", G_TYPE_STRING,
> -					 bayer_format_to_string(format), nullptr);
> -
> -	default:
> +	const gchar *s = bayer_format_to_string(format);
> +	if (s)
> +		return gst_structure_new("video/x-bayer", "format",
> +					 G_TYPE_STRING, s, nullptr);
> +	else
>  		return nullptr;
> -	}
>  }
>  
>  GstCaps *
> @@ -659,3 +657,39 @@ gst_libcamera_get_camera_manager(int &ret)
>  
>  	return cm;
>  }
> +
> +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;
> +}
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 5f4e8a0f..bbbd33db 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>
> @@ -92,3 +93,6 @@ public:
>  private:
>  	GRecMutex *mutex_;
>  };
> +
> +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 3aca4eed..85add936 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -29,6 +29,7 @@
>  
>  #include <atomic>
>  #include <queue>
> +#include <sstream>
>  #include <tuple>
>  #include <utility>
>  #include <vector>
> @@ -38,6 +39,7 @@
>  #include <libcamera/control_ids.h>
>  
>  #include <gst/base/base.h>
> +#include <gst/video/video.h>
>  
>  #include "gstlibcamera-controls.h"
>  #include "gstlibcamera-utils.h"
> @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> +	GstVideoOrientationMethod orientation;
>  
>  	std::atomic<GstEvent *> pending_eos;
>  
> @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> +	PROP_ORIENTATION,
>  	PROP_LAST
>  };
>  
> @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
>  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
>  			G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,
>  					      gst_libcamera_src_child_proxy_init)
> -			GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> -						"libcamera Source"))
> +				GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> +							"libcamera Source"))
>  
>  #define TEMPLATE_CAPS GST_STATIC_CAPS("video/x-raw; image/jpeg; video/x-bayer")
>  
> @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()
>  	return 0;
>  }
>  
> -void
> -GstLibcameraSrcState::requestCompleted(Request *request)
> +void GstLibcameraSrcState::requestCompleted(Request *request)
>  {
>  	GST_DEBUG_OBJECT(src_, "buffers are ready");
>  
> @@ -616,9 +619,17 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  		gst_libcamera_get_framerate_from_caps(caps, element_caps);
>  	}
>  
> +	/* Set orientation control. */
> +	state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> +
>  	/* Validate the configuration. */
> -	if (state->config_->validate() == CameraConfiguration::Invalid)
> +	CameraConfiguration::Status status = state->config_->validate();
> +	if (status == CameraConfiguration::Invalid)
>  		return false;
> +	else if (status == CameraConfiguration::Adjusted)
> +		GST_ELEMENT_INFO(self, RESOURCE, SETTINGS,
> +				 ("Configuration was adjusted"),
> +				 ("CameraConfiguration::validate() returned CameraConfiguration::Adjusted"));
>  
>  	int ret = state->cam_->configure(state->config_.get());
>  	if (ret) {
> @@ -643,6 +654,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  		g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
>  		gst_libcamera_framerate_to_caps(caps, element_caps);
>  
> +		if (status == CameraConfiguration::Adjusted &&
> +		    !gst_pad_peer_query_accept_caps(srcpad, caps))
> +			return false;
> +
>  		if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
>  			return false;
>  	}
> @@ -730,7 +745,8 @@ gst_libcamera_src_task_run(gpointer user_data)
>  		if (gst_pad_check_reconfigure(srcpad)) {
>  			/* Check if the caps even need changing. */
>  			g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> -			if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> +			g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps);
> +			if (gst_caps_is_empty(peercaps)) {
>  				reconfigure = true;
>  				break;
>  			}
> @@ -926,6 +942,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 = (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);
> @@ -945,6 +964,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, (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);
> @@ -1148,12 +1170,16 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  
>  	GParamSpec *spec = g_param_spec_string("camera-name", "Camera Name",
>  					       "Select by name which camera to use.", nullptr,
> -					       (GParamFlags)(GST_PARAM_MUTABLE_READY
> -							     | G_PARAM_CONSTRUCT
> -							     | G_PARAM_READWRITE
> -							     | G_PARAM_STATIC_STRINGS));
> +					       (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
>  
> +	/* Register the orientation enum type. */
> +	spec = g_param_spec_enum("orientation", "Orientation",
> +				 "Select the orientation of the camera.",
> +				 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);
>  }
>  
> -- 
> 2.43.0
>

Hi,

Le vendredi 25 juillet 2025 à 13:30 +0200, Giacomo Cappellini a écrit :
> libcamera allows to control the images orientation through the
> CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
> parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
> control it. Parameter is mapped internally to libcamera::Orientation via
> new gstlibcamera-utils functions:
> - gst_video_orientation_to_libcamera_orientation
> - libcamera_orientation_to_gst_video_orientation
> 
> Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
>  src/gstreamer/gstlibcamera-utils.h   |  4 ++
>  src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
>  3 files changed, 86 insertions(+), 22 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index a548b0c1..15069f98 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -10,6 +10,9 @@
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/orientation.h>
> +
> +#include <gst/video/video.h>
>  
>  using namespace libcamera;
>  
> @@ -20,7 +23,7 @@ static struct {
>  	/* Compressed */
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::MJPEG },
>  
> -	/* Bayer formats, gstreamer only supports 8-bit */
> +	/* Bayer formats */

Unrelated changes, but valid, what about giving it its own commit ?

>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SBGGR8 },
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SGBRG8 },
>  	{ GST_VIDEO_FORMAT_ENCODED, formats::SGRBG8 },
> @@ -317,20 +320,15 @@ bare_structure_from_format(const PixelFormat &format)
>  		return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
>  					 gst_video_format_to_string(gst_format), nullptr);
>  
> -	switch (format) {
> -	case formats::MJPEG:
> +	if (format == formats::MJPEG)
>  		return gst_structure_new_empty("image/jpeg");
>  
> -	case formats::SBGGR8:
> -	case formats::SGBRG8:
> -	case formats::SGRBG8:
> -	case formats::SRGGB8:
> -		return gst_structure_new("video/x-bayer", "format", G_TYPE_STRING,
> -					 bayer_format_to_string(format), nullptr);
> -
> -	default:
> +	const gchar *s = bayer_format_to_string(format);
> +	if (s)
> +		return gst_structure_new("video/x-bayer", "format",
> +					 G_TYPE_STRING, s, nullptr);
> +	else
>  		return nullptr;

Same, please dedicate a patch for that.

> -	}
>  }
>  
>  GstCaps *
> @@ -659,3 +657,39 @@ gst_libcamera_get_camera_manager(int &ret)
>  
>  	return cm;
>  }
> +
> +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;
> +}
> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> index 5f4e8a0f..bbbd33db 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>
> @@ -92,3 +93,6 @@ public:
>  private:
>  	GRecMutex *mutex_;
>  };
> +
> +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 3aca4eed..85add936 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -29,6 +29,7 @@
>  
>  #include <atomic>
>  #include <queue>
> +#include <sstream>
>  #include <tuple>
>  #include <utility>
>  #include <vector>
> @@ -38,6 +39,7 @@
>  #include <libcamera/control_ids.h>
>  
>  #include <gst/base/base.h>
> +#include <gst/video/video.h>
>  
>  #include "gstlibcamera-controls.h"
>  #include "gstlibcamera-utils.h"
> @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {
>  	GstTask *task;
>  
>  	gchar *camera_name;
> +	GstVideoOrientationMethod orientation;
>  
>  	std::atomic<GstEvent *> pending_eos;
>  
> @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {
>  enum {
>  	PROP_0,
>  	PROP_CAMERA_NAME,
> +	PROP_ORIENTATION,
>  	PROP_LAST
>  };
>  
> @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
>  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
>  			G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,
>  					      gst_libcamera_src_child_proxy_init)
> -			GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> -						"libcamera Source"))
> +				GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> +							"libcamera Source"))

Not sure why this change, it looked better before.

>  
>  #define TEMPLATE_CAPS GST_STATIC_CAPS("video/x-raw; image/jpeg; video/x-bayer")
>  
> @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()
>  	return 0;
>  }
>  
> -void
> -GstLibcameraSrcState::requestCompleted(Request *request)
> +void GstLibcameraSrcState::requestCompleted(Request *request)

Was GStreamer style, but at the same time, don't do style changes mixed with
your functional changes.

>  {
>  	GST_DEBUG_OBJECT(src_, "buffers are ready");
>  
> @@ -616,9 +619,17 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  		gst_libcamera_get_framerate_from_caps(caps, element_caps);
>  	}
>  
> +	/* Set orientation control. */
> +	state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> +
>  	/* Validate the configuration. */
> -	if (state->config_->validate() == CameraConfiguration::Invalid)
> +	CameraConfiguration::Status status = state->config_->validate();
> +	if (status == CameraConfiguration::Invalid)
>  		return false;
> +	else if (status == CameraConfiguration::Adjusted)
> +		GST_ELEMENT_INFO(self, RESOURCE, SETTINGS,
> +				 ("Configuration was adjusted"),
> +				 ("CameraConfiguration::validate() returned CameraConfiguration::Adjusted"));

Not sure this is worth a message to the application using the bus.

>  
>  	int ret = state->cam_->configure(state->config_.get());
>  	if (ret) {
> @@ -643,6 +654,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
>  		g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
>  		gst_libcamera_framerate_to_caps(caps, element_caps);
>  
> +		if (status == CameraConfiguration::Adjusted &&
> +		    !gst_pad_peer_query_accept_caps(srcpad, caps))
> +			return false;

Fair enough, this could though be done as its own patch, since it is valid
without the orientation. In fact, orientation is not part of the caps.

> +
>  		if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
>  			return false;
>  	}
> @@ -730,7 +745,8 @@ gst_libcamera_src_task_run(gpointer user_data)
>  		if (gst_pad_check_reconfigure(srcpad)) {
>  			/* Check if the caps even need changing. */
>  			g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> -			if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> +			g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps);
> +			if (gst_caps_is_empty(peercaps)) {

This patch I've seen as its own in some other submission, please drop.

>  				reconfigure = true;
>  				break;
>  			}
> @@ -926,6 +942,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 = (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);
> @@ -945,6 +964,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, (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);
> @@ -1148,12 +1170,16 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
>  
>  	GParamSpec *spec = g_param_spec_string("camera-name", "Camera Name",
>  					       "Select by name which camera to use.", nullptr,
> -					       (GParamFlags)(GST_PARAM_MUTABLE_READY
> -							     | G_PARAM_CONSTRUCT
> -							     | G_PARAM_READWRITE
> -							     | G_PARAM_STATIC_STRINGS));
> +					       (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));

Looked fine before, and was accepted as such.

>  	g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
>  
> +	/* Register the orientation enum type. */
> +	spec = g_param_spec_enum("orientation", "Orientation",

Is the name "orientation" proper ? In the wilderness of GStremer, I see rotate-
method (glimagesink), video-direction (videoflip), I'd pick one of these.

> +				 "Select the orientation of the camera.",
> +				 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);
>  }
>  

cheers,
Nicolas

Hi Umang Jain,

thank you for the detailed instructions and sorry for the error, this
is my first time doing dealing with this operation and with patchwork
in general. I made another mistake and I'll send V5 asap as
instructed. I'm sorry for that.

Cheers,

G.C.

Il giorno lun 28 lug 2025 alle ore 06:39 Umang Jain
<uajain@igalia.com> ha scritto:
>
> Hi Giacomo,
>
> There is one issue with the patch format. I think you have picked
> up Jaslo's series (right thing to do) but, you have squashed his
> commit+orientation patch into one. This is not right thing to do
> and only makes reviews/merging almost impossible.
>
> What is generally done in cases like this:
> Step 1: Download the patch series which you want to base you work on
> Step 2: apply to a local git tree
> Step 3: Base your work on top of that tree with separate patches
>         authored by you, build+test+send.
>
> After Step 3:, you should only send out the patches authored by you
> to the mailing list. And in the cover/patch comment, mention you have
> based your work based on XYZ series (in this case Jaslo's
> https://patchwork.libcamera.org/project/libcamera/list/?series=5311).
>
> For Step 1 & 2, if it is unclear - You can simply download the patch
> series by clicking on "series" on right hand side and apply locally
> with `git am path_to_file.patch`. Alternatively, if you want to go
> advanced simply use the `git-pw` utility
> (https://github.com/getpatchwork/git-pw) which simplies this process
> even further.
>
> I took a cursory look at the orientation related LOC, and it has started
> to look in the correct direction to me.
>
> On Fri, Jul 25, 2025 at 01:30:46PM +0200, Giacomo Cappellini wrote:
> > libcamera allows to control the images orientation through the
> > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
> > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
> > control it. Parameter is mapped internally to libcamera::Orientation via
> > new gstlibcamera-utils functions:
> > - gst_video_orientation_to_libcamera_orientation
> > - libcamera_orientation_to_gst_video_orientation
> >
> > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
> >  src/gstreamer/gstlibcamera-utils.h   |  4 ++
> >  src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
> >  3 files changed, 86 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index a548b0c1..15069f98 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -10,6 +10,9 @@
> >
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/formats.h>
> > +#include <libcamera/orientation.h>
> > +
> > +#include <gst/video/video.h>
> >
> >  using namespace libcamera;
> >
> > @@ -20,7 +23,7 @@ static struct {
> >       /* Compressed */
> >       { GST_VIDEO_FORMAT_ENCODED, formats::MJPEG },
> >
> > -     /* Bayer formats, gstreamer only supports 8-bit */
> > +     /* Bayer formats */
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SBGGR8 },
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SGBRG8 },
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SGRBG8 },
> > @@ -317,20 +320,15 @@ bare_structure_from_format(const PixelFormat &format)
> >               return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
> >                                        gst_video_format_to_string(gst_format), nullptr);
> >
> > -     switch (format) {
> > -     case formats::MJPEG:
> > +     if (format == formats::MJPEG)
> >               return gst_structure_new_empty("image/jpeg");
> >
> > -     case formats::SBGGR8:
> > -     case formats::SGBRG8:
> > -     case formats::SGRBG8:
> > -     case formats::SRGGB8:
> > -             return gst_structure_new("video/x-bayer", "format", G_TYPE_STRING,
> > -                                      bayer_format_to_string(format), nullptr);
> > -
> > -     default:
> > +     const gchar *s = bayer_format_to_string(format);
> > +     if (s)
> > +             return gst_structure_new("video/x-bayer", "format",
> > +                                      G_TYPE_STRING, s, nullptr);
> > +     else
> >               return nullptr;
> > -     }
> >  }
> >
> >  GstCaps *
> > @@ -659,3 +657,39 @@ gst_libcamera_get_camera_manager(int &ret)
> >
> >       return cm;
> >  }
> > +
> > +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;
> > +}
> > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > index 5f4e8a0f..bbbd33db 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>
> > @@ -92,3 +93,6 @@ public:
> >  private:
> >       GRecMutex *mutex_;
> >  };
> > +
> > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 3aca4eed..85add936 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -29,6 +29,7 @@
> >
> >  #include <atomic>
> >  #include <queue>
> > +#include <sstream>
> >  #include <tuple>
> >  #include <utility>
> >  #include <vector>
> > @@ -38,6 +39,7 @@
> >  #include <libcamera/control_ids.h>
> >
> >  #include <gst/base/base.h>
> > +#include <gst/video/video.h>
> >
> >  #include "gstlibcamera-controls.h"
> >  #include "gstlibcamera-utils.h"
> > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {
> >       GstTask *task;
> >
> >       gchar *camera_name;
> > +     GstVideoOrientationMethod orientation;
> >
> >       std::atomic<GstEvent *> pending_eos;
> >
> > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {
> >  enum {
> >       PROP_0,
> >       PROP_CAMERA_NAME,
> > +     PROP_ORIENTATION,
> >       PROP_LAST
> >  };
> >
> > @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
> >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> >                       G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,
> >                                             gst_libcamera_src_child_proxy_init)
> > -                     GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > -                                             "libcamera Source"))
> > +                             GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > +                                                     "libcamera Source"))
> >
> >  #define TEMPLATE_CAPS GST_STATIC_CAPS("video/x-raw; image/jpeg; video/x-bayer")
> >
> > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()
> >       return 0;
> >  }
> >
> > -void
> > -GstLibcameraSrcState::requestCompleted(Request *request)
> > +void GstLibcameraSrcState::requestCompleted(Request *request)
> >  {
> >       GST_DEBUG_OBJECT(src_, "buffers are ready");
> >
> > @@ -616,9 +619,17 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >               gst_libcamera_get_framerate_from_caps(caps, element_caps);
> >       }
> >
> > +     /* Set orientation control. */
> > +     state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> > +
> >       /* Validate the configuration. */
> > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > +     CameraConfiguration::Status status = state->config_->validate();
> > +     if (status == CameraConfiguration::Invalid)
> >               return false;
> > +     else if (status == CameraConfiguration::Adjusted)
> > +             GST_ELEMENT_INFO(self, RESOURCE, SETTINGS,
> > +                              ("Configuration was adjusted"),
> > +                              ("CameraConfiguration::validate() returned CameraConfiguration::Adjusted"));
> >
> >       int ret = state->cam_->configure(state->config_.get());
> >       if (ret) {
> > @@ -643,6 +654,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >               g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
> >               gst_libcamera_framerate_to_caps(caps, element_caps);
> >
> > +             if (status == CameraConfiguration::Adjusted &&
> > +                 !gst_pad_peer_query_accept_caps(srcpad, caps))
> > +                     return false;
> > +
> >               if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
> >                       return false;
> >       }
> > @@ -730,7 +745,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> >               if (gst_pad_check_reconfigure(srcpad)) {
> >                       /* Check if the caps even need changing. */
> >                       g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> > -                     if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> > +                     g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps);
> > +                     if (gst_caps_is_empty(peercaps)) {
> >                               reconfigure = true;
> >                               break;
> >                       }
> > @@ -926,6 +942,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 = (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);
> > @@ -945,6 +964,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, (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);
> > @@ -1148,12 +1170,16 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >
> >       GParamSpec *spec = g_param_spec_string("camera-name", "Camera Name",
> >                                              "Select by name which camera to use.", nullptr,
> > -                                            (GParamFlags)(GST_PARAM_MUTABLE_READY
> > -                                                          | G_PARAM_CONSTRUCT
> > -                                                          | G_PARAM_READWRITE
> > -                                                          | G_PARAM_STATIC_STRINGS));
> > +                                            (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> >
> > +     /* Register the orientation enum type. */
> > +     spec = g_param_spec_enum("orientation", "Orientation",
> > +                              "Select the orientation of the camera.",
> > +                              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);
> >  }
> >
> > --
> > 2.43.0
> >

Hi Nicolas,

as Umang Jain pointed out in previous mail I did the "send your patch
on top of the other series" wrong, so you're been commenting on code
that if for the most part of another patch (please check the cover
letter). I'll comment only on the point relevant to my patch and send
renamed property with V5.

Il giorno lun 28 lug 2025 alle ore 23:27 Nicolas Dufresne
<nicolas@ndufresne.ca> ha scritto:
>
> Hi,
>
> Le vendredi 25 juillet 2025 à 13:30 +0200, Giacomo Cappellini a écrit :
> > libcamera allows to control the images orientation through the
> > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
> > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
> > control it. Parameter is mapped internally to libcamera::Orientation via
> > new gstlibcamera-utils functions:
> > - gst_video_orientation_to_libcamera_orientation
> > - libcamera_orientation_to_gst_video_orientation
> >
> > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
> >  src/gstreamer/gstlibcamera-utils.h   |  4 ++
> >  src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
> >  3 files changed, 86 insertions(+), 22 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index a548b0c1..15069f98 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -10,6 +10,9 @@
> >
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/formats.h>
> > +#include <libcamera/orientation.h>
> > +
> > +#include <gst/video/video.h>
> >
> >  using namespace libcamera;
> >
> > @@ -20,7 +23,7 @@ static struct {
> >       /* Compressed */
> >       { GST_VIDEO_FORMAT_ENCODED, formats::MJPEG },
> >
> > -     /* Bayer formats, gstreamer only supports 8-bit */
> > +     /* Bayer formats */
>
> Unrelated changes, but valid, what about giving it its own commit ?
>
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SBGGR8 },
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SGBRG8 },
> >       { GST_VIDEO_FORMAT_ENCODED, formats::SGRBG8 },
> > @@ -317,20 +320,15 @@ bare_structure_from_format(const PixelFormat &format)
> >               return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
> >                                        gst_video_format_to_string(gst_format), nullptr);
> >
> > -     switch (format) {
> > -     case formats::MJPEG:
> > +     if (format == formats::MJPEG)
> >               return gst_structure_new_empty("image/jpeg");
> >
> > -     case formats::SBGGR8:
> > -     case formats::SGBRG8:
> > -     case formats::SGRBG8:
> > -     case formats::SRGGB8:
> > -             return gst_structure_new("video/x-bayer", "format", G_TYPE_STRING,
> > -                                      bayer_format_to_string(format), nullptr);
> > -
> > -     default:
> > +     const gchar *s = bayer_format_to_string(format);
> > +     if (s)
> > +             return gst_structure_new("video/x-bayer", "format",
> > +                                      G_TYPE_STRING, s, nullptr);
> > +     else
> >               return nullptr;
>
> Same, please dedicate a patch for that.
>
> > -     }
> >  }
> >
> >  GstCaps *
> > @@ -659,3 +657,39 @@ gst_libcamera_get_camera_manager(int &ret)
> >
> >       return cm;
> >  }
> > +
> > +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;
> > +}
> > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > index 5f4e8a0f..bbbd33db 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>
> > @@ -92,3 +93,6 @@ public:
> >  private:
> >       GRecMutex *mutex_;
> >  };
> > +
> > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 3aca4eed..85add936 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -29,6 +29,7 @@
> >
> >  #include <atomic>
> >  #include <queue>
> > +#include <sstream>
> >  #include <tuple>
> >  #include <utility>
> >  #include <vector>
> > @@ -38,6 +39,7 @@
> >  #include <libcamera/control_ids.h>
> >
> >  #include <gst/base/base.h>
> > +#include <gst/video/video.h>
> >
> >  #include "gstlibcamera-controls.h"
> >  #include "gstlibcamera-utils.h"
> > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {
> >       GstTask *task;
> >
> >       gchar *camera_name;
> > +     GstVideoOrientationMethod orientation;
> >
> >       std::atomic<GstEvent *> pending_eos;
> >
> > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {
> >  enum {
> >       PROP_0,
> >       PROP_CAMERA_NAME,
> > +     PROP_ORIENTATION,
> >       PROP_LAST
> >  };
> >
> > @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
> >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> >                       G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,
> >                                             gst_libcamera_src_child_proxy_init)
> > -                     GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > -                                             "libcamera Source"))
> > +                             GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > +                                                     "libcamera Source"))
>
> Not sure why this change, it looked better before.
>
> >
> >  #define TEMPLATE_CAPS GST_STATIC_CAPS("video/x-raw; image/jpeg; video/x-bayer")
> >
> > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()
> >       return 0;
> >  }
> >
> > -void
> > -GstLibcameraSrcState::requestCompleted(Request *request)
> > +void GstLibcameraSrcState::requestCompleted(Request *request)
>
> Was GStreamer style, but at the same time, don't do style changes mixed with
> your functional changes.
>
> >  {
> >       GST_DEBUG_OBJECT(src_, "buffers are ready");
> >
> > @@ -616,9 +619,17 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >               gst_libcamera_get_framerate_from_caps(caps, element_caps);
> >       }
> >
> > +     /* Set orientation control. */
> > +     state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> > +
> >       /* Validate the configuration. */
> > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > +     CameraConfiguration::Status status = state->config_->validate();
> > +     if (status == CameraConfiguration::Invalid)
> >               return false;
> > +     else if (status == CameraConfiguration::Adjusted)
> > +             GST_ELEMENT_INFO(self, RESOURCE, SETTINGS,
> > +                              ("Configuration was adjusted"),
> > +                              ("CameraConfiguration::validate() returned CameraConfiguration::Adjusted"));
>
> Not sure this is worth a message to the application using the bus.
>
> >
> >       int ret = state->cam_->configure(state->config_.get());
> >       if (ret) {
> > @@ -643,6 +654,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> >               g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
> >               gst_libcamera_framerate_to_caps(caps, element_caps);
> >
> > +             if (status == CameraConfiguration::Adjusted &&
> > +                 !gst_pad_peer_query_accept_caps(srcpad, caps))
> > +                     return false;
>
> Fair enough, this could though be done as its own patch, since it is valid
> without the orientation. In fact, orientation is not part of the caps.
>
> > +
> >               if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
> >                       return false;
> >       }
> > @@ -730,7 +745,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> >               if (gst_pad_check_reconfigure(srcpad)) {
> >                       /* Check if the caps even need changing. */
> >                       g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> > -                     if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> > +                     g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps);
> > +                     if (gst_caps_is_empty(peercaps)) {
>
> This patch I've seen as its own in some other submission, please drop.
>
> >                               reconfigure = true;
> >                               break;
> >                       }
> > @@ -926,6 +942,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 = (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);
> > @@ -945,6 +964,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, (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);
> > @@ -1148,12 +1170,16 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> >
> >       GParamSpec *spec = g_param_spec_string("camera-name", "Camera Name",
> >                                              "Select by name which camera to use.", nullptr,
> > -                                            (GParamFlags)(GST_PARAM_MUTABLE_READY
> > -                                                          | G_PARAM_CONSTRUCT
> > -                                                          | G_PARAM_READWRITE
> > -                                                          | G_PARAM_STATIC_STRINGS));
> > +                                            (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
>
> Looked fine before, and was accepted as such.
>
> >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> >
> > +     /* Register the orientation enum type. */
> > +     spec = g_param_spec_enum("orientation", "Orientation",
>
> Is the name "orientation" proper ? In the wilderness of GStremer, I see rotate-
> method (glimagesink), video-direction (videoflip), I'd pick one of these.
>
> > +                              "Select the orientation of the camera.",
> > +                              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);
> >  }
> >

I've been naming it orientation to discriminate it from rotation in
libcamera terms, as GStreamer (as you said) has no clear property name
for that.
If you agree on following one of the existing GStreamer property names
I'd vote for video-direction one of the videoflip element? Would you
agree?

> cheers,
> Nicolas

Best Regards,
G.C.

Hi Giaco,

On Tue, Jul 29, 2025 at 12:15:34AM +0200, Giacomo Cappellini wrote:
> Hi Nicolas,
> 
> as Umang Jain pointed out in previous mail I did the "send your patch
> on top of the other series" wrong, so you're been commenting on code
> that if for the most part of another patch (please check the cover
> letter). I'll comment only on the point relevant to my patch and send
> renamed property with V5.

Do you plan to send v5? Hopefully rebased over the latest master.

> 
> Il giorno lun 28 lug 2025 alle ore 23:27 Nicolas Dufresne
> <nicolas@ndufresne.ca> ha scritto:
> >
> > Hi,
> >
> > Le vendredi 25 juillet 2025 à 13:30 +0200, Giacomo Cappellini a écrit :
> > > libcamera allows to control the images orientation through the
> > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY
> > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to
> > > control it. Parameter is mapped internally to libcamera::Orientation via
> > > new gstlibcamera-utils functions:
> > > - gst_video_orientation_to_libcamera_orientation
> > > - libcamera_orientation_to_gst_video_orientation
> > >
> > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > ---
> > >  src/gstreamer/gstlibcamera-utils.cpp | 58 ++++++++++++++++++++++------
> > >  src/gstreamer/gstlibcamera-utils.h   |  4 ++
> > >  src/gstreamer/gstlibcamerasrc.cpp    | 46 +++++++++++++++++-----
> > >  3 files changed, 86 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > index a548b0c1..15069f98 100644
> > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > @@ -10,6 +10,9 @@
> > >
> > >  #include <libcamera/control_ids.h>
> > >  #include <libcamera/formats.h>
> > > +#include <libcamera/orientation.h>
> > > +
> > > +#include <gst/video/video.h>
> > >
> > >  using namespace libcamera;
> > >
> > > @@ -20,7 +23,7 @@ static struct {
> > >       /* Compressed */
> > >       { GST_VIDEO_FORMAT_ENCODED, formats::MJPEG },
> > >
> > > -     /* Bayer formats, gstreamer only supports 8-bit */
> > > +     /* Bayer formats */
> >
> > Unrelated changes, but valid, what about giving it its own commit ?
> >
> > >       { GST_VIDEO_FORMAT_ENCODED, formats::SBGGR8 },
> > >       { GST_VIDEO_FORMAT_ENCODED, formats::SGBRG8 },
> > >       { GST_VIDEO_FORMAT_ENCODED, formats::SGRBG8 },
> > > @@ -317,20 +320,15 @@ bare_structure_from_format(const PixelFormat &format)
> > >               return gst_structure_new("video/x-raw", "format", G_TYPE_STRING,
> > >                                        gst_video_format_to_string(gst_format), nullptr);
> > >
> > > -     switch (format) {
> > > -     case formats::MJPEG:
> > > +     if (format == formats::MJPEG)
> > >               return gst_structure_new_empty("image/jpeg");
> > >
> > > -     case formats::SBGGR8:
> > > -     case formats::SGBRG8:
> > > -     case formats::SGRBG8:
> > > -     case formats::SRGGB8:
> > > -             return gst_structure_new("video/x-bayer", "format", G_TYPE_STRING,
> > > -                                      bayer_format_to_string(format), nullptr);
> > > -
> > > -     default:
> > > +     const gchar *s = bayer_format_to_string(format);
> > > +     if (s)
> > > +             return gst_structure_new("video/x-bayer", "format",
> > > +                                      G_TYPE_STRING, s, nullptr);
> > > +     else
> > >               return nullptr;
> >
> > Same, please dedicate a patch for that.
> >
> > > -     }
> > >  }
> > >
> > >  GstCaps *
> > > @@ -659,3 +657,39 @@ gst_libcamera_get_camera_manager(int &ret)
> > >
> > >       return cm;
> > >  }
> > > +
> > > +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;
> > > +}
> > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h
> > > index 5f4e8a0f..bbbd33db 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>
> > > @@ -92,3 +93,6 @@ public:
> > >  private:
> > >       GRecMutex *mutex_;
> > >  };
> > > +
> > > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);
> > > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 3aca4eed..85add936 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -29,6 +29,7 @@
> > >
> > >  #include <atomic>
> > >  #include <queue>
> > > +#include <sstream>
> > >  #include <tuple>
> > >  #include <utility>
> > >  #include <vector>
> > > @@ -38,6 +39,7 @@
> > >  #include <libcamera/control_ids.h>
> > >
> > >  #include <gst/base/base.h>
> > > +#include <gst/video/video.h>
> > >
> > >  #include "gstlibcamera-controls.h"
> > >  #include "gstlibcamera-utils.h"
> > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {
> > >       GstTask *task;
> > >
> > >       gchar *camera_name;
> > > +     GstVideoOrientationMethod orientation;
> > >
> > >       std::atomic<GstEvent *> pending_eos;
> > >
> > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {
> > >  enum {
> > >       PROP_0,
> > >       PROP_CAMERA_NAME,
> > > +     PROP_ORIENTATION,
> > >       PROP_LAST
> > >  };
> > >
> > > @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,
> > >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,
> > >                       G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,
> > >                                             gst_libcamera_src_child_proxy_init)
> > > -                     GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > > -                                             "libcamera Source"))
> > > +                             GST_DEBUG_CATEGORY_INIT(source_debug, "libcamerasrc", 0,
> > > +                                                     "libcamera Source"))
> >
> > Not sure why this change, it looked better before.
> >
> > >
> > >  #define TEMPLATE_CAPS GST_STATIC_CAPS("video/x-raw; image/jpeg; video/x-bayer")
> > >
> > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()
> > >       return 0;
> > >  }
> > >
> > > -void
> > > -GstLibcameraSrcState::requestCompleted(Request *request)
> > > +void GstLibcameraSrcState::requestCompleted(Request *request)
> >
> > Was GStreamer style, but at the same time, don't do style changes mixed with
> > your functional changes.
> >
> > >  {
> > >       GST_DEBUG_OBJECT(src_, "buffers are ready");
> > >
> > > @@ -616,9 +619,17 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> > >               gst_libcamera_get_framerate_from_caps(caps, element_caps);
> > >       }
> > >
> > > +     /* Set orientation control. */
> > > +     state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);
> > > +
> > >       /* Validate the configuration. */
> > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > +     CameraConfiguration::Status status = state->config_->validate();
> > > +     if (status == CameraConfiguration::Invalid)
> > >               return false;
> > > +     else if (status == CameraConfiguration::Adjusted)
> > > +             GST_ELEMENT_INFO(self, RESOURCE, SETTINGS,
> > > +                              ("Configuration was adjusted"),
> > > +                              ("CameraConfiguration::validate() returned CameraConfiguration::Adjusted"));
> >
> > Not sure this is worth a message to the application using the bus.
> >
> > >
> > >       int ret = state->cam_->configure(state->config_.get());
> > >       if (ret) {
> > > @@ -643,6 +654,10 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)
> > >               g_autoptr(GstCaps) caps = gst_libcamera_stream_configuration_to_caps(stream_cfg, transfer[i]);
> > >               gst_libcamera_framerate_to_caps(caps, element_caps);
> > >
> > > +             if (status == CameraConfiguration::Adjusted &&
> > > +                 !gst_pad_peer_query_accept_caps(srcpad, caps))
> > > +                     return false;
> >
> > Fair enough, this could though be done as its own patch, since it is valid
> > without the orientation. In fact, orientation is not part of the caps.
> >
> > > +
> > >               if (!gst_pad_push_event(srcpad, gst_event_new_caps(caps)))
> > >                       return false;
> > >       }
> > > @@ -730,7 +745,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >               if (gst_pad_check_reconfigure(srcpad)) {
> > >                       /* Check if the caps even need changing. */
> > >                       g_autoptr(GstCaps) caps = gst_pad_get_current_caps(srcpad);
> > > -                     if (!gst_pad_peer_query_accept_caps(srcpad, caps)) {
> > > +                     g_autoptr(GstCaps) peercaps = gst_pad_peer_query_caps(srcpad, caps);
> > > +                     if (gst_caps_is_empty(peercaps)) {
> >
> > This patch I've seen as its own in some other submission, please drop.
> >
> > >                               reconfigure = true;
> > >                               break;
> > >                       }
> > > @@ -926,6 +942,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 = (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);
> > > @@ -945,6 +964,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, (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);
> > > @@ -1148,12 +1170,16 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)
> > >
> > >       GParamSpec *spec = g_param_spec_string("camera-name", "Camera Name",
> > >                                              "Select by name which camera to use.", nullptr,
> > > -                                            (GParamFlags)(GST_PARAM_MUTABLE_READY
> > > -                                                          | G_PARAM_CONSTRUCT
> > > -                                                          | G_PARAM_READWRITE
> > > -                                                          | G_PARAM_STATIC_STRINGS));
> > > +                                            (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
> >
> > Looked fine before, and was accepted as such.
> >
> > >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);
> > >
> > > +     /* Register the orientation enum type. */
> > > +     spec = g_param_spec_enum("orientation", "Orientation",
> >
> > Is the name "orientation" proper ? In the wilderness of GStremer, I see rotate-
> > method (glimagesink), video-direction (videoflip), I'd pick one of these.
> >
> > > +                              "Select the orientation of the camera.",
> > > +                              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);
> > >  }
> > >
> 
> I've been naming it orientation to discriminate it from rotation in
> libcamera terms, as GStreamer (as you said) has no clear property name
> for that.
> If you agree on following one of the existing GStreamer property names
> I'd vote for video-direction one of the videoflip element? Would you
> agree?
> 
> > cheers,
> > Nicolas
> 
> Best Regards,
> G.C.