[v3] gstreamer: Add support for Orientation

23902 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

Update CameraConfiguration::Adjusted case in negotiation to warn about
changes in StreamConfiguration and SensorConfiguration, as well as
the new Orientation parameter.

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

On Wed, Jul 23, 2025 at 04:07:55PM +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
> 
> Update CameraConfiguration::Adjusted case in negotiation to warn about
> changes in StreamConfiguration and SensorConfiguration, as well as
> the new Orientation parameter.
> 
> Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> ---
>  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
>  src/gstreamer/gstlibcamera-utils.h   |   4 +
>  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
>  3 files changed, 166 insertions(+), 9 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index a548b0c1..95d3813e 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;
>  
> @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> +
> +	/* Save original configuration for comparison after validation */
> +	std::vector<StreamConfiguration> orig_stream_cfgs;
> +	for (gsize i = 0; i < state->config_->size(); i++)
> +		orig_stream_cfgs.push_back(state->config_->at(i));
> +	std::optional<SensorConfiguration> orig_sensor_cfg = state->config_->sensorConfig;
> +	Orientation orig_orientation = state->config_->orientation;
> +
>  	/* Validate the configuration. */
> -	if (state->config_->validate() == CameraConfiguration::Invalid)
> +	switch (state->config_->validate()) {
> +	case CameraConfiguration::Valid:
> +		GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> +		break;
> +	case CameraConfiguration::Adjusted: {
> +		bool warned = false;
> +		// Warn if number of StreamConfigurations changed
> +		if (orig_stream_cfgs.size() != state->config_->size()) {
> +			GST_WARNING_OBJECT(self, "Number of StreamConfiguration elements changed: requested=%zu, actual=%zu",
> +					   orig_stream_cfgs.size(), state->config_->size());
> +			warned = true;
> +		}
> +		// Warn about changes in each StreamConfiguration
> +		// TODO implement diffing in StreamConfiguration
> +		for (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {
> +			if (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {
> +				GST_WARNING_OBJECT(self, "StreamConfiguration %zu changed: %s -> %s",
> +						   i, orig_stream_cfgs[i].toString().c_str(),
> +						   state->config_->at(i).toString().c_str());
> +				warned = true;
> +			}
> +		}
> +		// Warn about SensorConfiguration changes
> +		// TODO implement diffing in SensorConfiguration
> +		if (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {
> +			const SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> +			const SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;
> +			bool sensor_changed = false;
> +			std::ostringstream diff;
> +			if ((orig == nullptr) != (curr == nullptr)) {
> +				diff << "SensorConfiguration presence changed: "
> +				     << (orig ? "was present" : "was absent")
> +				     << " -> "
> +				     << (curr ? "present" : "absent");
> +				sensor_changed = true;
> +			} else if (orig && curr) {
> +				if (orig->bitDepth != curr->bitDepth) {
> +					diff << "bitDepth: " << orig->bitDepth << " -> " << curr->bitDepth << "; ";
> +					sensor_changed = true;
> +				}
> +				if (orig->analogCrop != curr->analogCrop) {
> +					diff << "analogCrop: " << orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << "; ";
> +					sensor_changed = true;
> +				}
> +				if (orig->binning.binX != curr->binning.binX ||
> +				    orig->binning.binY != curr->binning.binY) {
> +					diff << "binning: (" << orig->binning.binX << "," << orig->binning.binY << ") -> ("
> +					     << curr->binning.binX << "," << curr->binning.binY << "); ";
> +					sensor_changed = true;
> +				}
> +				if (orig->skipping.xOddInc != curr->skipping.xOddInc ||
> +				    orig->skipping.xEvenInc != curr->skipping.xEvenInc ||
> +				    orig->skipping.yOddInc != curr->skipping.yOddInc ||
> +				    orig->skipping.yEvenInc != curr->skipping.yEvenInc) {
> +					diff << "skipping: ("
> +					     << orig->skipping.xOddInc << "," << orig->skipping.xEvenInc << ","
> +					     << orig->skipping.yOddInc << "," << orig->skipping.yEvenInc << ") -> ("
> +					     << curr->skipping.xOddInc << "," << curr->skipping.xEvenInc << ","
> +					     << curr->skipping.yOddInc << "," << curr->skipping.yEvenInc << "); ";
> +					sensor_changed = true;
> +				}
> +				if (orig->outputSize != curr->outputSize) {
> +					diff << "outputSize: " << orig->outputSize.toString() << " -> " << curr->outputSize.toString() << "; ";
> +					sensor_changed = true;
> +				}
> +			}
> +			if (sensor_changed) {
> +				GST_WARNING_OBJECT(self, "SensorConfiguration changed: %s", diff.str().c_str());
> +				warned = true;
> +			}
> +		}
> +		// Warn about orientation change
> +		if (orig_orientation != state->config_->orientation) {
> +			GEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> +			const char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> +			const char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> +			GST_WARNING_OBJECT(self, "Orientation changed: %s -> %s", orig_orientation_str, new_orientation_str);
> +			warned = true;
> +		}
> +		if (!warned) {
> +			GST_DEBUG_OBJECT(self, "Camera configuration adjusted, but no significant changes detected.");
> +		}
> +		// Update Gst orientation property to match adjusted config
> +		self->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> +		break;
> +	}

I am still trying to understand why you need to diff the
CameraConfiguration ? Is it just for logging purposes - to warn that the
configuration has been changed, in a detailed manner? 

My goal to pointing you to Jaslo's patch was:
a) If the configuration is changed, that patch already logs a warning
b) If the the adjusted configuration is not acceptable by downstream
   elements, the pipeline streaming is rejected.

If the configuration is changed, could you not simply report the new
orientation in the property? 


> +	case CameraConfiguration::Invalid:
> +		GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> +				  ("Camera configuration is not supported"),
> +				  ("CameraConfiguration::validate() returned Invalid"));
>  		return false;
> +	}
>  
>  	int ret = state->cam_->configure(state->config_.get());
>  	if (ret) {
> @@ -926,6 +1029,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 +1051,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 +1257,17 @@ 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
>

The diff part has been suggested by jmondi and pobrn in IRC chat the very
same day Jaslo's patch has been posted, also my V1 patch was already just
printing the new orientation and a general message for
StreamConfigutation(s) and SensorConfiguration. There's no problem in
replacing/reverting it and use Jaslo's solution. There are no diffing
method on the relevant objects anyway so a clean solution would not be
available until then.
Being this my first patch, what happens now? Should I post a V4 with
changes in negotiation function deleted and wait for the merge of the two
patches, or should I integrate Jaslo's changes into mine, or vice-versa?

Thanks,
G.C.

On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:

> On Wed, Jul 23, 2025 at 04:07:55PM +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
> >
> > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > changes in StreamConfiguration and SensorConfiguration, as well as
> > the new Orientation parameter.
> >
> > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > ---
> >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> >  3 files changed, 166 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> b/src/gstreamer/gstlibcamera-utils.cpp
> > index a548b0c1..95d3813e 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;
> >
> > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > +
> > +     /* Save original configuration for comparison after validation */
> > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > +     for (gsize i = 0; i < state->config_->size(); i++)
> > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> state->config_->sensorConfig;
> > +     Orientation orig_orientation = state->config_->orientation;
> > +
> >       /* Validate the configuration. */
> > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > +     switch (state->config_->validate()) {
> > +     case CameraConfiguration::Valid:
> > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > +             break;
> > +     case CameraConfiguration::Adjusted: {
> > +             bool warned = false;
> > +             // Warn if number of StreamConfigurations changed
> > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > +                     GST_WARNING_OBJECT(self, "Number of
> StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > +                                        orig_stream_cfgs.size(),
> state->config_->size());
> > +                     warned = true;
> > +             }
> > +             // Warn about changes in each StreamConfiguration
> > +             // TODO implement diffing in StreamConfiguration
> > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> state->config_->size()); i++) {
> > +                     if (orig_stream_cfgs[i].toString() !=
> state->config_->at(i).toString()) {
> > +                             GST_WARNING_OBJECT(self,
> "StreamConfiguration %zu changed: %s -> %s",
> > +                                                i,
> orig_stream_cfgs[i].toString().c_str(),
> > +
> state->config_->at(i).toString().c_str());
> > +                             warned = true;
> > +                     }
> > +             }
> > +             // Warn about SensorConfiguration changes
> > +             // TODO implement diffing in SensorConfiguration
> > +             if (orig_sensor_cfg.has_value() ||
> state->config_->sensorConfig.has_value()) {
> > +                     const SensorConfiguration *orig =
> orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > +                     const SensorConfiguration *curr =
> state->config_->sensorConfig.has_value() ?
> &state->config_->sensorConfig.value() : nullptr;
> > +                     bool sensor_changed = false;
> > +                     std::ostringstream diff;
> > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > +                             diff << "SensorConfiguration presence
> changed: "
> > +                                  << (orig ? "was present" : "was
> absent")
> > +                                  << " -> "
> > +                                  << (curr ? "present" : "absent");
> > +                             sensor_changed = true;
> > +                     } else if (orig && curr) {
> > +                             if (orig->bitDepth != curr->bitDepth) {
> > +                                     diff << "bitDepth: " <<
> orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > +                                     sensor_changed = true;
> > +                             }
> > +                             if (orig->analogCrop != curr->analogCrop) {
> > +                                     diff << "analogCrop: " <<
> orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> ";
> > +                                     sensor_changed = true;
> > +                             }
> > +                             if (orig->binning.binX !=
> curr->binning.binX ||
> > +                                 orig->binning.binY !=
> curr->binning.binY) {
> > +                                     diff << "binning: (" <<
> orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > +                                          << curr->binning.binX << ","
> << curr->binning.binY << "); ";
> > +                                     sensor_changed = true;
> > +                             }
> > +                             if (orig->skipping.xOddInc !=
> curr->skipping.xOddInc ||
> > +                                 orig->skipping.xEvenInc !=
> curr->skipping.xEvenInc ||
> > +                                 orig->skipping.yOddInc !=
> curr->skipping.yOddInc ||
> > +                                 orig->skipping.yEvenInc !=
> curr->skipping.yEvenInc) {
> > +                                     diff << "skipping: ("
> > +                                          << orig->skipping.xOddInc <<
> "," << orig->skipping.xEvenInc << ","
> > +                                          << orig->skipping.yOddInc <<
> "," << orig->skipping.yEvenInc << ") -> ("
> > +                                          << curr->skipping.xOddInc <<
> "," << curr->skipping.xEvenInc << ","
> > +                                          << curr->skipping.yOddInc <<
> "," << curr->skipping.yEvenInc << "); ";
> > +                                     sensor_changed = true;
> > +                             }
> > +                             if (orig->outputSize != curr->outputSize) {
> > +                                     diff << "outputSize: " <<
> orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> ";
> > +                                     sensor_changed = true;
> > +                             }
> > +                     }
> > +                     if (sensor_changed) {
> > +                             GST_WARNING_OBJECT(self,
> "SensorConfiguration changed: %s", diff.str().c_str());
> > +                             warned = true;
> > +                     }
> > +             }
> > +             // Warn about orientation change
> > +             if (orig_orientation != state->config_->orientation) {
> > +                     GEnumClass *enum_class = (GEnumClass
> *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > +                     const char *orig_orientation_str =
> g_enum_get_value(enum_class,
> libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > +                     const char *new_orientation_str =
> g_enum_get_value(enum_class,
> libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> -> %s", orig_orientation_str, new_orientation_str);
> > +                     warned = true;
> > +             }
> > +             if (!warned) {
> > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> adjusted, but no significant changes detected.");
> > +             }
> > +             // Update Gst orientation property to match adjusted config
> > +             self->orientation =
> libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > +             break;
> > +     }
>
> I am still trying to understand why you need to diff the
> CameraConfiguration ? Is it just for logging purposes - to warn that the
> configuration has been changed, in a detailed manner?
>
> My goal to pointing you to Jaslo's patch was:
> a) If the configuration is changed, that patch already logs a warning
> b) If the the adjusted configuration is not acceptable by downstream
>    elements, the pipeline streaming is rejected.
>
> If the configuration is changed, could you not simply report the new
> orientation in the property?
>
>
> > +     case CameraConfiguration::Invalid:
> > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > +                               ("Camera configuration is not
> supported"),
> > +                               ("CameraConfiguration::validate()
> returned Invalid"));
> >               return false;
> > +     }
> >
> >       int ret = state->cam_->configure(state->config_.get());
> >       if (ret) {
> > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> 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
> >
>

On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:
> The diff part has been suggested by jmondi and pobrn in IRC chat the very
> same day Jaslo's patch has been posted, also my V1 patch was already just

Sorry, but that's not what happened

You suggested we need a CameraConfiguration diff and me and pobrn
tried to suggest how to do it. The fact you need was solely suggested
by you.

  giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
  giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
  giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
  pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
  giaco | I'll do what I can

Below the full log [*]

> printing the new orientation and a general message for
> StreamConfigutation(s) and SensorConfiguration. There's no problem in
> replacing/reverting it and use Jaslo's solution. There are no diffing
> method on the relevant objects anyway so a clean solution would not be
> available until then.
> Being this my first patch, what happens now? Should I post a V4 with
> changes in negotiation function deleted and wait for the merge of the two
> patches, or should I integrate Jaslo's changes into mine, or vice-versa?

If there's something you find useful in Jaslo's patches rebase your
changes on top of his ones and specify in the cover that your series
depend on his one.


>
> Thanks,
> G.C.
>
> On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:
>
> > On Wed, Jul 23, 2025 at 04:07:55PM +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
> > >
> > > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > > changes in StreamConfiguration and SensorConfiguration, as well as
> > > the new Orientation parameter.
> > >
> > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > ---
> > >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> > >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> > >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> > >  3 files changed, 166 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > b/src/gstreamer/gstlibcamera-utils.cpp
> > > index a548b0c1..95d3813e 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;
> > >
> > > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > > +
> > > +     /* Save original configuration for comparison after validation */
> > > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > > +     for (gsize i = 0; i < state->config_->size(); i++)
> > > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> > state->config_->sensorConfig;
> > > +     Orientation orig_orientation = state->config_->orientation;
> > > +
> > >       /* Validate the configuration. */
> > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > +     switch (state->config_->validate()) {
> > > +     case CameraConfiguration::Valid:
> > > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > > +             break;
> > > +     case CameraConfiguration::Adjusted: {
> > > +             bool warned = false;
> > > +             // Warn if number of StreamConfigurations changed
> > > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > > +                     GST_WARNING_OBJECT(self, "Number of
> > StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > > +                                        orig_stream_cfgs.size(),
> > state->config_->size());
> > > +                     warned = true;
> > > +             }
> > > +             // Warn about changes in each StreamConfiguration
> > > +             // TODO implement diffing in StreamConfiguration
> > > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> > state->config_->size()); i++) {
> > > +                     if (orig_stream_cfgs[i].toString() !=
> > state->config_->at(i).toString()) {
> > > +                             GST_WARNING_OBJECT(self,
> > "StreamConfiguration %zu changed: %s -> %s",
> > > +                                                i,
> > orig_stream_cfgs[i].toString().c_str(),
> > > +
> > state->config_->at(i).toString().c_str());
> > > +                             warned = true;
> > > +                     }
> > > +             }
> > > +             // Warn about SensorConfiguration changes
> > > +             // TODO implement diffing in SensorConfiguration
> > > +             if (orig_sensor_cfg.has_value() ||
> > state->config_->sensorConfig.has_value()) {
> > > +                     const SensorConfiguration *orig =
> > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > > +                     const SensorConfiguration *curr =
> > state->config_->sensorConfig.has_value() ?
> > &state->config_->sensorConfig.value() : nullptr;
> > > +                     bool sensor_changed = false;
> > > +                     std::ostringstream diff;
> > > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > > +                             diff << "SensorConfiguration presence
> > changed: "
> > > +                                  << (orig ? "was present" : "was
> > absent")
> > > +                                  << " -> "
> > > +                                  << (curr ? "present" : "absent");
> > > +                             sensor_changed = true;
> > > +                     } else if (orig && curr) {
> > > +                             if (orig->bitDepth != curr->bitDepth) {
> > > +                                     diff << "bitDepth: " <<
> > orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > > +                                     sensor_changed = true;
> > > +                             }
> > > +                             if (orig->analogCrop != curr->analogCrop) {
> > > +                                     diff << "analogCrop: " <<
> > orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> > ";
> > > +                                     sensor_changed = true;
> > > +                             }
> > > +                             if (orig->binning.binX !=
> > curr->binning.binX ||
> > > +                                 orig->binning.binY !=
> > curr->binning.binY) {
> > > +                                     diff << "binning: (" <<
> > orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > > +                                          << curr->binning.binX << ","
> > << curr->binning.binY << "); ";
> > > +                                     sensor_changed = true;
> > > +                             }
> > > +                             if (orig->skipping.xOddInc !=
> > curr->skipping.xOddInc ||
> > > +                                 orig->skipping.xEvenInc !=
> > curr->skipping.xEvenInc ||
> > > +                                 orig->skipping.yOddInc !=
> > curr->skipping.yOddInc ||
> > > +                                 orig->skipping.yEvenInc !=
> > curr->skipping.yEvenInc) {
> > > +                                     diff << "skipping: ("
> > > +                                          << orig->skipping.xOddInc <<
> > "," << orig->skipping.xEvenInc << ","
> > > +                                          << orig->skipping.yOddInc <<
> > "," << orig->skipping.yEvenInc << ") -> ("
> > > +                                          << curr->skipping.xOddInc <<
> > "," << curr->skipping.xEvenInc << ","
> > > +                                          << curr->skipping.yOddInc <<
> > "," << curr->skipping.yEvenInc << "); ";
> > > +                                     sensor_changed = true;
> > > +                             }
> > > +                             if (orig->outputSize != curr->outputSize) {
> > > +                                     diff << "outputSize: " <<
> > orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> > ";
> > > +                                     sensor_changed = true;
> > > +                             }
> > > +                     }
> > > +                     if (sensor_changed) {
> > > +                             GST_WARNING_OBJECT(self,
> > "SensorConfiguration changed: %s", diff.str().c_str());
> > > +                             warned = true;
> > > +                     }
> > > +             }
> > > +             // Warn about orientation change
> > > +             if (orig_orientation != state->config_->orientation) {
> > > +                     GEnumClass *enum_class = (GEnumClass
> > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > > +                     const char *orig_orientation_str =
> > g_enum_get_value(enum_class,
> > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > > +                     const char *new_orientation_str =
> > g_enum_get_value(enum_class,
> > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> > -> %s", orig_orientation_str, new_orientation_str);
> > > +                     warned = true;
> > > +             }
> > > +             if (!warned) {
> > > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> > adjusted, but no significant changes detected.");
> > > +             }
> > > +             // Update Gst orientation property to match adjusted config
> > > +             self->orientation =
> > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > > +             break;
> > > +     }
> >
> > I am still trying to understand why you need to diff the
> > CameraConfiguration ? Is it just for logging purposes - to warn that the
> > configuration has been changed, in a detailed manner?
> >
> > My goal to pointing you to Jaslo's patch was:
> > a) If the configuration is changed, that patch already logs a warning
> > b) If the the adjusted configuration is not acceptable by downstream
> >    elements, the pipeline streaming is rejected.
> >
> > If the configuration is changed, could you not simply report the new
> > orientation in the property?
> >
> >
> > > +     case CameraConfiguration::Invalid:
> > > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > +                               ("Camera configuration is not
> > supported"),
> > > +                               ("CameraConfiguration::validate()
> > returned Invalid"));
> > >               return false;
> > > +     }
> > >
> > >       int ret = state->cam_->configure(state->config_.get());
> > >       if (ret) {
> > > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> > 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
> > >
> >

[*]

  giaco | I'm doing the "track which CameraConfiguration attributes have been adjusted after validate()". How do I know what can be changed and
        | what not?
  giaco | it seems something validate() should do, not the caller
 jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out
 jmondi | it boils down to a few things, streams' sizes and formats and orientation
  giaco | can I assume that the total number of requested streams remains the same?
 jmondi | no
  giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?
 jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415
 jmondi | not larger, the pipeline can reduce it to match the number of streams it supports
 jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust
 jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)
  giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted
 jmondi | patches are welcome
  giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,
        | SensorConfiguration and Orientation, too
  giaco | and aall turtles down ColorSpace and on
  giaco | I think I will should to the user "hey your config is adjusted here's your string representation of the new one"
  giaco | *shout
 jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration
        | ?
 jmondi | building a proper diff function is not trivial I think
  giaco | diff has to be implemented in all the objects required to be diffed
  giaco | and share a common structure
  giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor
 jmondi | we should have somewhere a TODO list for ideas for people to contribute
 jmondi | I think we used to
  giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable
        | number of warnings
 jmondi | libcamera can adjust, maybe what it adjusts to is not suitable for the user's final use case, and pipewire/gstreamer/rpi-apps sits in
        | between
  giaco | how to get a copy of CameraConfiguration before validation?
  pobrn | don't
  giaco | then it's impossible to track what validate() does
  pobrn | you can make copies of the `StreamConfiguration`s
  pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of
        | `StreamConfiguration`
  giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
  giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
  giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
  pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
  giaco | I'll do what I can
  giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but "libcamerasrc
        | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter:<cs> error: Failed to generate camera configuration from roles"
  giaco | testing with "GST_DEBUG=2 gst-launch-1.0 libcamerasrc name=cs cs.src ! fakesink cs.src_0 ! fakevideosink" on a camera that supports just
        | 1 stream
 jmondi | the gstreamer element does not accept less streams than what it had requested
 jmondi | if (!config || config->size() != roles.size())
 jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno

It's important to note that this is not meant as an argument, but as a
detailed explanation of my perspective.

I don't agree with your assertion that the CameraConfiguration diffing
procedure was "not requested." This contradicts the evidence in the
IRC logs provided.
The entire premise of tracking configuration adjustments after
validate() was to provide meaningful information to users,
particularly within frameworks like GStreamer. The conversation
explicitly highlights the need to understand "what has changed." The
log snippet below is particularly pertinent:

jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)
and notifies you about that, what has changed is up to you to figure
out

This statement places the responsibility on me to determine what has
changed. How is one to "figure out" what has changed without a
mechanism to compare the before and after states? The logical and
necessary conclusion, which was subsequently discussed, was a diffing
procedure.

Furthermore, my direct question:
giaco | build a proper CameraConfiguration diff function, I need to
copy it before validation and compare if adjusted

was met with:

jmondi | patches are welcome

This is not a rejection; it is an invitation to develop the
functionality. If the intent was truly that this was not requested or
desired, that would have been the moment to communicate it, not after.
To wait until a patch is developed and submitted, only to then claim
it was "not requested" is inefficient use of resources. Had this been
communicated upfront, I could have focused my efforts elsewhere.
If the underlying truth is that is not right time for diffing (as
relevant objects do not provide diffing helpers) not the right place
(negotiation in gstreamer element) and to move this to a separate
task/patch, I agree.

Regarding the path forward, I'll post a new version (V4 with cover)
based on Jaslo's patch with the diffing functionality removed.

G.C.

Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi
<jacopo.mondi@ideasonboard.com> ha scritto:
>
> On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:
> > The diff part has been suggested by jmondi and pobrn in IRC chat the very
> > same day Jaslo's patch has been posted, also my V1 patch was already just
>
> Sorry, but that's not what happened
>
> You suggested we need a CameraConfiguration diff and me and pobrn
> tried to suggest how to do it. The fact you need was solely suggested
> by you.
>
>   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
>   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
>   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
>   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
>   giaco | I'll do what I can
>
> Below the full log [*]
>
> > printing the new orientation and a general message for
> > StreamConfigutation(s) and SensorConfiguration. There's no problem in
> > replacing/reverting it and use Jaslo's solution. There are no diffing
> > method on the relevant objects anyway so a clean solution would not be
> > available until then.
> > Being this my first patch, what happens now? Should I post a V4 with
> > changes in negotiation function deleted and wait for the merge of the two
> > patches, or should I integrate Jaslo's changes into mine, or vice-versa?
>
> If there's something you find useful in Jaslo's patches rebase your
> changes on top of his ones and specify in the cover that your series
> depend on his one.
>
>
> >
> > Thanks,
> > G.C.
> >
> > On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:
> >
> > > On Wed, Jul 23, 2025 at 04:07:55PM +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
> > > >
> > > > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > > > changes in StreamConfiguration and SensorConfiguration, as well as
> > > > the new Orientation parameter.
> > > >
> > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > > ---
> > > >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> > > >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> > > >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> > > >  3 files changed, 166 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > index a548b0c1..95d3813e 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;
> > > >
> > > > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > > > +
> > > > +     /* Save original configuration for comparison after validation */
> > > > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > > > +     for (gsize i = 0; i < state->config_->size(); i++)
> > > > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > > > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> > > state->config_->sensorConfig;
> > > > +     Orientation orig_orientation = state->config_->orientation;
> > > > +
> > > >       /* Validate the configuration. */
> > > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > > +     switch (state->config_->validate()) {
> > > > +     case CameraConfiguration::Valid:
> > > > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > > > +             break;
> > > > +     case CameraConfiguration::Adjusted: {
> > > > +             bool warned = false;
> > > > +             // Warn if number of StreamConfigurations changed
> > > > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > > > +                     GST_WARNING_OBJECT(self, "Number of
> > > StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > > > +                                        orig_stream_cfgs.size(),
> > > state->config_->size());
> > > > +                     warned = true;
> > > > +             }
> > > > +             // Warn about changes in each StreamConfiguration
> > > > +             // TODO implement diffing in StreamConfiguration
> > > > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> > > state->config_->size()); i++) {
> > > > +                     if (orig_stream_cfgs[i].toString() !=
> > > state->config_->at(i).toString()) {
> > > > +                             GST_WARNING_OBJECT(self,
> > > "StreamConfiguration %zu changed: %s -> %s",
> > > > +                                                i,
> > > orig_stream_cfgs[i].toString().c_str(),
> > > > +
> > > state->config_->at(i).toString().c_str());
> > > > +                             warned = true;
> > > > +                     }
> > > > +             }
> > > > +             // Warn about SensorConfiguration changes
> > > > +             // TODO implement diffing in SensorConfiguration
> > > > +             if (orig_sensor_cfg.has_value() ||
> > > state->config_->sensorConfig.has_value()) {
> > > > +                     const SensorConfiguration *orig =
> > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > > > +                     const SensorConfiguration *curr =
> > > state->config_->sensorConfig.has_value() ?
> > > &state->config_->sensorConfig.value() : nullptr;
> > > > +                     bool sensor_changed = false;
> > > > +                     std::ostringstream diff;
> > > > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > > > +                             diff << "SensorConfiguration presence
> > > changed: "
> > > > +                                  << (orig ? "was present" : "was
> > > absent")
> > > > +                                  << " -> "
> > > > +                                  << (curr ? "present" : "absent");
> > > > +                             sensor_changed = true;
> > > > +                     } else if (orig && curr) {
> > > > +                             if (orig->bitDepth != curr->bitDepth) {
> > > > +                                     diff << "bitDepth: " <<
> > > orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > > > +                                     sensor_changed = true;
> > > > +                             }
> > > > +                             if (orig->analogCrop != curr->analogCrop) {
> > > > +                                     diff << "analogCrop: " <<
> > > orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> > > ";
> > > > +                                     sensor_changed = true;
> > > > +                             }
> > > > +                             if (orig->binning.binX !=
> > > curr->binning.binX ||
> > > > +                                 orig->binning.binY !=
> > > curr->binning.binY) {
> > > > +                                     diff << "binning: (" <<
> > > orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > > > +                                          << curr->binning.binX << ","
> > > << curr->binning.binY << "); ";
> > > > +                                     sensor_changed = true;
> > > > +                             }
> > > > +                             if (orig->skipping.xOddInc !=
> > > curr->skipping.xOddInc ||
> > > > +                                 orig->skipping.xEvenInc !=
> > > curr->skipping.xEvenInc ||
> > > > +                                 orig->skipping.yOddInc !=
> > > curr->skipping.yOddInc ||
> > > > +                                 orig->skipping.yEvenInc !=
> > > curr->skipping.yEvenInc) {
> > > > +                                     diff << "skipping: ("
> > > > +                                          << orig->skipping.xOddInc <<
> > > "," << orig->skipping.xEvenInc << ","
> > > > +                                          << orig->skipping.yOddInc <<
> > > "," << orig->skipping.yEvenInc << ") -> ("
> > > > +                                          << curr->skipping.xOddInc <<
> > > "," << curr->skipping.xEvenInc << ","
> > > > +                                          << curr->skipping.yOddInc <<
> > > "," << curr->skipping.yEvenInc << "); ";
> > > > +                                     sensor_changed = true;
> > > > +                             }
> > > > +                             if (orig->outputSize != curr->outputSize) {
> > > > +                                     diff << "outputSize: " <<
> > > orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> > > ";
> > > > +                                     sensor_changed = true;
> > > > +                             }
> > > > +                     }
> > > > +                     if (sensor_changed) {
> > > > +                             GST_WARNING_OBJECT(self,
> > > "SensorConfiguration changed: %s", diff.str().c_str());
> > > > +                             warned = true;
> > > > +                     }
> > > > +             }
> > > > +             // Warn about orientation change
> > > > +             if (orig_orientation != state->config_->orientation) {
> > > > +                     GEnumClass *enum_class = (GEnumClass
> > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > > > +                     const char *orig_orientation_str =
> > > g_enum_get_value(enum_class,
> > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > > > +                     const char *new_orientation_str =
> > > g_enum_get_value(enum_class,
> > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > > > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> > > -> %s", orig_orientation_str, new_orientation_str);
> > > > +                     warned = true;
> > > > +             }
> > > > +             if (!warned) {
> > > > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> > > adjusted, but no significant changes detected.");
> > > > +             }
> > > > +             // Update Gst orientation property to match adjusted config
> > > > +             self->orientation =
> > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > > > +             break;
> > > > +     }
> > >
> > > I am still trying to understand why you need to diff the
> > > CameraConfiguration ? Is it just for logging purposes - to warn that the
> > > configuration has been changed, in a detailed manner?
> > >
> > > My goal to pointing you to Jaslo's patch was:
> > > a) If the configuration is changed, that patch already logs a warning
> > > b) If the the adjusted configuration is not acceptable by downstream
> > >    elements, the pipeline streaming is rejected.
> > >
> > > If the configuration is changed, could you not simply report the new
> > > orientation in the property?
> > >
> > >
> > > > +     case CameraConfiguration::Invalid:
> > > > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > +                               ("Camera configuration is not
> > > supported"),
> > > > +                               ("CameraConfiguration::validate()
> > > returned Invalid"));
> > > >               return false;
> > > > +     }
> > > >
> > > >       int ret = state->cam_->configure(state->config_.get());
> > > >       if (ret) {
> > > > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> > > 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
> > > >
> > >
>
> [*]
>
>   giaco | I'm doing the "track which CameraConfiguration attributes have been adjusted after validate()". How do I know what can be changed and
>         | what not?
>   giaco | it seems something validate() should do, not the caller
>  jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out
>  jmondi | it boils down to a few things, streams' sizes and formats and orientation
>   giaco | can I assume that the total number of requested streams remains the same?
>  jmondi | no
>   giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?
>  jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415
>  jmondi | not larger, the pipeline can reduce it to match the number of streams it supports
>  jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust
>  jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)
>   giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted
>  jmondi | patches are welcome
>   giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,
>         | SensorConfiguration and Orientation, too
>   giaco | and aall turtles down ColorSpace and on
>   giaco | I think I will should to the user "hey your config is adjusted here's your string representation of the new one"
>   giaco | *shout
>  jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration
>         | ?
>  jmondi | building a proper diff function is not trivial I think
>   giaco | diff has to be implemented in all the objects required to be diffed
>   giaco | and share a common structure
>   giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor
>  jmondi | we should have somewhere a TODO list for ideas for people to contribute
>  jmondi | I think we used to
>   giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable
>         | number of warnings
>  jmondi | libcamera can adjust, maybe what it adjusts to is not suitable for the user's final use case, and pipewire/gstreamer/rpi-apps sits in
>         | between
>   giaco | how to get a copy of CameraConfiguration before validation?
>   pobrn | don't
>   giaco | then it's impossible to track what validate() does
>   pobrn | you can make copies of the `StreamConfiguration`s
>   pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of
>         | `StreamConfiguration`
>   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
>   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
>   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
>   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
>   giaco | I'll do what I can
>   giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but "libcamerasrc
>         | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter:<cs> error: Failed to generate camera configuration from roles"
>   giaco | testing with "GST_DEBUG=2 gst-launch-1.0 libcamerasrc name=cs cs.src ! fakesink cs.src_0 ! fakevideosink" on a camera that supports just
>         | 1 stream
>  jmondi | the gstreamer element does not accept less streams than what it had requested
>  jmondi | if (!config || config->size() != roles.size())
>  jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno

Listen,
  you can try to put things as you like but

On Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:
> It's important to note that this is not meant as an argument, but as a
> detailed explanation of my perspective.
>
> I don't agree with your assertion that the CameraConfiguration diffing
> procedure was "not requested." This contradicts the evidence in the
> IRC logs provided.
> The entire premise of tracking configuration adjustments after
> validate() was to provide meaningful information to users,
> particularly within frameworks like GStreamer. The conversation
> explicitly highlights the need to understand "what has changed." The
> log snippet below is particularly pertinent:
>
> jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)
> and notifies you about that, what has changed is up to you to figure
> out
>
> This statement places the responsibility on me to determine what has
> changed. How is one to "figure out" what has changed without a
> mechanism to compare the before and after states? The logical and
> necessary conclusion, which was subsequently discussed, was a diffing
> procedure.

All of out bindings report "Configuration has been adjusted"
in example

Android:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734

gstreamer:
https://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620

You decided you want to report "what has changed" nobody asked you to

>
> Furthermore, my direct question:
> giaco | build a proper CameraConfiguration diff function, I need to
> copy it before validation and compare if adjusted
>
> was met with:
>
> jmondi | patches are welcome
>

You tell me (imperatively) to "build a proper CameraConfiguration diff
function" and the only thing I can suggest is that you are free to
send patches, not because gstreamer need it, because it might be a
useful addition to libcamera in general (I also suggested to add a
todo list for contributors for that matter).

nobody asked you to do so. stop putting words in my mouth please.

> This is not a rejection; it is an invitation to develop the
> functionality. If the intent was truly that this was not requested or
> desired, that would have been the moment to communicate it, not after.

It's a useful addition to libcamera. Not a requirement for gstreamer.

> To wait until a patch is developed and submitted, only to then claim
> it was "not requested" is inefficient use of resources. Had this been
> communicated upfront, I could have focused my efforts elsewhere.

Likewise.

I'll make sure not do waste your time anymore by making sure
I won't waste mine supporting you like I've always tried to do,
don't worry.

> If the underlying truth is that is not right time for diffing (as
> relevant objects do not provide diffing helpers) not the right place
> (negotiation in gstreamer element) and to move this to a separate
> task/patch, I agree.
>
> Regarding the path forward, I'll post a new version (V4 with cover)
> based on Jaslo's patch with the diffing functionality removed.
>
> G.C.
>
> Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> ha scritto:
> >
> > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:
> > > The diff part has been suggested by jmondi and pobrn in IRC chat the very
> > > same day Jaslo's patch has been posted, also my V1 patch was already just
> >
> > Sorry, but that's not what happened
> >
> > You suggested we need a CameraConfiguration diff and me and pobrn
> > tried to suggest how to do it. The fact you need was solely suggested
> > by you.
> >
> >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> >   giaco | I'll do what I can
> >
> > Below the full log [*]
> >
> > > printing the new orientation and a general message for
> > > StreamConfigutation(s) and SensorConfiguration. There's no problem in
> > > replacing/reverting it and use Jaslo's solution. There are no diffing
> > > method on the relevant objects anyway so a clean solution would not be
> > > available until then.
> > > Being this my first patch, what happens now? Should I post a V4 with
> > > changes in negotiation function deleted and wait for the merge of the two
> > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?
> >
> > If there's something you find useful in Jaslo's patches rebase your
> > changes on top of his ones and specify in the cover that your series
> > depend on his one.
> >
> >
> > >
> > > Thanks,
> > > G.C.
> > >
> > > On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:
> > >
> > > > On Wed, Jul 23, 2025 at 04:07:55PM +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
> > > > >
> > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > > > > changes in StreamConfiguration and SensorConfiguration, as well as
> > > > > the new Orientation parameter.
> > > > >
> > > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > > > ---
> > > > >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> > > > >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> > > > >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> > > > >  3 files changed, 166 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > index a548b0c1..95d3813e 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;
> > > > >
> > > > > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > > > > +
> > > > > +     /* Save original configuration for comparison after validation */
> > > > > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > > > > +     for (gsize i = 0; i < state->config_->size(); i++)
> > > > > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > > > > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> > > > state->config_->sensorConfig;
> > > > > +     Orientation orig_orientation = state->config_->orientation;
> > > > > +
> > > > >       /* Validate the configuration. */
> > > > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > > > +     switch (state->config_->validate()) {
> > > > > +     case CameraConfiguration::Valid:
> > > > > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > > > > +             break;
> > > > > +     case CameraConfiguration::Adjusted: {
> > > > > +             bool warned = false;
> > > > > +             // Warn if number of StreamConfigurations changed
> > > > > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > > > > +                     GST_WARNING_OBJECT(self, "Number of
> > > > StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > > > > +                                        orig_stream_cfgs.size(),
> > > > state->config_->size());
> > > > > +                     warned = true;
> > > > > +             }
> > > > > +             // Warn about changes in each StreamConfiguration
> > > > > +             // TODO implement diffing in StreamConfiguration
> > > > > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> > > > state->config_->size()); i++) {
> > > > > +                     if (orig_stream_cfgs[i].toString() !=
> > > > state->config_->at(i).toString()) {
> > > > > +                             GST_WARNING_OBJECT(self,
> > > > "StreamConfiguration %zu changed: %s -> %s",
> > > > > +                                                i,
> > > > orig_stream_cfgs[i].toString().c_str(),
> > > > > +
> > > > state->config_->at(i).toString().c_str());
> > > > > +                             warned = true;
> > > > > +                     }
> > > > > +             }
> > > > > +             // Warn about SensorConfiguration changes
> > > > > +             // TODO implement diffing in SensorConfiguration
> > > > > +             if (orig_sensor_cfg.has_value() ||
> > > > state->config_->sensorConfig.has_value()) {
> > > > > +                     const SensorConfiguration *orig =
> > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > > > > +                     const SensorConfiguration *curr =
> > > > state->config_->sensorConfig.has_value() ?
> > > > &state->config_->sensorConfig.value() : nullptr;
> > > > > +                     bool sensor_changed = false;
> > > > > +                     std::ostringstream diff;
> > > > > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > > > > +                             diff << "SensorConfiguration presence
> > > > changed: "
> > > > > +                                  << (orig ? "was present" : "was
> > > > absent")
> > > > > +                                  << " -> "
> > > > > +                                  << (curr ? "present" : "absent");
> > > > > +                             sensor_changed = true;
> > > > > +                     } else if (orig && curr) {
> > > > > +                             if (orig->bitDepth != curr->bitDepth) {
> > > > > +                                     diff << "bitDepth: " <<
> > > > orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > > > > +                                     sensor_changed = true;
> > > > > +                             }
> > > > > +                             if (orig->analogCrop != curr->analogCrop) {
> > > > > +                                     diff << "analogCrop: " <<
> > > > orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> > > > ";
> > > > > +                                     sensor_changed = true;
> > > > > +                             }
> > > > > +                             if (orig->binning.binX !=
> > > > curr->binning.binX ||
> > > > > +                                 orig->binning.binY !=
> > > > curr->binning.binY) {
> > > > > +                                     diff << "binning: (" <<
> > > > orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > > > > +                                          << curr->binning.binX << ","
> > > > << curr->binning.binY << "); ";
> > > > > +                                     sensor_changed = true;
> > > > > +                             }
> > > > > +                             if (orig->skipping.xOddInc !=
> > > > curr->skipping.xOddInc ||
> > > > > +                                 orig->skipping.xEvenInc !=
> > > > curr->skipping.xEvenInc ||
> > > > > +                                 orig->skipping.yOddInc !=
> > > > curr->skipping.yOddInc ||
> > > > > +                                 orig->skipping.yEvenInc !=
> > > > curr->skipping.yEvenInc) {
> > > > > +                                     diff << "skipping: ("
> > > > > +                                          << orig->skipping.xOddInc <<
> > > > "," << orig->skipping.xEvenInc << ","
> > > > > +                                          << orig->skipping.yOddInc <<
> > > > "," << orig->skipping.yEvenInc << ") -> ("
> > > > > +                                          << curr->skipping.xOddInc <<
> > > > "," << curr->skipping.xEvenInc << ","
> > > > > +                                          << curr->skipping.yOddInc <<
> > > > "," << curr->skipping.yEvenInc << "); ";
> > > > > +                                     sensor_changed = true;
> > > > > +                             }
> > > > > +                             if (orig->outputSize != curr->outputSize) {
> > > > > +                                     diff << "outputSize: " <<
> > > > orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> > > > ";
> > > > > +                                     sensor_changed = true;
> > > > > +                             }
> > > > > +                     }
> > > > > +                     if (sensor_changed) {
> > > > > +                             GST_WARNING_OBJECT(self,
> > > > "SensorConfiguration changed: %s", diff.str().c_str());
> > > > > +                             warned = true;
> > > > > +                     }
> > > > > +             }
> > > > > +             // Warn about orientation change
> > > > > +             if (orig_orientation != state->config_->orientation) {
> > > > > +                     GEnumClass *enum_class = (GEnumClass
> > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > > > > +                     const char *orig_orientation_str =
> > > > g_enum_get_value(enum_class,
> > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > > > > +                     const char *new_orientation_str =
> > > > g_enum_get_value(enum_class,
> > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > > > > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> > > > -> %s", orig_orientation_str, new_orientation_str);
> > > > > +                     warned = true;
> > > > > +             }
> > > > > +             if (!warned) {
> > > > > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> > > > adjusted, but no significant changes detected.");
> > > > > +             }
> > > > > +             // Update Gst orientation property to match adjusted config
> > > > > +             self->orientation =
> > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > > > > +             break;
> > > > > +     }
> > > >
> > > > I am still trying to understand why you need to diff the
> > > > CameraConfiguration ? Is it just for logging purposes - to warn that the
> > > > configuration has been changed, in a detailed manner?
> > > >
> > > > My goal to pointing you to Jaslo's patch was:
> > > > a) If the configuration is changed, that patch already logs a warning
> > > > b) If the the adjusted configuration is not acceptable by downstream
> > > >    elements, the pipeline streaming is rejected.
> > > >
> > > > If the configuration is changed, could you not simply report the new
> > > > orientation in the property?
> > > >
> > > >
> > > > > +     case CameraConfiguration::Invalid:
> > > > > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > > +                               ("Camera configuration is not
> > > > supported"),
> > > > > +                               ("CameraConfiguration::validate()
> > > > returned Invalid"));
> > > > >               return false;
> > > > > +     }
> > > > >
> > > > >       int ret = state->cam_->configure(state->config_.get());
> > > > >       if (ret) {
> > > > > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> > > > 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
> > > > >
> > > >
> >
> > [*]
> >
> >   giaco | I'm doing the "track which CameraConfiguration attributes have been adjusted after validate()". How do I know what can be changed and
> >         | what not?
> >   giaco | it seems something validate() should do, not the caller
> >  jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out
> >  jmondi | it boils down to a few things, streams' sizes and formats and orientation
> >   giaco | can I assume that the total number of requested streams remains the same?
> >  jmondi | no
> >   giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?
> >  jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415
> >  jmondi | not larger, the pipeline can reduce it to match the number of streams it supports
> >  jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust
> >  jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)
> >   giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted
> >  jmondi | patches are welcome
> >   giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,
> >         | SensorConfiguration and Orientation, too
> >   giaco | and aall turtles down ColorSpace and on
> >   giaco | I think I will should to the user "hey your config is adjusted here's your string representation of the new one"
> >   giaco | *shout
> >  jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration
> >         | ?
> >  jmondi | building a proper diff function is not trivial I think
> >   giaco | diff has to be implemented in all the objects required to be diffed
> >   giaco | and share a common structure
> >   giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor
> >  jmondi | we should have somewhere a TODO list for ideas for people to contribute
> >  jmondi | I think we used to
> >   giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable
> >         | number of warnings
> >  jmondi | libcamera can adjust, maybe what it adjusts to is not suitable for the user's final use case, and pipewire/gstreamer/rpi-apps sits in
> >         | between
> >   giaco | how to get a copy of CameraConfiguration before validation?
> >   pobrn | don't
> >   giaco | then it's impossible to track what validate() does
> >   pobrn | you can make copies of the `StreamConfiguration`s
> >   pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of
> >         | `StreamConfiguration`
> >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> >   giaco | I'll do what I can
> >   giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but "libcamerasrc
> >         | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter:<cs> error: Failed to generate camera configuration from roles"
> >   giaco | testing with "GST_DEBUG=2 gst-launch-1.0 libcamerasrc name=cs cs.src ! fakesink cs.src_0 ! fakevideosink" on a camera that supports just
> >         | 1 stream
> >  jmondi | the gstreamer element does not accept less streams than what it had requested
> >  jmondi | if (!config || config->size() != roles.size())
> >  jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno

Hi

2025. 07. 24. 18:32 keltezéssel, Giacomo Cappellini írta:
> The diff part has been suggested by jmondi and pobrn in IRC chat the very same day Jaslo's patch has been posted, also my V1 patch was already just printing the new orientation and a general message for StreamConfigutation(s) and SensorConfiguration. There's no problem in replacing/reverting it and use Jaslo's solution. There are no diffing method on the relevant objects anyway so a clean solution would not be available until then.
> Being this my first patch, what happens now? Should I post a V4 with changes in negotiation function deleted and wait for the merge of the two patches, or should I integrate Jaslo's changes into mine, or vice-versa?

I have to admit that unfortunately I haven't read much of the discussion
regarding this change in detail whether on irc or in email. I was just
trying to help with the concrete question at hand. My intention was not to
suggest that it should be done. Sorry about that!


Regards,
Barnabás Pőcze

> 
> Thanks,
> G.C.
> 
> On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com <mailto:uajain@igalia.com>> wrote:
> 
>     On Wed, Jul 23, 2025 at 04:07:55PM +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
>      >
>      > Update CameraConfiguration::Adjusted case in negotiation to warn about
>      > changes in StreamConfiguration and SensorConfiguration, as well as
>      > the new Orientation parameter.
>      >
>      > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com <mailto:giacomo.cappellini.87@gmail.com>>
>      > ---
>      >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
>      >  src/gstreamer/gstlibcamera-utils.h   |   4 +
>      >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
>      >  3 files changed, 166 insertions(+), 9 deletions(-)
>      >
>      > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
>      > index a548b0c1..95d3813e 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;
>      >
>      > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
>      > +
>      > +     /* Save original configuration for comparison after validation */
>      > +     std::vector<StreamConfiguration> orig_stream_cfgs;
>      > +     for (gsize i = 0; i < state->config_->size(); i++)
>      > +             orig_stream_cfgs.push_back(state->config_->at(i));
>      > +     std::optional<SensorConfiguration> orig_sensor_cfg = state->config_->sensorConfig;
>      > +     Orientation orig_orientation = state->config_->orientation;
>      > +
>      >       /* Validate the configuration. */
>      > -     if (state->config_->validate() == CameraConfiguration::Invalid)
>      > +     switch (state->config_->validate()) {
>      > +     case CameraConfiguration::Valid:
>      > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
>      > +             break;
>      > +     case CameraConfiguration::Adjusted: {
>      > +             bool warned = false;
>      > +             // Warn if number of StreamConfigurations changed
>      > +             if (orig_stream_cfgs.size() != state->config_->size()) {
>      > +                     GST_WARNING_OBJECT(self, "Number of StreamConfiguration elements changed: requested=%zu, actual=%zu",
>      > +                                        orig_stream_cfgs.size(), state->config_->size());
>      > +                     warned = true;
>      > +             }
>      > +             // Warn about changes in each StreamConfiguration
>      > +             // TODO implement diffing in StreamConfiguration
>      > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {
>      > +                     if (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {
>      > +                             GST_WARNING_OBJECT(self, "StreamConfiguration %zu changed: %s -> %s",
>      > +                                                i, orig_stream_cfgs[i].toString().c_str(),
>      > +                                                state->config_->at(i).toString().c_str());
>      > +                             warned = true;
>      > +                     }
>      > +             }
>      > +             // Warn about SensorConfiguration changes
>      > +             // TODO implement diffing in SensorConfiguration
>      > +             if (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {
>      > +                     const SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
>      > +                     const SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;
>      > +                     bool sensor_changed = false;
>      > +                     std::ostringstream diff;
>      > +                     if ((orig == nullptr) != (curr == nullptr)) {
>      > +                             diff << "SensorConfiguration presence changed: "
>      > +                                  << (orig ? "was present" : "was absent")
>      > +                                  << " -> "
>      > +                                  << (curr ? "present" : "absent");
>      > +                             sensor_changed = true;
>      > +                     } else if (orig && curr) {
>      > +                             if (orig->bitDepth != curr->bitDepth) {
>      > +                                     diff << "bitDepth: " << orig->bitDepth << " -> " << curr->bitDepth << "; ";
>      > +                                     sensor_changed = true;
>      > +                             }
>      > +                             if (orig->analogCrop != curr->analogCrop) {
>      > +                                     diff << "analogCrop: " << orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << "; ";
>      > +                                     sensor_changed = true;
>      > +                             }
>      > +                             if (orig->binning.binX != curr->binning.binX ||
>      > +                                 orig->binning.binY != curr->binning.binY) {
>      > +                                     diff << "binning: (" << orig->binning.binX << "," << orig->binning.binY << ") -> ("
>      > +                                          << curr->binning.binX << "," << curr->binning.binY << "); ";
>      > +                                     sensor_changed = true;
>      > +                             }
>      > +                             if (orig->skipping.xOddInc != curr->skipping.xOddInc ||
>      > +                                 orig->skipping.xEvenInc != curr->skipping.xEvenInc ||
>      > +                                 orig->skipping.yOddInc != curr->skipping.yOddInc ||
>      > +                                 orig->skipping.yEvenInc != curr->skipping.yEvenInc) {
>      > +                                     diff << "skipping: ("
>      > +                                          << orig->skipping.xOddInc << "," << orig->skipping.xEvenInc << ","
>      > +                                          << orig->skipping.yOddInc << "," << orig->skipping.yEvenInc << ") -> ("
>      > +                                          << curr->skipping.xOddInc << "," << curr->skipping.xEvenInc << ","
>      > +                                          << curr->skipping.yOddInc << "," << curr->skipping.yEvenInc << "); ";
>      > +                                     sensor_changed = true;
>      > +                             }
>      > +                             if (orig->outputSize != curr->outputSize) {
>      > +                                     diff << "outputSize: " << orig->outputSize.toString() << " -> " << curr->outputSize.toString() << "; ";
>      > +                                     sensor_changed = true;
>      > +                             }
>      > +                     }
>      > +                     if (sensor_changed) {
>      > +                             GST_WARNING_OBJECT(self, "SensorConfiguration changed: %s", diff.str().c_str());
>      > +                             warned = true;
>      > +                     }
>      > +             }
>      > +             // Warn about orientation change
>      > +             if (orig_orientation != state->config_->orientation) {
>      > +                     GEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
>      > +                     const char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
>      > +                     const char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
>      > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s -> %s", orig_orientation_str, new_orientation_str);
>      > +                     warned = true;
>      > +             }
>      > +             if (!warned) {
>      > +                     GST_DEBUG_OBJECT(self, "Camera configuration adjusted, but no significant changes detected.");
>      > +             }
>      > +             // Update Gst orientation property to match adjusted config
>      > +             self->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
>      > +             break;
>      > +     }
> 
>     I am still trying to understand why you need to diff the
>     CameraConfiguration ? Is it just for logging purposes - to warn that the
>     configuration has been changed, in a detailed manner?
> 
>     My goal to pointing you to Jaslo's patch was:
>     a) If the configuration is changed, that patch already logs a warning
>     b) If the the adjusted configuration is not acceptable by downstream
>         elements, the pipeline streaming is rejected.
> 
>     If the configuration is changed, could you not simply report the new
>     orientation in the property?
> 
> 
>      > +     case CameraConfiguration::Invalid:
>      > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
>      > +                               ("Camera configuration is not supported"),
>      > +                               ("CameraConfiguration::validate() returned Invalid"));
>      >               return false;
>      > +     }
>      >
>      >       int ret = state->cam_->configure(state->config_.get());
>      >       if (ret) {
>      > @@ -926,6 +1029,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 +1051,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 +1257,17 @@ 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
>      >
>

Jacopo Mondi,

I'm sorry that my previous email was perceived as an aggressive or
argumentative attack. That was absolutely not my intent, and I
explicitly stated that my aim was to provide a detailed explanation of
my perspective, not to create an argument. I regret that this was not
clear and considered a waste of time.

I genuinely appreciate the support you've provided, specially as I
navigate my first contributions. I understand your frustration and,
while I hope we can move past this misunderstanding, I respect your
decision regarding future support, even if this will make harder for
me to contribute again and experiment with libcamera features.

I've just sent patch V4 based on top of Jaslo's series where his code
takes the lead on case CameraConfiguration::Adjusted.

Regards,

G.C.

Il giorno ven 25 lug 2025 alle ore 12:20 Jacopo Mondi
<jacopo.mondi@ideasonboard.com> ha scritto:
>
> Listen,
>   you can try to put things as you like but
>
> On Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:
> > It's important to note that this is not meant as an argument, but as a
> > detailed explanation of my perspective.
> >
> > I don't agree with your assertion that the CameraConfiguration diffing
> > procedure was "not requested." This contradicts the evidence in the
> > IRC logs provided.
> > The entire premise of tracking configuration adjustments after
> > validate() was to provide meaningful information to users,
> > particularly within frameworks like GStreamer. The conversation
> > explicitly highlights the need to understand "what has changed." The
> > log snippet below is particularly pertinent:
> >
> > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)
> > and notifies you about that, what has changed is up to you to figure
> > out
> >
> > This statement places the responsibility on me to determine what has
> > changed. How is one to "figure out" what has changed without a
> > mechanism to compare the before and after states? The logical and
> > necessary conclusion, which was subsequently discussed, was a diffing
> > procedure.
>
> All of out bindings report "Configuration has been adjusted"
> in example
>
> Android:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734
>
> gstreamer:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620
>
> You decided you want to report "what has changed" nobody asked you to
>
> >
> > Furthermore, my direct question:
> > giaco | build a proper CameraConfiguration diff function, I need to
> > copy it before validation and compare if adjusted
> >
> > was met with:
> >
> > jmondi | patches are welcome
> >
>
> You tell me (imperatively) to "build a proper CameraConfiguration diff
> function" and the only thing I can suggest is that you are free to
> send patches, not because gstreamer need it, because it might be a
> useful addition to libcamera in general (I also suggested to add a
> todo list for contributors for that matter).
>
> nobody asked you to do so. stop putting words in my mouth please.
>
> > This is not a rejection; it is an invitation to develop the
> > functionality. If the intent was truly that this was not requested or
> > desired, that would have been the moment to communicate it, not after.
>
> It's a useful addition to libcamera. Not a requirement for gstreamer.
>
> > To wait until a patch is developed and submitted, only to then claim
> > it was "not requested" is inefficient use of resources. Had this been
> > communicated upfront, I could have focused my efforts elsewhere.
>
> Likewise.
>
> I'll make sure not do waste your time anymore by making sure
> I won't waste mine supporting you like I've always tried to do,
> don't worry.
>
> > If the underlying truth is that is not right time for diffing (as
> > relevant objects do not provide diffing helpers) not the right place
> > (negotiation in gstreamer element) and to move this to a separate
> > task/patch, I agree.
> >
> > Regarding the path forward, I'll post a new version (V4 with cover)
> > based on Jaslo's patch with the diffing functionality removed.
> >
> > G.C.
> >
> > Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> ha scritto:
> > >
> > > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:
> > > > The diff part has been suggested by jmondi and pobrn in IRC chat the very
> > > > same day Jaslo's patch has been posted, also my V1 patch was already just
> > >
> > > Sorry, but that's not what happened
> > >
> > > You suggested we need a CameraConfiguration diff and me and pobrn
> > > tried to suggest how to do it. The fact you need was solely suggested
> > > by you.
> > >
> > >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> > >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> > >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> > >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> > >   giaco | I'll do what I can
> > >
> > > Below the full log [*]
> > >
> > > > printing the new orientation and a general message for
> > > > StreamConfigutation(s) and SensorConfiguration. There's no problem in
> > > > replacing/reverting it and use Jaslo's solution. There are no diffing
> > > > method on the relevant objects anyway so a clean solution would not be
> > > > available until then.
> > > > Being this my first patch, what happens now? Should I post a V4 with
> > > > changes in negotiation function deleted and wait for the merge of the two
> > > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?
> > >
> > > If there's something you find useful in Jaslo's patches rebase your
> > > changes on top of his ones and specify in the cover that your series
> > > depend on his one.
> > >
> > >
> > > >
> > > > Thanks,
> > > > G.C.
> > > >
> > > > On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:
> > > >
> > > > > On Wed, Jul 23, 2025 at 04:07:55PM +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
> > > > > >
> > > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > > > > > changes in StreamConfiguration and SensorConfiguration, as well as
> > > > > > the new Orientation parameter.
> > > > > >
> > > > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > > > > ---
> > > > > >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> > > > > >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> > > > > >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> > > > > >  3 files changed, 166 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > index a548b0c1..95d3813e 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;
> > > > > >
> > > > > > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > > > > > +
> > > > > > +     /* Save original configuration for comparison after validation */
> > > > > > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > > > > > +     for (gsize i = 0; i < state->config_->size(); i++)
> > > > > > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > > > > > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> > > > > state->config_->sensorConfig;
> > > > > > +     Orientation orig_orientation = state->config_->orientation;
> > > > > > +
> > > > > >       /* Validate the configuration. */
> > > > > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > > > > +     switch (state->config_->validate()) {
> > > > > > +     case CameraConfiguration::Valid:
> > > > > > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > > > > > +             break;
> > > > > > +     case CameraConfiguration::Adjusted: {
> > > > > > +             bool warned = false;
> > > > > > +             // Warn if number of StreamConfigurations changed
> > > > > > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > > > > > +                     GST_WARNING_OBJECT(self, "Number of
> > > > > StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > > > > > +                                        orig_stream_cfgs.size(),
> > > > > state->config_->size());
> > > > > > +                     warned = true;
> > > > > > +             }
> > > > > > +             // Warn about changes in each StreamConfiguration
> > > > > > +             // TODO implement diffing in StreamConfiguration
> > > > > > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> > > > > state->config_->size()); i++) {
> > > > > > +                     if (orig_stream_cfgs[i].toString() !=
> > > > > state->config_->at(i).toString()) {
> > > > > > +                             GST_WARNING_OBJECT(self,
> > > > > "StreamConfiguration %zu changed: %s -> %s",
> > > > > > +                                                i,
> > > > > orig_stream_cfgs[i].toString().c_str(),
> > > > > > +
> > > > > state->config_->at(i).toString().c_str());
> > > > > > +                             warned = true;
> > > > > > +                     }
> > > > > > +             }
> > > > > > +             // Warn about SensorConfiguration changes
> > > > > > +             // TODO implement diffing in SensorConfiguration
> > > > > > +             if (orig_sensor_cfg.has_value() ||
> > > > > state->config_->sensorConfig.has_value()) {
> > > > > > +                     const SensorConfiguration *orig =
> > > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > > > > > +                     const SensorConfiguration *curr =
> > > > > state->config_->sensorConfig.has_value() ?
> > > > > &state->config_->sensorConfig.value() : nullptr;
> > > > > > +                     bool sensor_changed = false;
> > > > > > +                     std::ostringstream diff;
> > > > > > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > > > > > +                             diff << "SensorConfiguration presence
> > > > > changed: "
> > > > > > +                                  << (orig ? "was present" : "was
> > > > > absent")
> > > > > > +                                  << " -> "
> > > > > > +                                  << (curr ? "present" : "absent");
> > > > > > +                             sensor_changed = true;
> > > > > > +                     } else if (orig && curr) {
> > > > > > +                             if (orig->bitDepth != curr->bitDepth) {
> > > > > > +                                     diff << "bitDepth: " <<
> > > > > orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > > > > > +                                     sensor_changed = true;
> > > > > > +                             }
> > > > > > +                             if (orig->analogCrop != curr->analogCrop) {
> > > > > > +                                     diff << "analogCrop: " <<
> > > > > orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> > > > > ";
> > > > > > +                                     sensor_changed = true;
> > > > > > +                             }
> > > > > > +                             if (orig->binning.binX !=
> > > > > curr->binning.binX ||
> > > > > > +                                 orig->binning.binY !=
> > > > > curr->binning.binY) {
> > > > > > +                                     diff << "binning: (" <<
> > > > > orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > > > > > +                                          << curr->binning.binX << ","
> > > > > << curr->binning.binY << "); ";
> > > > > > +                                     sensor_changed = true;
> > > > > > +                             }
> > > > > > +                             if (orig->skipping.xOddInc !=
> > > > > curr->skipping.xOddInc ||
> > > > > > +                                 orig->skipping.xEvenInc !=
> > > > > curr->skipping.xEvenInc ||
> > > > > > +                                 orig->skipping.yOddInc !=
> > > > > curr->skipping.yOddInc ||
> > > > > > +                                 orig->skipping.yEvenInc !=
> > > > > curr->skipping.yEvenInc) {
> > > > > > +                                     diff << "skipping: ("
> > > > > > +                                          << orig->skipping.xOddInc <<
> > > > > "," << orig->skipping.xEvenInc << ","
> > > > > > +                                          << orig->skipping.yOddInc <<
> > > > > "," << orig->skipping.yEvenInc << ") -> ("
> > > > > > +                                          << curr->skipping.xOddInc <<
> > > > > "," << curr->skipping.xEvenInc << ","
> > > > > > +                                          << curr->skipping.yOddInc <<
> > > > > "," << curr->skipping.yEvenInc << "); ";
> > > > > > +                                     sensor_changed = true;
> > > > > > +                             }
> > > > > > +                             if (orig->outputSize != curr->outputSize) {
> > > > > > +                                     diff << "outputSize: " <<
> > > > > orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> > > > > ";
> > > > > > +                                     sensor_changed = true;
> > > > > > +                             }
> > > > > > +                     }
> > > > > > +                     if (sensor_changed) {
> > > > > > +                             GST_WARNING_OBJECT(self,
> > > > > "SensorConfiguration changed: %s", diff.str().c_str());
> > > > > > +                             warned = true;
> > > > > > +                     }
> > > > > > +             }
> > > > > > +             // Warn about orientation change
> > > > > > +             if (orig_orientation != state->config_->orientation) {
> > > > > > +                     GEnumClass *enum_class = (GEnumClass
> > > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > > > > > +                     const char *orig_orientation_str =
> > > > > g_enum_get_value(enum_class,
> > > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > > > > > +                     const char *new_orientation_str =
> > > > > g_enum_get_value(enum_class,
> > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > > > > > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> > > > > -> %s", orig_orientation_str, new_orientation_str);
> > > > > > +                     warned = true;
> > > > > > +             }
> > > > > > +             if (!warned) {
> > > > > > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> > > > > adjusted, but no significant changes detected.");
> > > > > > +             }
> > > > > > +             // Update Gst orientation property to match adjusted config
> > > > > > +             self->orientation =
> > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > > > > > +             break;
> > > > > > +     }
> > > > >
> > > > > I am still trying to understand why you need to diff the
> > > > > CameraConfiguration ? Is it just for logging purposes - to warn that the
> > > > > configuration has been changed, in a detailed manner?
> > > > >
> > > > > My goal to pointing you to Jaslo's patch was:
> > > > > a) If the configuration is changed, that patch already logs a warning
> > > > > b) If the the adjusted configuration is not acceptable by downstream
> > > > >    elements, the pipeline streaming is rejected.
> > > > >
> > > > > If the configuration is changed, could you not simply report the new
> > > > > orientation in the property?
> > > > >
> > > > >
> > > > > > +     case CameraConfiguration::Invalid:
> > > > > > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > > > +                               ("Camera configuration is not
> > > > > supported"),
> > > > > > +                               ("CameraConfiguration::validate()
> > > > > returned Invalid"));
> > > > > >               return false;
> > > > > > +     }
> > > > > >
> > > > > >       int ret = state->cam_->configure(state->config_.get());
> > > > > >       if (ret) {
> > > > > > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> > > > > 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
> > > > > >
> > > > >
> > >
> > > [*]
> > >
> > >   giaco | I'm doing the "track which CameraConfiguration attributes have been adjusted after validate()". How do I know what can be changed and
> > >         | what not?
> > >   giaco | it seems something validate() should do, not the caller
> > >  jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out
> > >  jmondi | it boils down to a few things, streams' sizes and formats and orientation
> > >   giaco | can I assume that the total number of requested streams remains the same?
> > >  jmondi | no
> > >   giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?
> > >  jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415
> > >  jmondi | not larger, the pipeline can reduce it to match the number of streams it supports
> > >  jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust
> > >  jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)
> > >   giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted
> > >  jmondi | patches are welcome
> > >   giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,
> > >         | SensorConfiguration and Orientation, too
> > >   giaco | and aall turtles down ColorSpace and on
> > >   giaco | I think I will should to the user "hey your config is adjusted here's your string representation of the new one"
> > >   giaco | *shout
> > >  jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration
> > >         | ?
> > >  jmondi | building a proper diff function is not trivial I think
> > >   giaco | diff has to be implemented in all the objects required to be diffed
> > >   giaco | and share a common structure
> > >   giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor
> > >  jmondi | we should have somewhere a TODO list for ideas for people to contribute
> > >  jmondi | I think we used to
> > >   giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable
> > >         | number of warnings
> > >  jmondi | libcamera can adjust, maybe what it adjusts to is not suitable for the user's final use case, and pipewire/gstreamer/rpi-apps sits in
> > >         | between
> > >   giaco | how to get a copy of CameraConfiguration before validation?
> > >   pobrn | don't
> > >   giaco | then it's impossible to track what validate() does
> > >   pobrn | you can make copies of the `StreamConfiguration`s
> > >   pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of
> > >         | `StreamConfiguration`
> > >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> > >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> > >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> > >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> > >   giaco | I'll do what I can
> > >   giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but "libcamerasrc
> > >         | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter:<cs> error: Failed to generate camera configuration from roles"
> > >   giaco | testing with "GST_DEBUG=2 gst-launch-1.0 libcamerasrc name=cs cs.src ! fakesink cs.src_0 ! fakevideosink" on a camera that supports just
> > >         | 1 stream
> > >  jmondi | the gstreamer element does not accept less streams than what it had requested
> > >  jmondi | if (!config || config->size() != roles.size())
> > >  jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno

Hi Barnabás,

Thanks for your message and for clarifying. I appreciate you stepping
in to help with the concrete question. No worries at all about the
misunderstanding; it's acknowledged.

Thanks again,
G.C.

Il giorno ven 25 lug 2025 alle ore 13:34 Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> ha scritto:
>
> Hi
>
> 2025. 07. 24. 18:32 keltezéssel, Giacomo Cappellini írta:
> > The diff part has been suggested by jmondi and pobrn in IRC chat the very same day Jaslo's patch has been posted, also my V1 patch was already just printing the new orientation and a general message for StreamConfigutation(s) and SensorConfiguration. There's no problem in replacing/reverting it and use Jaslo's solution. There are no diffing method on the relevant objects anyway so a clean solution would not be available until then.
> > Being this my first patch, what happens now? Should I post a V4 with changes in negotiation function deleted and wait for the merge of the two patches, or should I integrate Jaslo's changes into mine, or vice-versa?
>
> I have to admit that unfortunately I haven't read much of the discussion
> regarding this change in detail whether on irc or in email. I was just
> trying to help with the concrete question at hand. My intention was not to
> suggest that it should be done. Sorry about that!
>
>
> Regards,
> Barnabás Pőcze
>
> >
> > Thanks,
> > G.C.
> >
> > On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com <mailto:uajain@igalia.com>> wrote:
> >
> >     On Wed, Jul 23, 2025 at 04:07:55PM +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
> >      >
> >      > Update CameraConfiguration::Adjusted case in negotiation to warn about
> >      > changes in StreamConfiguration and SensorConfiguration, as well as
> >      > the new Orientation parameter.
> >      >
> >      > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com <mailto:giacomo.cappellini.87@gmail.com>>
> >      > ---
> >      >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> >      >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> >      >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> >      >  3 files changed, 166 insertions(+), 9 deletions(-)
> >      >
> >      > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> >      > index a548b0c1..95d3813e 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;
> >      >
> >      > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> >      > +
> >      > +     /* Save original configuration for comparison after validation */
> >      > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> >      > +     for (gsize i = 0; i < state->config_->size(); i++)
> >      > +             orig_stream_cfgs.push_back(state->config_->at(i));
> >      > +     std::optional<SensorConfiguration> orig_sensor_cfg = state->config_->sensorConfig;
> >      > +     Orientation orig_orientation = state->config_->orientation;
> >      > +
> >      >       /* Validate the configuration. */
> >      > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> >      > +     switch (state->config_->validate()) {
> >      > +     case CameraConfiguration::Valid:
> >      > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> >      > +             break;
> >      > +     case CameraConfiguration::Adjusted: {
> >      > +             bool warned = false;
> >      > +             // Warn if number of StreamConfigurations changed
> >      > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> >      > +                     GST_WARNING_OBJECT(self, "Number of StreamConfiguration elements changed: requested=%zu, actual=%zu",
> >      > +                                        orig_stream_cfgs.size(), state->config_->size());
> >      > +                     warned = true;
> >      > +             }
> >      > +             // Warn about changes in each StreamConfiguration
> >      > +             // TODO implement diffing in StreamConfiguration
> >      > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {
> >      > +                     if (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {
> >      > +                             GST_WARNING_OBJECT(self, "StreamConfiguration %zu changed: %s -> %s",
> >      > +                                                i, orig_stream_cfgs[i].toString().c_str(),
> >      > +                                                state->config_->at(i).toString().c_str());
> >      > +                             warned = true;
> >      > +                     }
> >      > +             }
> >      > +             // Warn about SensorConfiguration changes
> >      > +             // TODO implement diffing in SensorConfiguration
> >      > +             if (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {
> >      > +                     const SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> >      > +                     const SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;
> >      > +                     bool sensor_changed = false;
> >      > +                     std::ostringstream diff;
> >      > +                     if ((orig == nullptr) != (curr == nullptr)) {
> >      > +                             diff << "SensorConfiguration presence changed: "
> >      > +                                  << (orig ? "was present" : "was absent")
> >      > +                                  << " -> "
> >      > +                                  << (curr ? "present" : "absent");
> >      > +                             sensor_changed = true;
> >      > +                     } else if (orig && curr) {
> >      > +                             if (orig->bitDepth != curr->bitDepth) {
> >      > +                                     diff << "bitDepth: " << orig->bitDepth << " -> " << curr->bitDepth << "; ";
> >      > +                                     sensor_changed = true;
> >      > +                             }
> >      > +                             if (orig->analogCrop != curr->analogCrop) {
> >      > +                                     diff << "analogCrop: " << orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << "; ";
> >      > +                                     sensor_changed = true;
> >      > +                             }
> >      > +                             if (orig->binning.binX != curr->binning.binX ||
> >      > +                                 orig->binning.binY != curr->binning.binY) {
> >      > +                                     diff << "binning: (" << orig->binning.binX << "," << orig->binning.binY << ") -> ("
> >      > +                                          << curr->binning.binX << "," << curr->binning.binY << "); ";
> >      > +                                     sensor_changed = true;
> >      > +                             }
> >      > +                             if (orig->skipping.xOddInc != curr->skipping.xOddInc ||
> >      > +                                 orig->skipping.xEvenInc != curr->skipping.xEvenInc ||
> >      > +                                 orig->skipping.yOddInc != curr->skipping.yOddInc ||
> >      > +                                 orig->skipping.yEvenInc != curr->skipping.yEvenInc) {
> >      > +                                     diff << "skipping: ("
> >      > +                                          << orig->skipping.xOddInc << "," << orig->skipping.xEvenInc << ","
> >      > +                                          << orig->skipping.yOddInc << "," << orig->skipping.yEvenInc << ") -> ("
> >      > +                                          << curr->skipping.xOddInc << "," << curr->skipping.xEvenInc << ","
> >      > +                                          << curr->skipping.yOddInc << "," << curr->skipping.yEvenInc << "); ";
> >      > +                                     sensor_changed = true;
> >      > +                             }
> >      > +                             if (orig->outputSize != curr->outputSize) {
> >      > +                                     diff << "outputSize: " << orig->outputSize.toString() << " -> " << curr->outputSize.toString() << "; ";
> >      > +                                     sensor_changed = true;
> >      > +                             }
> >      > +                     }
> >      > +                     if (sensor_changed) {
> >      > +                             GST_WARNING_OBJECT(self, "SensorConfiguration changed: %s", diff.str().c_str());
> >      > +                             warned = true;
> >      > +                     }
> >      > +             }
> >      > +             // Warn about orientation change
> >      > +             if (orig_orientation != state->config_->orientation) {
> >      > +                     GEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> >      > +                     const char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> >      > +                     const char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> >      > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s -> %s", orig_orientation_str, new_orientation_str);
> >      > +                     warned = true;
> >      > +             }
> >      > +             if (!warned) {
> >      > +                     GST_DEBUG_OBJECT(self, "Camera configuration adjusted, but no significant changes detected.");
> >      > +             }
> >      > +             // Update Gst orientation property to match adjusted config
> >      > +             self->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> >      > +             break;
> >      > +     }
> >
> >     I am still trying to understand why you need to diff the
> >     CameraConfiguration ? Is it just for logging purposes - to warn that the
> >     configuration has been changed, in a detailed manner?
> >
> >     My goal to pointing you to Jaslo's patch was:
> >     a) If the configuration is changed, that patch already logs a warning
> >     b) If the the adjusted configuration is not acceptable by downstream
> >         elements, the pipeline streaming is rejected.
> >
> >     If the configuration is changed, could you not simply report the new
> >     orientation in the property?
> >
> >
> >      > +     case CameraConfiguration::Invalid:
> >      > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> >      > +                               ("Camera configuration is not supported"),
> >      > +                               ("CameraConfiguration::validate() returned Invalid"));
> >      >               return false;
> >      > +     }
> >      >
> >      >       int ret = state->cam_->configure(state->config_.get());
> >      >       if (ret) {
> >      > @@ -926,6 +1029,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 +1051,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 +1257,17 @@ 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 Giacomo

On Fri, Jul 25, 2025 at 02:15:10PM +0200, Giacomo Cappellini wrote:
> Jacopo Mondi,
>
> I'm sorry that my previous email was perceived as an aggressive or
> argumentative attack. That was absolutely not my intent, and I
> explicitly stated that my aim was to provide a detailed explanation of
> my perspective, not to create an argument. I regret that this was not
> clear and considered a waste of time.
>
> I genuinely appreciate the support you've provided, specially as I
> navigate my first contributions. I understand your frustration and,
> while I hope we can move past this misunderstanding, I respect your
> decision regarding future support, even if this will make harder for
> me to contribute again and experiment with libcamera features.

Thanks for your reconciling email and allow me to say I'm sorry if my
message was too aggressive.

As always, getting a conversation tone from writing is complex, and
here we're mixing emails and irc logs. I'm sorry if I've
mis-interpreted your message.

Thank you again for your interest and contributions to the project.

>
> I've just sent patch V4 based on top of Jaslo's series where his code
> takes the lead on case CameraConfiguration::Adjusted.
>
> Regards,
>
> G.C.
>
> Il giorno ven 25 lug 2025 alle ore 12:20 Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> ha scritto:
> >
> > Listen,
> >   you can try to put things as you like but
> >
> > On Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:
> > > It's important to note that this is not meant as an argument, but as a
> > > detailed explanation of my perspective.
> > >
> > > I don't agree with your assertion that the CameraConfiguration diffing
> > > procedure was "not requested." This contradicts the evidence in the
> > > IRC logs provided.
> > > The entire premise of tracking configuration adjustments after
> > > validate() was to provide meaningful information to users,
> > > particularly within frameworks like GStreamer. The conversation
> > > explicitly highlights the need to understand "what has changed." The
> > > log snippet below is particularly pertinent:
> > >
> > > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)
> > > and notifies you about that, what has changed is up to you to figure
> > > out
> > >
> > > This statement places the responsibility on me to determine what has
> > > changed. How is one to "figure out" what has changed without a
> > > mechanism to compare the before and after states? The logical and
> > > necessary conclusion, which was subsequently discussed, was a diffing
> > > procedure.
> >
> > All of out bindings report "Configuration has been adjusted"
> > in example
> >
> > Android:
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734
> >
> > gstreamer:
> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620
> >
> > You decided you want to report "what has changed" nobody asked you to
> >
> > >
> > > Furthermore, my direct question:
> > > giaco | build a proper CameraConfiguration diff function, I need to
> > > copy it before validation and compare if adjusted
> > >
> > > was met with:
> > >
> > > jmondi | patches are welcome
> > >
> >
> > You tell me (imperatively) to "build a proper CameraConfiguration diff
> > function" and the only thing I can suggest is that you are free to
> > send patches, not because gstreamer need it, because it might be a
> > useful addition to libcamera in general (I also suggested to add a
> > todo list for contributors for that matter).
> >
> > nobody asked you to do so. stop putting words in my mouth please.
> >
> > > This is not a rejection; it is an invitation to develop the
> > > functionality. If the intent was truly that this was not requested or
> > > desired, that would have been the moment to communicate it, not after.
> >
> > It's a useful addition to libcamera. Not a requirement for gstreamer.
> >
> > > To wait until a patch is developed and submitted, only to then claim
> > > it was "not requested" is inefficient use of resources. Had this been
> > > communicated upfront, I could have focused my efforts elsewhere.
> >
> > Likewise.
> >
> > I'll make sure not do waste your time anymore by making sure
> > I won't waste mine supporting you like I've always tried to do,
> > don't worry.
> >
> > > If the underlying truth is that is not right time for diffing (as
> > > relevant objects do not provide diffing helpers) not the right place
> > > (negotiation in gstreamer element) and to move this to a separate
> > > task/patch, I agree.
> > >
> > > Regarding the path forward, I'll post a new version (V4 with cover)
> > > based on Jaslo's patch with the diffing functionality removed.
> > >
> > > G.C.
> > >
> > > Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi
> > > <jacopo.mondi@ideasonboard.com> ha scritto:
> > > >
> > > > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:
> > > > > The diff part has been suggested by jmondi and pobrn in IRC chat the very
> > > > > same day Jaslo's patch has been posted, also my V1 patch was already just
> > > >
> > > > Sorry, but that's not what happened
> > > >
> > > > You suggested we need a CameraConfiguration diff and me and pobrn
> > > > tried to suggest how to do it. The fact you need was solely suggested
> > > > by you.
> > > >
> > > >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> > > >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> > > >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> > > >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> > > >   giaco | I'll do what I can
> > > >
> > > > Below the full log [*]
> > > >
> > > > > printing the new orientation and a general message for
> > > > > StreamConfigutation(s) and SensorConfiguration. There's no problem in
> > > > > replacing/reverting it and use Jaslo's solution. There are no diffing
> > > > > method on the relevant objects anyway so a clean solution would not be
> > > > > available until then.
> > > > > Being this my first patch, what happens now? Should I post a V4 with
> > > > > changes in negotiation function deleted and wait for the merge of the two
> > > > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?
> > > >
> > > > If there's something you find useful in Jaslo's patches rebase your
> > > > changes on top of his ones and specify in the cover that your series
> > > > depend on his one.
> > > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > G.C.
> > > > >
> > > > > On Thu, Jul 24, 2025, 05:37 Umang Jain <uajain@igalia.com> wrote:
> > > > >
> > > > > > On Wed, Jul 23, 2025 at 04:07:55PM +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
> > > > > > >
> > > > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about
> > > > > > > changes in StreamConfiguration and SensorConfiguration, as well as
> > > > > > > the new Orientation parameter.
> > > > > > >
> > > > > > > Signed-off-by: Giacomo Cappellini <giacomo.cappellini.87@gmail.com>
> > > > > > > ---
> > > > > > >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++
> > > > > > >  src/gstreamer/gstlibcamera-utils.h   |   4 +
> > > > > > >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--
> > > > > > >  3 files changed, 166 insertions(+), 9 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > b/src/gstreamer/gstlibcamera-utils.cpp
> > > > > > > index a548b0c1..95d3813e 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;
> > > > > > >
> > > > > > > @@ -659,3 +662,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..5f483701 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,109 @@ 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);
> > > > > > > +
> > > > > > > +     /* Save original configuration for comparison after validation */
> > > > > > > +     std::vector<StreamConfiguration> orig_stream_cfgs;
> > > > > > > +     for (gsize i = 0; i < state->config_->size(); i++)
> > > > > > > +             orig_stream_cfgs.push_back(state->config_->at(i));
> > > > > > > +     std::optional<SensorConfiguration> orig_sensor_cfg =
> > > > > > state->config_->sensorConfig;
> > > > > > > +     Orientation orig_orientation = state->config_->orientation;
> > > > > > > +
> > > > > > >       /* Validate the configuration. */
> > > > > > > -     if (state->config_->validate() == CameraConfiguration::Invalid)
> > > > > > > +     switch (state->config_->validate()) {
> > > > > > > +     case CameraConfiguration::Valid:
> > > > > > > +             GST_DEBUG_OBJECT(self, "Camera configuration is valid");
> > > > > > > +             break;
> > > > > > > +     case CameraConfiguration::Adjusted: {
> > > > > > > +             bool warned = false;
> > > > > > > +             // Warn if number of StreamConfigurations changed
> > > > > > > +             if (orig_stream_cfgs.size() != state->config_->size()) {
> > > > > > > +                     GST_WARNING_OBJECT(self, "Number of
> > > > > > StreamConfiguration elements changed: requested=%zu, actual=%zu",
> > > > > > > +                                        orig_stream_cfgs.size(),
> > > > > > state->config_->size());
> > > > > > > +                     warned = true;
> > > > > > > +             }
> > > > > > > +             // Warn about changes in each StreamConfiguration
> > > > > > > +             // TODO implement diffing in StreamConfiguration
> > > > > > > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),
> > > > > > state->config_->size()); i++) {
> > > > > > > +                     if (orig_stream_cfgs[i].toString() !=
> > > > > > state->config_->at(i).toString()) {
> > > > > > > +                             GST_WARNING_OBJECT(self,
> > > > > > "StreamConfiguration %zu changed: %s -> %s",
> > > > > > > +                                                i,
> > > > > > orig_stream_cfgs[i].toString().c_str(),
> > > > > > > +
> > > > > > state->config_->at(i).toString().c_str());
> > > > > > > +                             warned = true;
> > > > > > > +                     }
> > > > > > > +             }
> > > > > > > +             // Warn about SensorConfiguration changes
> > > > > > > +             // TODO implement diffing in SensorConfiguration
> > > > > > > +             if (orig_sensor_cfg.has_value() ||
> > > > > > state->config_->sensorConfig.has_value()) {
> > > > > > > +                     const SensorConfiguration *orig =
> > > > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;
> > > > > > > +                     const SensorConfiguration *curr =
> > > > > > state->config_->sensorConfig.has_value() ?
> > > > > > &state->config_->sensorConfig.value() : nullptr;
> > > > > > > +                     bool sensor_changed = false;
> > > > > > > +                     std::ostringstream diff;
> > > > > > > +                     if ((orig == nullptr) != (curr == nullptr)) {
> > > > > > > +                             diff << "SensorConfiguration presence
> > > > > > changed: "
> > > > > > > +                                  << (orig ? "was present" : "was
> > > > > > absent")
> > > > > > > +                                  << " -> "
> > > > > > > +                                  << (curr ? "present" : "absent");
> > > > > > > +                             sensor_changed = true;
> > > > > > > +                     } else if (orig && curr) {
> > > > > > > +                             if (orig->bitDepth != curr->bitDepth) {
> > > > > > > +                                     diff << "bitDepth: " <<
> > > > > > orig->bitDepth << " -> " << curr->bitDepth << "; ";
> > > > > > > +                                     sensor_changed = true;
> > > > > > > +                             }
> > > > > > > +                             if (orig->analogCrop != curr->analogCrop) {
> > > > > > > +                                     diff << "analogCrop: " <<
> > > > > > orig->analogCrop.toString() << " -> " << curr->analogCrop.toString() << ";
> > > > > > ";
> > > > > > > +                                     sensor_changed = true;
> > > > > > > +                             }
> > > > > > > +                             if (orig->binning.binX !=
> > > > > > curr->binning.binX ||
> > > > > > > +                                 orig->binning.binY !=
> > > > > > curr->binning.binY) {
> > > > > > > +                                     diff << "binning: (" <<
> > > > > > orig->binning.binX << "," << orig->binning.binY << ") -> ("
> > > > > > > +                                          << curr->binning.binX << ","
> > > > > > << curr->binning.binY << "); ";
> > > > > > > +                                     sensor_changed = true;
> > > > > > > +                             }
> > > > > > > +                             if (orig->skipping.xOddInc !=
> > > > > > curr->skipping.xOddInc ||
> > > > > > > +                                 orig->skipping.xEvenInc !=
> > > > > > curr->skipping.xEvenInc ||
> > > > > > > +                                 orig->skipping.yOddInc !=
> > > > > > curr->skipping.yOddInc ||
> > > > > > > +                                 orig->skipping.yEvenInc !=
> > > > > > curr->skipping.yEvenInc) {
> > > > > > > +                                     diff << "skipping: ("
> > > > > > > +                                          << orig->skipping.xOddInc <<
> > > > > > "," << orig->skipping.xEvenInc << ","
> > > > > > > +                                          << orig->skipping.yOddInc <<
> > > > > > "," << orig->skipping.yEvenInc << ") -> ("
> > > > > > > +                                          << curr->skipping.xOddInc <<
> > > > > > "," << curr->skipping.xEvenInc << ","
> > > > > > > +                                          << curr->skipping.yOddInc <<
> > > > > > "," << curr->skipping.yEvenInc << "); ";
> > > > > > > +                                     sensor_changed = true;
> > > > > > > +                             }
> > > > > > > +                             if (orig->outputSize != curr->outputSize) {
> > > > > > > +                                     diff << "outputSize: " <<
> > > > > > orig->outputSize.toString() << " -> " << curr->outputSize.toString() << ";
> > > > > > ";
> > > > > > > +                                     sensor_changed = true;
> > > > > > > +                             }
> > > > > > > +                     }
> > > > > > > +                     if (sensor_changed) {
> > > > > > > +                             GST_WARNING_OBJECT(self,
> > > > > > "SensorConfiguration changed: %s", diff.str().c_str());
> > > > > > > +                             warned = true;
> > > > > > > +                     }
> > > > > > > +             }
> > > > > > > +             // Warn about orientation change
> > > > > > > +             if (orig_orientation != state->config_->orientation) {
> > > > > > > +                     GEnumClass *enum_class = (GEnumClass
> > > > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);
> > > > > > > +                     const char *orig_orientation_str =
> > > > > > g_enum_get_value(enum_class,
> > > > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;
> > > > > > > +                     const char *new_orientation_str =
> > > > > > g_enum_get_value(enum_class,
> > > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;
> > > > > > > +                     GST_WARNING_OBJECT(self, "Orientation changed: %s
> > > > > > -> %s", orig_orientation_str, new_orientation_str);
> > > > > > > +                     warned = true;
> > > > > > > +             }
> > > > > > > +             if (!warned) {
> > > > > > > +                     GST_DEBUG_OBJECT(self, "Camera configuration
> > > > > > adjusted, but no significant changes detected.");
> > > > > > > +             }
> > > > > > > +             // Update Gst orientation property to match adjusted config
> > > > > > > +             self->orientation =
> > > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);
> > > > > > > +             break;
> > > > > > > +     }
> > > > > >
> > > > > > I am still trying to understand why you need to diff the
> > > > > > CameraConfiguration ? Is it just for logging purposes - to warn that the
> > > > > > configuration has been changed, in a detailed manner?
> > > > > >
> > > > > > My goal to pointing you to Jaslo's patch was:
> > > > > > a) If the configuration is changed, that patch already logs a warning
> > > > > > b) If the the adjusted configuration is not acceptable by downstream
> > > > > >    elements, the pipeline streaming is rejected.
> > > > > >
> > > > > > If the configuration is changed, could you not simply report the new
> > > > > > orientation in the property?
> > > > > >
> > > > > >
> > > > > > > +     case CameraConfiguration::Invalid:
> > > > > > > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,
> > > > > > > +                               ("Camera configuration is not
> > > > > > supported"),
> > > > > > > +                               ("CameraConfiguration::validate()
> > > > > > returned Invalid"));
> > > > > > >               return false;
> > > > > > > +     }
> > > > > > >
> > > > > > >       int ret = state->cam_->configure(state->config_.get());
> > > > > > >       if (ret) {
> > > > > > > @@ -926,6 +1029,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 +1051,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 +1257,17 @@
> > > > > > 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
> > > > > > >
> > > > > >
> > > >
> > > > [*]
> > > >
> > > >   giaco | I'm doing the "track which CameraConfiguration attributes have been adjusted after validate()". How do I know what can be changed and
> > > >         | what not?
> > > >   giaco | it seems something validate() should do, not the caller
> > > >  jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out
> > > >  jmondi | it boils down to a few things, streams' sizes and formats and orientation
> > > >   giaco | can I assume that the total number of requested streams remains the same?
> > > >  jmondi | no
> > > >   giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?
> > > >  jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415
> > > >  jmondi | not larger, the pipeline can reduce it to match the number of streams it supports
> > > >  jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust
> > > >  jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)
> > > >   giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted
> > > >  jmondi | patches are welcome
> > > >   giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,
> > > >         | SensorConfiguration and Orientation, too
> > > >   giaco | and aall turtles down ColorSpace and on
> > > >   giaco | I think I will should to the user "hey your config is adjusted here's your string representation of the new one"
> > > >   giaco | *shout
> > > >  jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration
> > > >         | ?
> > > >  jmondi | building a proper diff function is not trivial I think
> > > >   giaco | diff has to be implemented in all the objects required to be diffed
> > > >   giaco | and share a common structure
> > > >   giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor
> > > >  jmondi | we should have somewhere a TODO list for ideas for people to contribute
> > > >  jmondi | I think we used to
> > > >   giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable
> > > >         | number of warnings
> > > >  jmondi | libcamera can adjust, maybe what it adjusts to is not suitable for the user's final use case, and pipewire/gstreamer/rpi-apps sits in
> > > >         | between
> > > >   giaco | how to get a copy of CameraConfiguration before validation?
> > > >   pobrn | don't
> > > >   giaco | then it's impossible to track what validate() does
> > > >   pobrn | you can make copies of the `StreamConfiguration`s
> > > >   pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of
> > > >         | `StreamConfiguration`
> > > >   giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted
> > > >   giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable
> > > >   giaco | it seems quite not the moment to provide to users such information considering the lack of support for this in the config validation api
> > > >   pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation
> > > >   giaco | I'll do what I can
> > > >   giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but "libcamerasrc
> > > >         | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter:<cs> error: Failed to generate camera configuration from roles"
> > > >   giaco | testing with "GST_DEBUG=2 gst-launch-1.0 libcamerasrc name=cs cs.src ! fakesink cs.src_0 ! fakevideosink" on a camera that supports just
> > > >         | 1 stream
> > > >  jmondi | the gstreamer element does not accept less streams than what it had requested
> > > >  jmondi | if (!config || config->size() != roles.size())
> > > >  jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno