[{"id":35067,"web_url":"https://patchwork.libcamera.org/comment/35067/","msgid":"","date":"2025-07-24T03:37:12","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> libcamera allows to control the images orientation through the\n> CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> control it. Parameter is mapped internally to libcamera::Orientation via\n> new gstlibcamera-utils functions:\n> - gst_video_orientation_to_libcamera_orientation\n> - libcamera_orientation_to_gst_video_orientation\n> \n> Update CameraConfiguration::Adjusted case in negotiation to warn about\n> changes in StreamConfiguration and SensorConfiguration, as well as\n> the new Orientation parameter.\n> \n> Signed-off-by: Giacomo Cappellini \n> ---\n> src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> src/gstreamer/gstlibcamera-utils.h | 4 +\n> src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> 3 files changed, 166 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index a548b0c1..95d3813e 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -10,6 +10,9 @@\n> \n> #include \n> #include \n> +#include \n> +\n> +#include \n> \n> using namespace libcamera;\n> \n> @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> \n> \treturn cm;\n> }\n> +\n> +static const struct {\n> +\tOrientation orientation;\n> +\tGstVideoOrientationMethod method;\n> +} orientation_map[]{\n> +\t{ Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> +\t{ Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> +\t{ Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> +\t{ Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> +\t{ Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> +\t{ Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> +\t{ Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> +\t{ Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> +};\n> +\n> +Orientation\n> +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method)\n> +{\n> +\tfor (auto &b : orientation_map) {\n> +\t\tif (b.method == method)\n> +\t\t\treturn b.orientation;\n> +\t}\n> +\n> +\treturn Orientation::Rotate0;\n> +}\n> +\n> +GstVideoOrientationMethod\n> +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> +{\n> +\tfor (auto &a : orientation_map) {\n> +\t\tif (a.orientation == orientation)\n> +\t\t\treturn a.method;\n> +\t}\n> +\n> +\treturn GST_VIDEO_ORIENTATION_IDENTITY;\n> +}\n> diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> index 5f4e8a0f..bbbd33db 100644\n> --- a/src/gstreamer/gstlibcamera-utils.h\n> +++ b/src/gstreamer/gstlibcamera-utils.h\n> @@ -10,6 +10,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -92,3 +93,6 @@ public:\n> private:\n> \tGRecMutex *mutex_;\n> };\n> +\n> +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);\n> +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 3aca4eed..5f483701 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -29,6 +29,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -38,6 +39,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \"gstlibcamera-controls.h\"\n> #include \"gstlibcamera-utils.h\"\n> @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> \tGstTask *task;\n> \n> \tgchar *camera_name;\n> +\tGstVideoOrientationMethod orientation;\n> \n> \tstd::atomic pending_eos;\n> \n> @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> enum {\n> \tPROP_0,\n> \tPROP_CAMERA_NAME,\n> +\tPROP_ORIENTATION,\n> \tPROP_LAST\n> };\n> \n> @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> \t\t\tG_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> \t\t\t\t\t gst_libcamera_src_child_proxy_init)\n> -\t\t\tGST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> -\t\t\t\t\t\t\"libcamera Source\"))\n> +\t\t\t\tGST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> +\t\t\t\t\t\t\t\"libcamera Source\"))\n> \n> #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg; video/x-bayer\")\n> \n> @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> \treturn 0;\n> }\n> \n> -void\n> -GstLibcameraSrcState::requestCompleted(Request *request)\n> +void GstLibcameraSrcState::requestCompleted(Request *request)\n> {\n> \tGST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> \n> @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> \t\tgst_libcamera_get_framerate_from_caps(caps, element_caps);\n> \t}\n> \n> +\t/* Set orientation control. */\n> +\tstate->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);\n> +\n> +\t/* Save original configuration for comparison after validation */\n> +\tstd::vector orig_stream_cfgs;\n> +\tfor (gsize i = 0; i < state->config_->size(); i++)\n> +\t\torig_stream_cfgs.push_back(state->config_->at(i));\n> +\tstd::optional orig_sensor_cfg = state->config_->sensorConfig;\n> +\tOrientation orig_orientation = state->config_->orientation;\n> +\n> \t/* Validate the configuration. */\n> -\tif (state->config_->validate() == CameraConfiguration::Invalid)\n> +\tswitch (state->config_->validate()) {\n> +\tcase CameraConfiguration::Valid:\n> +\t\tGST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> +\t\tbreak;\n> +\tcase CameraConfiguration::Adjusted: {\n> +\t\tbool warned = false;\n> +\t\t// Warn if number of StreamConfigurations changed\n> +\t\tif (orig_stream_cfgs.size() != state->config_->size()) {\n> +\t\t\tGST_WARNING_OBJECT(self, \"Number of StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> +\t\t\t\t\t orig_stream_cfgs.size(), state->config_->size());\n> +\t\t\twarned = true;\n> +\t\t}\n> +\t\t// Warn about changes in each StreamConfiguration\n> +\t\t// TODO implement diffing in StreamConfiguration\n> +\t\tfor (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {\n> +\t\t\tif (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {\n> +\t\t\t\tGST_WARNING_OBJECT(self, \"StreamConfiguration %zu changed: %s -> %s\",\n> +\t\t\t\t\t\t i, orig_stream_cfgs[i].toString().c_str(),\n> +\t\t\t\t\t\t state->config_->at(i).toString().c_str());\n> +\t\t\t\twarned = true;\n> +\t\t\t}\n> +\t\t}\n> +\t\t// Warn about SensorConfiguration changes\n> +\t\t// TODO implement diffing in SensorConfiguration\n> +\t\tif (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {\n> +\t\t\tconst SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> +\t\t\tconst SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;\n> +\t\t\tbool sensor_changed = false;\n> +\t\t\tstd::ostringstream diff;\n> +\t\t\tif ((orig == nullptr) != (curr == nullptr)) {\n> +\t\t\t\tdiff << \"SensorConfiguration presence changed: \"\n> +\t\t\t\t << (orig ? \"was present\" : \"was absent\")\n> +\t\t\t\t << \" -> \"\n> +\t\t\t\t << (curr ? \"present\" : \"absent\");\n> +\t\t\t\tsensor_changed = true;\n> +\t\t\t} else if (orig && curr) {\n> +\t\t\t\tif (orig->bitDepth != curr->bitDepth) {\n> +\t\t\t\t\tdiff << \"bitDepth: \" << orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> +\t\t\t\t\tsensor_changed = true;\n> +\t\t\t\t}\n> +\t\t\t\tif (orig->analogCrop != curr->analogCrop) {\n> +\t\t\t\t\tdiff << \"analogCrop: \" << orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \"; \";\n> +\t\t\t\t\tsensor_changed = true;\n> +\t\t\t\t}\n> +\t\t\t\tif (orig->binning.binX != curr->binning.binX ||\n> +\t\t\t\t orig->binning.binY != curr->binning.binY) {\n> +\t\t\t\t\tdiff << \"binning: (\" << orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> +\t\t\t\t\t << curr->binning.binX << \",\" << curr->binning.binY << \"); \";\n> +\t\t\t\t\tsensor_changed = true;\n> +\t\t\t\t}\n> +\t\t\t\tif (orig->skipping.xOddInc != curr->skipping.xOddInc ||\n> +\t\t\t\t orig->skipping.xEvenInc != curr->skipping.xEvenInc ||\n> +\t\t\t\t orig->skipping.yOddInc != curr->skipping.yOddInc ||\n> +\t\t\t\t orig->skipping.yEvenInc != curr->skipping.yEvenInc) {\n> +\t\t\t\t\tdiff << \"skipping: (\"\n> +\t\t\t\t\t << orig->skipping.xOddInc << \",\" << orig->skipping.xEvenInc << \",\"\n> +\t\t\t\t\t << orig->skipping.yOddInc << \",\" << orig->skipping.yEvenInc << \") -> (\"\n> +\t\t\t\t\t << curr->skipping.xOddInc << \",\" << curr->skipping.xEvenInc << \",\"\n> +\t\t\t\t\t << curr->skipping.yOddInc << \",\" << curr->skipping.yEvenInc << \"); \";\n> +\t\t\t\t\tsensor_changed = true;\n> +\t\t\t\t}\n> +\t\t\t\tif (orig->outputSize != curr->outputSize) {\n> +\t\t\t\t\tdiff << \"outputSize: \" << orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \"; \";\n> +\t\t\t\t\tsensor_changed = true;\n> +\t\t\t\t}\n> +\t\t\t}\n> +\t\t\tif (sensor_changed) {\n> +\t\t\t\tGST_WARNING_OBJECT(self, \"SensorConfiguration changed: %s\", diff.str().c_str());\n> +\t\t\t\twarned = true;\n> +\t\t\t}\n> +\t\t}\n> +\t\t// Warn about orientation change\n> +\t\tif (orig_orientation != state->config_->orientation) {\n> +\t\t\tGEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> +\t\t\tconst char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> +\t\t\tconst char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> +\t\t\tGST_WARNING_OBJECT(self, \"Orientation changed: %s -> %s\", orig_orientation_str, new_orientation_str);\n> +\t\t\twarned = true;\n> +\t\t}\n> +\t\tif (!warned) {\n> +\t\t\tGST_DEBUG_OBJECT(self, \"Camera configuration adjusted, but no significant changes detected.\");\n> +\t\t}\n> +\t\t// Update Gst orientation property to match adjusted config\n> +\t\tself->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> +\t\tbreak;\n> +\t}\n\nI am still trying to understand why you need to diff the\nCameraConfiguration ? Is it just for logging purposes - to warn that the\nconfiguration has been changed, in a detailed manner? \n\nMy goal to pointing you to Jaslo's patch was:\na) If the configuration is changed, that patch already logs a warning\nb) If the the adjusted configuration is not acceptable by downstream\n elements, the pipeline streaming is rejected.\n\nIf the configuration is changed, could you not simply report the new\norientation in the property? \n\n\n> +\tcase CameraConfiguration::Invalid:\n> +\t\tGST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> +\t\t\t\t (\"Camera configuration is not supported\"),\n> +\t\t\t\t (\"CameraConfiguration::validate() returned Invalid\"));\n> \t\treturn false;\n> +\t}\n> \n> \tint ret = state->cam_->configure(state->config_.get());\n> \tif (ret) {\n> @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> \t\tg_free(self->camera_name);\n> \t\tself->camera_name = g_value_dup_string(value);\n> \t\tbreak;\n> +\tcase PROP_ORIENTATION:\n> +\t\tself->orientation = (GstVideoOrientationMethod)g_value_get_enum(value);\n> +\t\tbreak;\n> \tdefault:\n> \t\tif (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))\n> \t\t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> \tcase PROP_CAMERA_NAME:\n> \t\tg_value_set_string(value, self->camera_name);\n> \t\tbreak;\n> +\tcase PROP_ORIENTATION:\n> +\t\tg_value_set_enum(value, (gint)self->orientation);\n> +\t\tbreak;\n> \tdefault:\n> \t\tif (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))\n> \t\t\tG_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> @@ -1148,12 +1257,17 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> \n> \tGParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera Name\",\n> \t\t\t\t\t \"Select by name which camera to use.\", nullptr,\n> -\t\t\t\t\t (GParamFlags)(GST_PARAM_MUTABLE_READY\n> -\t\t\t\t\t\t\t | G_PARAM_CONSTRUCT\n> -\t\t\t\t\t\t\t | G_PARAM_READWRITE\n> -\t\t\t\t\t\t\t | G_PARAM_STATIC_STRINGS));\n> +\t\t\t\t\t (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> \tg_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> \n> +\t/* Register the orientation enum type. */\n> +\tspec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> +\t\t\t\t \"Select the orientation of the camera.\",\n> +\t\t\t\t GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> +\t\t\t\t GST_VIDEO_ORIENTATION_IDENTITY,\n> +\t\t\t\t (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> +\tg_object_class_install_property(object_class, PROP_ORIENTATION, spec);\n> +\n> \tGstCameraControls::installProperties(object_class, PROP_LAST);\n> }\n> \n> -- \n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C4BB6C3237\n\tfor ;\n\tThu, 24 Jul 2025 03:37:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA21C69078;\n\tThu, 24 Jul 2025 05:37:11 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E8B7469078\n\tfor ;\n\tThu, 24 Jul 2025 05:37:09 +0200 (CEST)","from [49.36.71.87] (helo=uajain) by fanzine2.igalia.com with\n\tesmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uemlX-002xNT-5R; Thu, 24 Jul 2025 05:37:08 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"T5Rmgxsr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=Q+H/uEbOWSnH7mJssO65UIzt1Zwx+R09cwUP1KjG31U=;\n\tb=T5Rmgxsr+HeA/acFooSqQ9Gy4z\n\tLguKSV0hcdT1ARHipDiGQpQe3ATJW3uufvU9+K34hF1Cb3Bd9ge/9xTqnB3EN/6xPraCMhc+W8Z4R\n\t76/bV1ClOd3hqFxaXr1Ne/SStxo+arYglcJnGc3DbjOuhbkemc2gotcANRyKPWl3BSv0Zeo/DBBzW\n\tXC5qMDhoUDe/JrN/7YDsgT3bphIGLqELy+nzRUt4daRzz3GGriARAzWEBHwHwKIMaUQZHyW9yGIru\n\tQAnKnSsshN5Ef+U6S8bm0HoxL+TmXsv1xc+3lpV68QPaRQTZqvaWDqcTwHnQRdwDoXYVcD0lH5+9P\n\tx8DWiihw==;","Date":"Thu, 24 Jul 2025 09:07:12 +0530","From":"Umang Jain ","To":"Giacomo Cappellini ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","Message-ID":"","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>","User-Agent":"NeoMutt/20250510-dirty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35105,"web_url":"https://patchwork.libcamera.org/comment/35105/","msgid":"","date":"2025-07-24T16:32:29","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":231,"url":"https://patchwork.libcamera.org/api/people/231/","name":"Giacomo Cappellini","email":"giacomo.cappellini.87@gmail.com"},"content":"The diff part has been suggested by jmondi and pobrn in IRC chat the very\nsame day Jaslo's patch has been posted, also my V1 patch was already just\nprinting the new orientation and a general message for\nStreamConfigutation(s) and SensorConfiguration. There's no problem in\nreplacing/reverting it and use Jaslo's solution. There are no diffing\nmethod on the relevant objects anyway so a clean solution would not be\navailable until then.\nBeing this my first patch, what happens now? Should I post a V4 with\nchanges in negotiation function deleted and wait for the merge of the two\npatches, or should I integrate Jaslo's changes into mine, or vice-versa?\n\nThanks,\nG.C.\n\nOn Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n\n> On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n\n> libcamera allows to control the images orientation through the\n> > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > control it. Parameter is mapped internally to libcamera::Orientation via\n> > new gstlibcamera-utils functions:\n> > - gst_video_orientation_to_libcamera_orientation\n> > - libcamera_orientation_to_gst_video_orientation\n> >\n> > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > changes in StreamConfiguration and SensorConfiguration, as well as\n> > the new Orientation parameter.\n> >\n> > Signed-off-by: Giacomo Cappellini \n> > ---\n> > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > 3 files changed, 166 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> b/src/gstreamer/gstlibcamera-utils.cpp\n> > index a548b0c1..95d3813e 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -10,6 +10,9 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > +\n> > +#include \n> >\n> > using namespace libcamera;\n> >\n> > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> >\n> > return cm;\n> > }\n> > +\n> > +static const struct {\n> > + Orientation orientation;\n> > + GstVideoOrientationMethod method;\n> > +} orientation_map[]{\n> > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > +};\n> > +\n> > +Orientation\n> >\n> +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> method)\n> > +{\n> > + for (auto &b : orientation_map) {\n> > + if (b.method == method)\n> > + return b.orientation;\n> > + }\n> > +\n> > + return Orientation::Rotate0;\n> > +}\n> > +\n> > +GstVideoOrientationMethod\n> > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > +{\n> > + for (auto &a : orientation_map) {\n> > + if (a.orientation == orientation)\n> > + return a.method;\n> > + }\n> > +\n> > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> b/src/gstreamer/gstlibcamera-utils.h\n> > index 5f4e8a0f..bbbd33db 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.h\n> > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > @@ -10,6 +10,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -92,3 +93,6 @@ public:\n> > private:\n> > GRecMutex *mutex_;\n> > };\n> > +\n> > +libcamera::Orientation\n> gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> method);\n> > +GstVideoOrientationMethod\n> libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> orientation);\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 3aca4eed..5f483701 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -29,6 +29,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -38,6 +39,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \"gstlibcamera-controls.h\"\n> > #include \"gstlibcamera-utils.h\"\n> > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > GstTask *task;\n> >\n> > gchar *camera_name;\n> > + GstVideoOrientationMethod orientation;\n> >\n> > std::atomic pending_eos;\n> >\n> > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > enum {\n> > PROP_0,\n> > PROP_CAMERA_NAME,\n> > + PROP_ORIENTATION,\n> > PROP_LAST\n> > };\n> >\n> > @@ -166,8 +170,8 @@ static void\n> gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> GST_TYPE_ELEMENT,\n> > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> >\n> gst_libcamera_src_child_proxy_init)\n> > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> \"libcamerasrc\", 0,\n> > - \"libcamera Source\"))\n> > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> \"libcamerasrc\", 0,\n> > + \"libcamera\n> Source\"))\n> >\n> > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> video/x-bayer\")\n> >\n> > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > return 0;\n> > }\n> >\n> > -void\n> > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > {\n> > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> >\n> > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > }\n> >\n> > + /* Set orientation control. */\n> > + state->config_->orientation =\n> gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > +\n> > + /* Save original configuration for comparison after validation */\n> > + std::vector orig_stream_cfgs;\n> > + for (gsize i = 0; i < state->config_->size(); i++)\n> > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > + std::optional orig_sensor_cfg =\n> state->config_->sensorConfig;\n> > + Orientation orig_orientation = state->config_->orientation;\n> > +\n> > /* Validate the configuration. */\n> > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > + switch (state->config_->validate()) {\n> > + case CameraConfiguration::Valid:\n> > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > + break;\n> > + case CameraConfiguration::Adjusted: {\n> > + bool warned = false;\n> > + // Warn if number of StreamConfigurations changed\n> > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > + GST_WARNING_OBJECT(self, \"Number of\n> StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > + orig_stream_cfgs.size(),\n> state->config_->size());\n> > + warned = true;\n> > + }\n> > + // Warn about changes in each StreamConfiguration\n> > + // TODO implement diffing in StreamConfiguration\n> > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> state->config_->size()); i++) {\n> > + if (orig_stream_cfgs[i].toString() !=\n> state->config_->at(i).toString()) {\n> > + GST_WARNING_OBJECT(self,\n> \"StreamConfiguration %zu changed: %s -> %s\",\n> > + i,\n> orig_stream_cfgs[i].toString().c_str(),\n> > +\n> state->config_->at(i).toString().c_str());\n> > + warned = true;\n> > + }\n> > + }\n> > + // Warn about SensorConfiguration changes\n> > + // TODO implement diffing in SensorConfiguration\n> > + if (orig_sensor_cfg.has_value() ||\n> state->config_->sensorConfig.has_value()) {\n> > + const SensorConfiguration *orig =\n> orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > + const SensorConfiguration *curr =\n> state->config_->sensorConfig.has_value() ?\n> &state->config_->sensorConfig.value() : nullptr;\n> > + bool sensor_changed = false;\n> > + std::ostringstream diff;\n> > + if ((orig == nullptr) != (curr == nullptr)) {\n> > + diff << \"SensorConfiguration presence\n> changed: \"\n> > + << (orig ? \"was present\" : \"was\n> absent\")\n> > + << \" -> \"\n> > + << (curr ? \"present\" : \"absent\");\n> > + sensor_changed = true;\n> > + } else if (orig && curr) {\n> > + if (orig->bitDepth != curr->bitDepth) {\n> > + diff << \"bitDepth: \" <<\n> orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > + sensor_changed = true;\n> > + }\n> > + if (orig->analogCrop != curr->analogCrop) {\n> > + diff << \"analogCrop: \" <<\n> orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> \";\n> > + sensor_changed = true;\n> > + }\n> > + if (orig->binning.binX !=\n> curr->binning.binX ||\n> > + orig->binning.binY !=\n> curr->binning.binY) {\n> > + diff << \"binning: (\" <<\n> orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > + << curr->binning.binX << \",\"\n> << curr->binning.binY << \"); \";\n> > + sensor_changed = true;\n> > + }\n> > + if (orig->skipping.xOddInc !=\n> curr->skipping.xOddInc ||\n> > + orig->skipping.xEvenInc !=\n> curr->skipping.xEvenInc ||\n> > + orig->skipping.yOddInc !=\n> curr->skipping.yOddInc ||\n> > + orig->skipping.yEvenInc !=\n> curr->skipping.yEvenInc) {\n> > + diff << \"skipping: (\"\n> > + << orig->skipping.xOddInc <<\n> \",\" << orig->skipping.xEvenInc << \",\"\n> > + << orig->skipping.yOddInc <<\n> \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > + << curr->skipping.xOddInc <<\n> \",\" << curr->skipping.xEvenInc << \",\"\n> > + << curr->skipping.yOddInc <<\n> \",\" << curr->skipping.yEvenInc << \"); \";\n> > + sensor_changed = true;\n> > + }\n> > + if (orig->outputSize != curr->outputSize) {\n> > + diff << \"outputSize: \" <<\n> orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> \";\n> > + sensor_changed = true;\n> > + }\n> > + }\n> > + if (sensor_changed) {\n> > + GST_WARNING_OBJECT(self,\n> \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > + warned = true;\n> > + }\n> > + }\n> > + // Warn about orientation change\n> > + if (orig_orientation != state->config_->orientation) {\n> > + GEnumClass *enum_class = (GEnumClass\n> *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > + const char *orig_orientation_str =\n> g_enum_get_value(enum_class,\n> libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > + const char *new_orientation_str =\n> g_enum_get_value(enum_class,\n> libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> -> %s\", orig_orientation_str, new_orientation_str);\n> > + warned = true;\n> > + }\n> > + if (!warned) {\n> > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> adjusted, but no significant changes detected.\");\n> > + }\n> > + // Update Gst orientation property to match adjusted config\n> > + self->orientation =\n> libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > + break;\n> > + }\n>\n> I am still trying to understand why you need to diff the\n> CameraConfiguration ? Is it just for logging purposes - to warn that the\n> configuration has been changed, in a detailed manner?\n>\n> My goal to pointing you to Jaslo's patch was:\n> a) If the configuration is changed, that patch already logs a warning\n> b) If the the adjusted configuration is not acceptable by downstream\n> elements, the pipeline streaming is rejected.\n>\n> If the configuration is changed, could you not simply report the new\n> orientation in the property?\n>\n>\n> > + case CameraConfiguration::Invalid:\n> > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > + (\"Camera configuration is not\n> supported\"),\n> > + (\"CameraConfiguration::validate()\n> returned Invalid\"));\n> > return false;\n> > + }\n> >\n> > int ret = state->cam_->configure(state->config_.get());\n> > if (ret) {\n> > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> guint prop_id,\n> > g_free(self->camera_name);\n> > self->camera_name = g_value_dup_string(value);\n> > break;\n> > + case PROP_ORIENTATION:\n> > + self->orientation =\n> (GstVideoOrientationMethod)g_value_get_enum(value);\n> > + break;\n> > default:\n> > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> value, pspec))\n> > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> pspec);\n> > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> guint prop_id, GValue *value,\n> > case PROP_CAMERA_NAME:\n> > g_value_set_string(value, self->camera_name);\n> > break;\n> > + case PROP_ORIENTATION:\n> > + g_value_set_enum(value, (gint)self->orientation);\n> > + break;\n> > default:\n> > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> value, pspec))\n> > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> pspec);\n> > @@ -1148,12 +1257,17 @@\n> gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >\n> > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> Name\",\n> > \"Select by name which\n> camera to use.\", nullptr,\n> > -\n> (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > - |\n> G_PARAM_CONSTRUCT\n> > - |\n> G_PARAM_READWRITE\n> > - |\n> G_PARAM_STATIC_STRINGS));\n> > +\n> (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> spec);\n> >\n> > + /* Register the orientation enum type. */\n> > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > + \"Select the orientation of the camera.\",\n> > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> spec);\n> > +\n> > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > }\n> >\n> > --\n> > 2.43.0\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B9168BDCC1\n\tfor ;\n\tThu, 24 Jul 2025 16:32:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 803206907F;\n\tThu, 24 Jul 2025 18:32:45 +0200 (CEST)","from mail-il1-x134.google.com (mail-il1-x134.google.com\n\t[IPv6:2607:f8b0:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD8A161508\n\tfor ;\n\tThu, 24 Jul 2025 18:32:42 +0200 (CEST)","by mail-il1-x134.google.com with SMTP id\n\te9e14a558f8ab-3e29ee0faf3so4172415ab.2\n\tfor ;\n\tThu, 24 Jul 2025 09:32:42 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"RSEWPzA9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1753374761; x=1753979561;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=XdLL/P29eD7+oVfSdwBbeDVAZTCWmLw/Rm78DrO/QVM=;\n\tb=RSEWPzA9mSeVHDroHqsA7bn6J7IEuLsvYphu2c71C5qOQBGndX5kG+OF0yZCq2LLXD\n\ttBr4W5vYtSyUZhzYoyM8VC5rTyzrKUTQFjT60U3SSqXyiIe6Qf20SMfqB1OYmWnCAQTF\n\t+JNanfxEcXEZDIKAve0r8Q8ye14iqe302K44uyarcgOEygfDwUJzzADA5W9j3FiHk0ff\n\tOOTxpEWaC9RqEdL4m3rLKelW7HZOulQ/3eJqo8IuXH5mes7E6UDeBxktPQkstL2QNf6h\n\t3KD5qFX/amlXq+NdscTQeaA6pH21anF6ypsVdN1pZiX+x2peNL6lDNOqk8p2ce5mlOEg\n\tvZLA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1753374761; x=1753979561;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=XdLL/P29eD7+oVfSdwBbeDVAZTCWmLw/Rm78DrO/QVM=;\n\tb=dunysvMYmYt6JFDZWiOhpndJXEspfS4ZiHshx8od3GLnCnDfLCP9Da4Lod/J/2J0MJ\n\teRuLusrUD81ibfxZ4Q+BhMitPEBowRErtKWDYcd1neHcOItR2AYuO729hn3uT+Z6VKcU\n\t86NjmYav/7GGZXrDN0DENoY7a6z48OdaReR3vA4pZFN7BUDYmPTQBx04jnA31upSXa60\n\ts9jCnsctHVrIoHHhWKMmZKWzM77ijMKKKM/5C2udqyenE7/1OaOZbUBIvVhVBAibasHE\n\tgDZyrAddtB71Bsb/coefZQ3k7u5akoSTaD2CcwZf8tupN733wVO3tQ7yGdUo04zK9Jm5\n\tXXJQ==","X-Gm-Message-State":"AOJu0YzMPmh1ETWH14IkpiDkw8sv/gXTp1lMVDnvjCh9pqZGK3cUb+hH\n\tnN6nhHFGGN0cDIw8yHrvMrxONcES95FDOmkeoH1Fwok4Rq/ZfZGfd+lFo5+/z105fo01D/7xhQe\n\tgAIyQ3/e3eSaATVWHsZ8G3vTFzs/r6Us=","X-Gm-Gg":"ASbGnctZ99Xp/fE4G7KTdXDFaW/K/Qu7Wai+3zqwVrpWpanWd43HL2Rdxeq2oy0ZoKY\n\tkY2RtNOmBkLqQCf0FA+Yzvhu4MgE7g7BoVn/OWiU0KIaZ7rbFB1fvxv+3GHiZOUj4+myPBQMPDj\n\t9Gz9g+a9zUAtfdSTu9dlnPNekCYV4RAnbs/dmY4NmohncYT+ihXT8dmkSFMs28xHph1fA9VXO30\n\txsid43s","X-Google-Smtp-Source":"AGHT+IGNSaINYZendj/kEb8kDwZputDkhveI1rFnk2iKYez2INgvhazYwJzX6sV9kntdL8O4S1H99D1c+x0okOQlb4I=","X-Received":"by 2002:a05:6e02:350c:b0:3e2:9fa5:d4a5 with SMTP id\n\te9e14a558f8ab-3e32fdb98e2mr107717735ab.15.1753374761203;\n\tThu, 24 Jul 2025 09:32:41 -0700 (PDT)","MIME-Version":"1.0","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t","In-Reply-To":"","From":"Giacomo Cappellini ","Date":"Thu, 24 Jul 2025 18:32:29 +0200","X-Gm-Features":"Ac12FXwIJ4NIhgQpmqJTd94HJp1gURt3qBIWGXBbYMYXUiRKA2Yc6BS5T2G1ccc","Message-ID":"","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"multipart/alternative; boundary=\"000000000000ccc1e0063aaf5f5c\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35114,"web_url":"https://patchwork.libcamera.org/comment/35114/","msgid":"","date":"2025-07-25T07:32:44","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:\n> The diff part has been suggested by jmondi and pobrn in IRC chat the very\n> same day Jaslo's patch has been posted, also my V1 patch was already just\n\nSorry, but that's not what happened\n\nYou suggested we need a CameraConfiguration diff and me and pobrn\ntried to suggest how to do it. The fact you need was solely suggested\nby you.\n\n giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n 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\n pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n giaco | I'll do what I can\n\nBelow the full log [*]\n\n> printing the new orientation and a general message for\n> StreamConfigutation(s) and SensorConfiguration. There's no problem in\n> replacing/reverting it and use Jaslo's solution. There are no diffing\n> method on the relevant objects anyway so a clean solution would not be\n> available until then.\n> Being this my first patch, what happens now? Should I post a V4 with\n> changes in negotiation function deleted and wait for the merge of the two\n> patches, or should I integrate Jaslo's changes into mine, or vice-versa?\n\nIf there's something you find useful in Jaslo's patches rebase your\nchanges on top of his ones and specify in the cover that your series\ndepend on his one.\n\n\n>\n> Thanks,\n> G.C.\n>\n> On Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n>\n> > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n>\n> > libcamera allows to control the images orientation through the\n> > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > new gstlibcamera-utils functions:\n> > > - gst_video_orientation_to_libcamera_orientation\n> > > - libcamera_orientation_to_gst_video_orientation\n> > >\n> > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > the new Orientation parameter.\n> > >\n> > > Signed-off-by: Giacomo Cappellini \n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > index a548b0c1..95d3813e 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > @@ -10,6 +10,9 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > +\n> > > +#include \n> > >\n> > > using namespace libcamera;\n> > >\n> > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > >\n> > > return cm;\n> > > }\n> > > +\n> > > +static const struct {\n> > > + Orientation orientation;\n> > > + GstVideoOrientationMethod method;\n> > > +} orientation_map[]{\n> > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > +};\n> > > +\n> > > +Orientation\n> > >\n> > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > method)\n> > > +{\n> > > + for (auto &b : orientation_map) {\n> > > + if (b.method == method)\n> > > + return b.orientation;\n> > > + }\n> > > +\n> > > + return Orientation::Rotate0;\n> > > +}\n> > > +\n> > > +GstVideoOrientationMethod\n> > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > +{\n> > > + for (auto &a : orientation_map) {\n> > > + if (a.orientation == orientation)\n> > > + return a.method;\n> > > + }\n> > > +\n> > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> > b/src/gstreamer/gstlibcamera-utils.h\n> > > index 5f4e8a0f..bbbd33db 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > @@ -10,6 +10,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -92,3 +93,6 @@ public:\n> > > private:\n> > > GRecMutex *mutex_;\n> > > };\n> > > +\n> > > +libcamera::Orientation\n> > gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > method);\n> > > +GstVideoOrientationMethod\n> > libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> > orientation);\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 3aca4eed..5f483701 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -29,6 +29,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -38,6 +39,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \"gstlibcamera-controls.h\"\n> > > #include \"gstlibcamera-utils.h\"\n> > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > GstTask *task;\n> > >\n> > > gchar *camera_name;\n> > > + GstVideoOrientationMethod orientation;\n> > >\n> > > std::atomic pending_eos;\n> > >\n> > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > enum {\n> > > PROP_0,\n> > > PROP_CAMERA_NAME,\n> > > + PROP_ORIENTATION,\n> > > PROP_LAST\n> > > };\n> > >\n> > > @@ -166,8 +170,8 @@ static void\n> > gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> > GST_TYPE_ELEMENT,\n> > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > >\n> > gst_libcamera_src_child_proxy_init)\n> > > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> > \"libcamerasrc\", 0,\n> > > - \"libcamera Source\"))\n> > > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> > \"libcamerasrc\", 0,\n> > > + \"libcamera\n> > Source\"))\n> > >\n> > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> > video/x-bayer\")\n> > >\n> > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > return 0;\n> > > }\n> > >\n> > > -void\n> > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > {\n> > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > >\n> > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > }\n> > >\n> > > + /* Set orientation control. */\n> > > + state->config_->orientation =\n> > gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > +\n> > > + /* Save original configuration for comparison after validation */\n> > > + std::vector orig_stream_cfgs;\n> > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > + std::optional orig_sensor_cfg =\n> > state->config_->sensorConfig;\n> > > + Orientation orig_orientation = state->config_->orientation;\n> > > +\n> > > /* Validate the configuration. */\n> > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > + switch (state->config_->validate()) {\n> > > + case CameraConfiguration::Valid:\n> > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > + break;\n> > > + case CameraConfiguration::Adjusted: {\n> > > + bool warned = false;\n> > > + // Warn if number of StreamConfigurations changed\n> > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > + GST_WARNING_OBJECT(self, \"Number of\n> > StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > + orig_stream_cfgs.size(),\n> > state->config_->size());\n> > > + warned = true;\n> > > + }\n> > > + // Warn about changes in each StreamConfiguration\n> > > + // TODO implement diffing in StreamConfiguration\n> > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> > state->config_->size()); i++) {\n> > > + if (orig_stream_cfgs[i].toString() !=\n> > state->config_->at(i).toString()) {\n> > > + GST_WARNING_OBJECT(self,\n> > \"StreamConfiguration %zu changed: %s -> %s\",\n> > > + i,\n> > orig_stream_cfgs[i].toString().c_str(),\n> > > +\n> > state->config_->at(i).toString().c_str());\n> > > + warned = true;\n> > > + }\n> > > + }\n> > > + // Warn about SensorConfiguration changes\n> > > + // TODO implement diffing in SensorConfiguration\n> > > + if (orig_sensor_cfg.has_value() ||\n> > state->config_->sensorConfig.has_value()) {\n> > > + const SensorConfiguration *orig =\n> > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > + const SensorConfiguration *curr =\n> > state->config_->sensorConfig.has_value() ?\n> > &state->config_->sensorConfig.value() : nullptr;\n> > > + bool sensor_changed = false;\n> > > + std::ostringstream diff;\n> > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > + diff << \"SensorConfiguration presence\n> > changed: \"\n> > > + << (orig ? \"was present\" : \"was\n> > absent\")\n> > > + << \" -> \"\n> > > + << (curr ? \"present\" : \"absent\");\n> > > + sensor_changed = true;\n> > > + } else if (orig && curr) {\n> > > + if (orig->bitDepth != curr->bitDepth) {\n> > > + diff << \"bitDepth: \" <<\n> > orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->analogCrop != curr->analogCrop) {\n> > > + diff << \"analogCrop: \" <<\n> > orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> > \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->binning.binX !=\n> > curr->binning.binX ||\n> > > + orig->binning.binY !=\n> > curr->binning.binY) {\n> > > + diff << \"binning: (\" <<\n> > orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > + << curr->binning.binX << \",\"\n> > << curr->binning.binY << \"); \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->skipping.xOddInc !=\n> > curr->skipping.xOddInc ||\n> > > + orig->skipping.xEvenInc !=\n> > curr->skipping.xEvenInc ||\n> > > + orig->skipping.yOddInc !=\n> > curr->skipping.yOddInc ||\n> > > + orig->skipping.yEvenInc !=\n> > curr->skipping.yEvenInc) {\n> > > + diff << \"skipping: (\"\n> > > + << orig->skipping.xOddInc <<\n> > \",\" << orig->skipping.xEvenInc << \",\"\n> > > + << orig->skipping.yOddInc <<\n> > \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > + << curr->skipping.xOddInc <<\n> > \",\" << curr->skipping.xEvenInc << \",\"\n> > > + << curr->skipping.yOddInc <<\n> > \",\" << curr->skipping.yEvenInc << \"); \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->outputSize != curr->outputSize) {\n> > > + diff << \"outputSize: \" <<\n> > orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> > \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + }\n> > > + if (sensor_changed) {\n> > > + GST_WARNING_OBJECT(self,\n> > \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > + warned = true;\n> > > + }\n> > > + }\n> > > + // Warn about orientation change\n> > > + if (orig_orientation != state->config_->orientation) {\n> > > + GEnumClass *enum_class = (GEnumClass\n> > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > + const char *orig_orientation_str =\n> > g_enum_get_value(enum_class,\n> > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > + const char *new_orientation_str =\n> > g_enum_get_value(enum_class,\n> > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> > -> %s\", orig_orientation_str, new_orientation_str);\n> > > + warned = true;\n> > > + }\n> > > + if (!warned) {\n> > > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> > adjusted, but no significant changes detected.\");\n> > > + }\n> > > + // Update Gst orientation property to match adjusted config\n> > > + self->orientation =\n> > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > + break;\n> > > + }\n> >\n> > I am still trying to understand why you need to diff the\n> > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > configuration has been changed, in a detailed manner?\n> >\n> > My goal to pointing you to Jaslo's patch was:\n> > a) If the configuration is changed, that patch already logs a warning\n> > b) If the the adjusted configuration is not acceptable by downstream\n> > elements, the pipeline streaming is rejected.\n> >\n> > If the configuration is changed, could you not simply report the new\n> > orientation in the property?\n> >\n> >\n> > > + case CameraConfiguration::Invalid:\n> > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > + (\"Camera configuration is not\n> > supported\"),\n> > > + (\"CameraConfiguration::validate()\n> > returned Invalid\"));\n> > > return false;\n> > > + }\n> > >\n> > > int ret = state->cam_->configure(state->config_.get());\n> > > if (ret) {\n> > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> > guint prop_id,\n> > > g_free(self->camera_name);\n> > > self->camera_name = g_value_dup_string(value);\n> > > break;\n> > > + case PROP_ORIENTATION:\n> > > + self->orientation =\n> > (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > + break;\n> > > default:\n> > > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> > value, pspec))\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > pspec);\n> > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> > guint prop_id, GValue *value,\n> > > case PROP_CAMERA_NAME:\n> > > g_value_set_string(value, self->camera_name);\n> > > break;\n> > > + case PROP_ORIENTATION:\n> > > + g_value_set_enum(value, (gint)self->orientation);\n> > > + break;\n> > > default:\n> > > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> > value, pspec))\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > pspec);\n> > > @@ -1148,12 +1257,17 @@\n> > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > >\n> > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> > Name\",\n> > > \"Select by name which\n> > camera to use.\", nullptr,\n> > > -\n> > (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > - |\n> > G_PARAM_CONSTRUCT\n> > > - |\n> > G_PARAM_READWRITE\n> > > - |\n> > G_PARAM_STATIC_STRINGS));\n> > > +\n> > (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> > spec);\n> > >\n> > > + /* Register the orientation enum type. */\n> > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > + \"Select the orientation of the camera.\",\n> > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> > G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> > spec);\n> > > +\n> > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > }\n> > >\n> > > --\n> > > 2.43.0\n> > >\n> >\n\n[*]\n\n giaco | I'm doing the \"track which CameraConfiguration attributes have been adjusted after validate()\". How do I know what can be changed and\n | what not?\n giaco | it seems something validate() should do, not the caller\n jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out\n jmondi | it boils down to a few things, streams' sizes and formats and orientation\n giaco | can I assume that the total number of requested streams remains the same?\n jmondi | no\n giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?\n jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415\n jmondi | not larger, the pipeline can reduce it to match the number of streams it supports\n jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust\n jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)\n giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted\n jmondi | patches are welcome\n giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,\n | SensorConfiguration and Orientation, too\n giaco | and aall turtles down ColorSpace and on\n giaco | I think I will should to the user \"hey your config is adjusted here's your string representation of the new one\"\n giaco | *shout\n jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration\n | ?\n jmondi | building a proper diff function is not trivial I think\n giaco | diff has to be implemented in all the objects required to be diffed\n giaco | and share a common structure\n giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor\n jmondi | we should have somewhere a TODO list for ideas for people to contribute\n jmondi | I think we used to\n giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable\n | number of warnings\n 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\n | between\n giaco | how to get a copy of CameraConfiguration before validation?\n pobrn | don't\n giaco | then it's impossible to track what validate() does\n pobrn | you can make copies of the `StreamConfiguration`s\n pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of\n | `StreamConfiguration`\n giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n 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\n pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n giaco | I'll do what I can\n giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but \"libcamerasrc\n | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter: error: Failed to generate camera configuration from roles\"\n 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\n | 1 stream\n jmondi | the gstreamer element does not accept less streams than what it had requested\n jmondi | if (!config || config->size() != roles.size())\n jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0AD33BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 07:32:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D57196908B;\n\tFri, 25 Jul 2025 09:32:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADADC69080\n\tfor ;\n\tFri, 25 Jul 2025 09:32:49 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 479A59CE;\n\tFri, 25 Jul 2025 09:32:09 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Q83DNaNZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753428730;\n\tbh=G2QC23sgIRf+LDN0TMFXobyCX2kBPADC3lnhpVlv5ec=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Q83DNaNZFvjcgsVX39j3qnTqkREfRf7UqbYpevm8P6LurrscW42z8a+1zAZEvUVMC\n\tdNUGnjjg/0PjoTdMMQNj4H0AEwYBR0pYVMg1kxfLWY2ZvIoq7Cv7Xac5hdAetaJqOH\n\tei86SLGkN7jAqHiUkQBQV2OLvQF3F+lE5M2HTF6U=","Date":"Fri, 25 Jul 2025 09:32:44 +0200","From":"Jacopo Mondi ","To":"Giacomo Cappellini ","Cc":"Umang Jain , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","Message-ID":"","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35118,"web_url":"https://patchwork.libcamera.org/comment/35118/","msgid":"","date":"2025-07-25T09:51:17","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":231,"url":"https://patchwork.libcamera.org/api/people/231/","name":"Giacomo Cappellini","email":"giacomo.cappellini.87@gmail.com"},"content":"It's important to note that this is not meant as an argument, but as a\ndetailed explanation of my perspective.\n\nI don't agree with your assertion that the CameraConfiguration diffing\nprocedure was \"not requested.\" This contradicts the evidence in the\nIRC logs provided.\nThe entire premise of tracking configuration adjustments after\nvalidate() was to provide meaningful information to users,\nparticularly within frameworks like GStreamer. The conversation\nexplicitly highlights the need to understand \"what has changed.\" The\nlog snippet below is particularly pertinent:\n\njmondi | giaco: validate() adjusts the CameraConfiguration (if it can)\nand notifies you about that, what has changed is up to you to figure\nout\n\nThis statement places the responsibility on me to determine what has\nchanged. How is one to \"figure out\" what has changed without a\nmechanism to compare the before and after states? The logical and\nnecessary conclusion, which was subsequently discussed, was a diffing\nprocedure.\n\nFurthermore, my direct question:\ngiaco | build a proper CameraConfiguration diff function, I need to\ncopy it before validation and compare if adjusted\n\nwas met with:\n\njmondi | patches are welcome\n\nThis is not a rejection; it is an invitation to develop the\nfunctionality. If the intent was truly that this was not requested or\ndesired, that would have been the moment to communicate it, not after.\nTo wait until a patch is developed and submitted, only to then claim\nit was \"not requested\" is inefficient use of resources. Had this been\ncommunicated upfront, I could have focused my efforts elsewhere.\nIf the underlying truth is that is not right time for diffing (as\nrelevant objects do not provide diffing helpers) not the right place\n(negotiation in gstreamer element) and to move this to a separate\ntask/patch, I agree.\n\nRegarding the path forward, I'll post a new version (V4 with cover)\nbased on Jaslo's patch with the diffing functionality removed.\n\nG.C.\n\nIl giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi\n ha scritto:\n>\n> On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:\n> > The diff part has been suggested by jmondi and pobrn in IRC chat the very\n> > same day Jaslo's patch has been posted, also my V1 patch was already just\n>\n> Sorry, but that's not what happened\n>\n> You suggested we need a CameraConfiguration diff and me and pobrn\n> tried to suggest how to do it. The fact you need was solely suggested\n> by you.\n>\n> giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> 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\n> pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> giaco | I'll do what I can\n>\n> Below the full log [*]\n>\n> > printing the new orientation and a general message for\n> > StreamConfigutation(s) and SensorConfiguration. There's no problem in\n> > replacing/reverting it and use Jaslo's solution. There are no diffing\n> > method on the relevant objects anyway so a clean solution would not be\n> > available until then.\n> > Being this my first patch, what happens now? Should I post a V4 with\n> > changes in negotiation function deleted and wait for the merge of the two\n> > patches, or should I integrate Jaslo's changes into mine, or vice-versa?\n>\n> If there's something you find useful in Jaslo's patches rebase your\n> changes on top of his ones and specify in the cover that your series\n> depend on his one.\n>\n>\n> >\n> > Thanks,\n> > G.C.\n> >\n> > On Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n> >\n> > > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> >\n> > > libcamera allows to control the images orientation through the\n> > > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > > new gstlibcamera-utils functions:\n> > > > - gst_video_orientation_to_libcamera_orientation\n> > > > - libcamera_orientation_to_gst_video_orientation\n> > > >\n> > > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > > the new Orientation parameter.\n> > > >\n> > > > Signed-off-by: Giacomo Cappellini \n> > > > ---\n> > > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > index a548b0c1..95d3813e 100644\n> > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > @@ -10,6 +10,9 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > +\n> > > > +#include \n> > > >\n> > > > using namespace libcamera;\n> > > >\n> > > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > > >\n> > > > return cm;\n> > > > }\n> > > > +\n> > > > +static const struct {\n> > > > + Orientation orientation;\n> > > > + GstVideoOrientationMethod method;\n> > > > +} orientation_map[]{\n> > > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > > +};\n> > > > +\n> > > > +Orientation\n> > > >\n> > > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > method)\n> > > > +{\n> > > > + for (auto &b : orientation_map) {\n> > > > + if (b.method == method)\n> > > > + return b.orientation;\n> > > > + }\n> > > > +\n> > > > + return Orientation::Rotate0;\n> > > > +}\n> > > > +\n> > > > +GstVideoOrientationMethod\n> > > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > > +{\n> > > > + for (auto &a : orientation_map) {\n> > > > + if (a.orientation == orientation)\n> > > > + return a.method;\n> > > > + }\n> > > > +\n> > > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > > +}\n> > > > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> > > b/src/gstreamer/gstlibcamera-utils.h\n> > > > index 5f4e8a0f..bbbd33db 100644\n> > > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > > @@ -10,6 +10,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -92,3 +93,6 @@ public:\n> > > > private:\n> > > > GRecMutex *mutex_;\n> > > > };\n> > > > +\n> > > > +libcamera::Orientation\n> > > gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > method);\n> > > > +GstVideoOrientationMethod\n> > > libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> > > orientation);\n> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > index 3aca4eed..5f483701 100644\n> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > @@ -29,6 +29,7 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -38,6 +39,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \"gstlibcamera-controls.h\"\n> > > > #include \"gstlibcamera-utils.h\"\n> > > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > > GstTask *task;\n> > > >\n> > > > gchar *camera_name;\n> > > > + GstVideoOrientationMethod orientation;\n> > > >\n> > > > std::atomic pending_eos;\n> > > >\n> > > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > > enum {\n> > > > PROP_0,\n> > > > PROP_CAMERA_NAME,\n> > > > + PROP_ORIENTATION,\n> > > > PROP_LAST\n> > > > };\n> > > >\n> > > > @@ -166,8 +170,8 @@ static void\n> > > gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> > > GST_TYPE_ELEMENT,\n> > > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > > >\n> > > gst_libcamera_src_child_proxy_init)\n> > > > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > \"libcamerasrc\", 0,\n> > > > - \"libcamera Source\"))\n> > > > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > \"libcamerasrc\", 0,\n> > > > + \"libcamera\n> > > Source\"))\n> > > >\n> > > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> > > video/x-bayer\")\n> > > >\n> > > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > > return 0;\n> > > > }\n> > > >\n> > > > -void\n> > > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > {\n> > > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > > >\n> > > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > > }\n> > > >\n> > > > + /* Set orientation control. */\n> > > > + state->config_->orientation =\n> > > gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > > +\n> > > > + /* Save original configuration for comparison after validation */\n> > > > + std::vector orig_stream_cfgs;\n> > > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > > + std::optional orig_sensor_cfg =\n> > > state->config_->sensorConfig;\n> > > > + Orientation orig_orientation = state->config_->orientation;\n> > > > +\n> > > > /* Validate the configuration. */\n> > > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > > + switch (state->config_->validate()) {\n> > > > + case CameraConfiguration::Valid:\n> > > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > > + break;\n> > > > + case CameraConfiguration::Adjusted: {\n> > > > + bool warned = false;\n> > > > + // Warn if number of StreamConfigurations changed\n> > > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > > + GST_WARNING_OBJECT(self, \"Number of\n> > > StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > > + orig_stream_cfgs.size(),\n> > > state->config_->size());\n> > > > + warned = true;\n> > > > + }\n> > > > + // Warn about changes in each StreamConfiguration\n> > > > + // TODO implement diffing in StreamConfiguration\n> > > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> > > state->config_->size()); i++) {\n> > > > + if (orig_stream_cfgs[i].toString() !=\n> > > state->config_->at(i).toString()) {\n> > > > + GST_WARNING_OBJECT(self,\n> > > \"StreamConfiguration %zu changed: %s -> %s\",\n> > > > + i,\n> > > orig_stream_cfgs[i].toString().c_str(),\n> > > > +\n> > > state->config_->at(i).toString().c_str());\n> > > > + warned = true;\n> > > > + }\n> > > > + }\n> > > > + // Warn about SensorConfiguration changes\n> > > > + // TODO implement diffing in SensorConfiguration\n> > > > + if (orig_sensor_cfg.has_value() ||\n> > > state->config_->sensorConfig.has_value()) {\n> > > > + const SensorConfiguration *orig =\n> > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > > + const SensorConfiguration *curr =\n> > > state->config_->sensorConfig.has_value() ?\n> > > &state->config_->sensorConfig.value() : nullptr;\n> > > > + bool sensor_changed = false;\n> > > > + std::ostringstream diff;\n> > > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > > + diff << \"SensorConfiguration presence\n> > > changed: \"\n> > > > + << (orig ? \"was present\" : \"was\n> > > absent\")\n> > > > + << \" -> \"\n> > > > + << (curr ? \"present\" : \"absent\");\n> > > > + sensor_changed = true;\n> > > > + } else if (orig && curr) {\n> > > > + if (orig->bitDepth != curr->bitDepth) {\n> > > > + diff << \"bitDepth: \" <<\n> > > orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > > + sensor_changed = true;\n> > > > + }\n> > > > + if (orig->analogCrop != curr->analogCrop) {\n> > > > + diff << \"analogCrop: \" <<\n> > > orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> > > \";\n> > > > + sensor_changed = true;\n> > > > + }\n> > > > + if (orig->binning.binX !=\n> > > curr->binning.binX ||\n> > > > + orig->binning.binY !=\n> > > curr->binning.binY) {\n> > > > + diff << \"binning: (\" <<\n> > > orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > > + << curr->binning.binX << \",\"\n> > > << curr->binning.binY << \"); \";\n> > > > + sensor_changed = true;\n> > > > + }\n> > > > + if (orig->skipping.xOddInc !=\n> > > curr->skipping.xOddInc ||\n> > > > + orig->skipping.xEvenInc !=\n> > > curr->skipping.xEvenInc ||\n> > > > + orig->skipping.yOddInc !=\n> > > curr->skipping.yOddInc ||\n> > > > + orig->skipping.yEvenInc !=\n> > > curr->skipping.yEvenInc) {\n> > > > + diff << \"skipping: (\"\n> > > > + << orig->skipping.xOddInc <<\n> > > \",\" << orig->skipping.xEvenInc << \",\"\n> > > > + << orig->skipping.yOddInc <<\n> > > \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > > + << curr->skipping.xOddInc <<\n> > > \",\" << curr->skipping.xEvenInc << \",\"\n> > > > + << curr->skipping.yOddInc <<\n> > > \",\" << curr->skipping.yEvenInc << \"); \";\n> > > > + sensor_changed = true;\n> > > > + }\n> > > > + if (orig->outputSize != curr->outputSize) {\n> > > > + diff << \"outputSize: \" <<\n> > > orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> > > \";\n> > > > + sensor_changed = true;\n> > > > + }\n> > > > + }\n> > > > + if (sensor_changed) {\n> > > > + GST_WARNING_OBJECT(self,\n> > > \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > > + warned = true;\n> > > > + }\n> > > > + }\n> > > > + // Warn about orientation change\n> > > > + if (orig_orientation != state->config_->orientation) {\n> > > > + GEnumClass *enum_class = (GEnumClass\n> > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > > + const char *orig_orientation_str =\n> > > g_enum_get_value(enum_class,\n> > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > > + const char *new_orientation_str =\n> > > g_enum_get_value(enum_class,\n> > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> > > -> %s\", orig_orientation_str, new_orientation_str);\n> > > > + warned = true;\n> > > > + }\n> > > > + if (!warned) {\n> > > > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> > > adjusted, but no significant changes detected.\");\n> > > > + }\n> > > > + // Update Gst orientation property to match adjusted config\n> > > > + self->orientation =\n> > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > > + break;\n> > > > + }\n> > >\n> > > I am still trying to understand why you need to diff the\n> > > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > > configuration has been changed, in a detailed manner?\n> > >\n> > > My goal to pointing you to Jaslo's patch was:\n> > > a) If the configuration is changed, that patch already logs a warning\n> > > b) If the the adjusted configuration is not acceptable by downstream\n> > > elements, the pipeline streaming is rejected.\n> > >\n> > > If the configuration is changed, could you not simply report the new\n> > > orientation in the property?\n> > >\n> > >\n> > > > + case CameraConfiguration::Invalid:\n> > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > + (\"Camera configuration is not\n> > > supported\"),\n> > > > + (\"CameraConfiguration::validate()\n> > > returned Invalid\"));\n> > > > return false;\n> > > > + }\n> > > >\n> > > > int ret = state->cam_->configure(state->config_.get());\n> > > > if (ret) {\n> > > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> > > guint prop_id,\n> > > > g_free(self->camera_name);\n> > > > self->camera_name = g_value_dup_string(value);\n> > > > break;\n> > > > + case PROP_ORIENTATION:\n> > > > + self->orientation =\n> > > (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > > + break;\n> > > > default:\n> > > > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> > > value, pspec))\n> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > pspec);\n> > > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> > > guint prop_id, GValue *value,\n> > > > case PROP_CAMERA_NAME:\n> > > > g_value_set_string(value, self->camera_name);\n> > > > break;\n> > > > + case PROP_ORIENTATION:\n> > > > + g_value_set_enum(value, (gint)self->orientation);\n> > > > + break;\n> > > > default:\n> > > > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> > > value, pspec))\n> > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > pspec);\n> > > > @@ -1148,12 +1257,17 @@\n> > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > >\n> > > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> > > Name\",\n> > > > \"Select by name which\n> > > camera to use.\", nullptr,\n> > > > -\n> > > (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > > - |\n> > > G_PARAM_CONSTRUCT\n> > > > - |\n> > > G_PARAM_READWRITE\n> > > > - |\n> > > G_PARAM_STATIC_STRINGS));\n> > > > +\n> > > (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> > > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> > > spec);\n> > > >\n> > > > + /* Register the orientation enum type. */\n> > > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > > + \"Select the orientation of the camera.\",\n> > > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> > > G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> > > spec);\n> > > > +\n> > > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > > }\n> > > >\n> > > > --\n> > > > 2.43.0\n> > > >\n> > >\n>\n> [*]\n>\n> giaco | I'm doing the \"track which CameraConfiguration attributes have been adjusted after validate()\". How do I know what can be changed and\n> | what not?\n> giaco | it seems something validate() should do, not the caller\n> jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out\n> jmondi | it boils down to a few things, streams' sizes and formats and orientation\n> giaco | can I assume that the total number of requested streams remains the same?\n> jmondi | no\n> giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?\n> jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415\n> jmondi | not larger, the pipeline can reduce it to match the number of streams it supports\n> jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust\n> jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)\n> giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted\n> jmondi | patches are welcome\n> giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,\n> | SensorConfiguration and Orientation, too\n> giaco | and aall turtles down ColorSpace and on\n> giaco | I think I will should to the user \"hey your config is adjusted here's your string representation of the new one\"\n> giaco | *shout\n> jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration\n> | ?\n> jmondi | building a proper diff function is not trivial I think\n> giaco | diff has to be implemented in all the objects required to be diffed\n> giaco | and share a common structure\n> giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor\n> jmondi | we should have somewhere a TODO list for ideas for people to contribute\n> jmondi | I think we used to\n> giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable\n> | number of warnings\n> 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\n> | between\n> giaco | how to get a copy of CameraConfiguration before validation?\n> pobrn | don't\n> giaco | then it's impossible to track what validate() does\n> pobrn | you can make copies of the `StreamConfiguration`s\n> pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of\n> | `StreamConfiguration`\n> giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> 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\n> pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> giaco | I'll do what I can\n> giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but \"libcamerasrc\n> | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter: error: Failed to generate camera configuration from roles\"\n> 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\n> | 1 stream\n> jmondi | the gstreamer element does not accept less streams than what it had requested\n> jmondi | if (!config || config->size() != roles.size())\n> jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 367A7C3237\n\tfor ;\n\tFri, 25 Jul 2025 09:51:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B9726908D;\n\tFri, 25 Jul 2025 11:51:33 +0200 (CEST)","from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com\n\t[IPv6:2607:f8b0:4864:20::d2c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD39D69080\n\tfor ;\n\tFri, 25 Jul 2025 11:51:30 +0200 (CEST)","by mail-io1-xd2c.google.com with SMTP id\n\tca18e2360f4ac-875acfc133dso81263539f.1\n\tfor ;\n\tFri, 25 Jul 2025 02:51:30 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"UONHcZ/b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1753437089; x=1754041889;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=06qxlxnPuA2pbRGQXGTeUmkgrgOWEiWuBgpWQYFibTM=;\n\tb=UONHcZ/bDOCGw+ez9gIX6XqNKmWPYwWwWdJE19shc0pILAofPW89ifaYPoeL0sqvUA\n\tFFnghaYR2W6KAG/gryr5/qFQmU8J0WXhEUnaTV3HJNs+ADBnv/uE718aboLJuQP2/4x6\n\tw36zk4nab++Bm2kD2Jkcoyu5KIwAIxHPOOT6PBN6uCgUW01YuhmXiVKA0gqTVtpF0pbk\n\t0k5DT/HkJzt+RXSWMqdOhfV2QINpo+oeiGZCmqJ36as2oQcKBRbISdF1P4iDHQUUie0z\n\tXbsChV24N7WZeQtaF9+afrlDcOXY4eSqZGIfWCDC37+NLoaDE9m3wpynzCgXuagSzCXZ\n\tk5dQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1753437089; x=1754041889;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=06qxlxnPuA2pbRGQXGTeUmkgrgOWEiWuBgpWQYFibTM=;\n\tb=swZXMCM5Xias1d0khKeb9A+j1xwBae1nvldA+o0PIpvcnLq4EWV44K6fRpPg+NTv1z\n\tDfLYd7cRsTwG4hTgwsCRTowB9wdzwZdstnpjAM4cOOuL88+MyCOkejzNw3Ez3sLEWVrA\n\tjpk+maDb7I/cvDLcurYCjzQy0zpLuSev7SeBJSM5Fir7oXr2aRp4sKxrwaI8/1d2D/oD\n\t95kyOLLdOexAqV5ntTdAyo3WN8gcv6UQD6RTTHTqdOh6LP45EXR9Td+8tX+VQdqtDQLe\n\teBBrMdNIjAQbn0rhMHfwu89/O39e02CeXse31q6jEj4isANexGggh8DC4RtPcdzixkVf\n\t7d3A==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCV7CSzRD9pQJTiBFBgWKQqPL41CZNm4NqMTR7Q65niJQJ38hLbNM19IYRnyACLTcJqTaagwmJ9HjDApGnDYTms=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy6g9y/KMkULHncSi2DCegaJ1XbHOgFREEerCVx6Sa3WM6RTVoL\n\tzMcEhzqZQeDH3mnpkKyVuRqNLIAdieHLT7um/A3FDEysSGzdrxdQKv6/J3ZLB51fXa9AUwiHxMM\n\tcmOrQG0vE28YT/ijX+w6TZsRkHzOshVs=","X-Gm-Gg":"ASbGncuvWwwAceGzJ5XpBI5Ogm5WMouVzBZ1ln5eJ2BeYYxVw25T5tWpclIHUs2Wt1y\n\tws1cyldZeVhEXfC+8CdoPKs4YSiEQEuVWs0RQiFNq1Qg/SqL8QsMjNtrgHT7e/NYrd21HQkIW68\n\tvu+KVSy384lnGqu9yt7jTMbFA5TS792cQDR3Bbil7dpQjHJn07dC/6zAuXYLqx1+wLrJO0wkjlP\n\t64JawNL","X-Google-Smtp-Source":"AGHT+IFuoutALv8yrLfIqW7CSGqufuOUszSjrNP4subPyf4qdGi0TxUBTt+siwwqzCqUCsjbQwSTTihilxqUxHWnUBA=","X-Received":"by 2002:a05:6602:1501:b0:86c:ee8b:c089 with SMTP id\n\tca18e2360f4ac-8800f069d78mr199822139f.3.1753437088964;\n\tFri, 25 Jul 2025 02:51:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Giacomo Cappellini ","Date":"Fri, 25 Jul 2025 11:51:17 +0200","X-Gm-Features":"Ac12FXyPYLuBm9OG8wNOPjiqbsxTBi05oNBKrffygy5L0C4P4U7UaGl5kzvg1vc","Message-ID":"","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","To":"Jacopo Mondi ","Cc":"Umang Jain , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35120,"web_url":"https://patchwork.libcamera.org/comment/35120/","msgid":"","date":"2025-07-25T10:20:46","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Listen,\n you can try to put things as you like but\n\nOn Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:\n> It's important to note that this is not meant as an argument, but as a\n> detailed explanation of my perspective.\n>\n> I don't agree with your assertion that the CameraConfiguration diffing\n> procedure was \"not requested.\" This contradicts the evidence in the\n> IRC logs provided.\n> The entire premise of tracking configuration adjustments after\n> validate() was to provide meaningful information to users,\n> particularly within frameworks like GStreamer. The conversation\n> explicitly highlights the need to understand \"what has changed.\" The\n> log snippet below is particularly pertinent:\n>\n> jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)\n> and notifies you about that, what has changed is up to you to figure\n> out\n>\n> This statement places the responsibility on me to determine what has\n> changed. How is one to \"figure out\" what has changed without a\n> mechanism to compare the before and after states? The logical and\n> necessary conclusion, which was subsequently discussed, was a diffing\n> procedure.\n\nAll of out bindings report \"Configuration has been adjusted\"\nin example\n\nAndroid:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734\n\ngstreamer:\nhttps://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620\n\nYou decided you want to report \"what has changed\" nobody asked you to\n\n>\n> Furthermore, my direct question:\n> giaco | build a proper CameraConfiguration diff function, I need to\n> copy it before validation and compare if adjusted\n>\n> was met with:\n>\n> jmondi | patches are welcome\n>\n\nYou tell me (imperatively) to \"build a proper CameraConfiguration diff\nfunction\" and the only thing I can suggest is that you are free to\nsend patches, not because gstreamer need it, because it might be a\nuseful addition to libcamera in general (I also suggested to add a\ntodo list for contributors for that matter).\n\nnobody asked you to do so. stop putting words in my mouth please.\n\n> This is not a rejection; it is an invitation to develop the\n> functionality. If the intent was truly that this was not requested or\n> desired, that would have been the moment to communicate it, not after.\n\nIt's a useful addition to libcamera. Not a requirement for gstreamer.\n\n> To wait until a patch is developed and submitted, only to then claim\n> it was \"not requested\" is inefficient use of resources. Had this been\n> communicated upfront, I could have focused my efforts elsewhere.\n\nLikewise.\n\nI'll make sure not do waste your time anymore by making sure\nI won't waste mine supporting you like I've always tried to do,\ndon't worry.\n\n> If the underlying truth is that is not right time for diffing (as\n> relevant objects do not provide diffing helpers) not the right place\n> (negotiation in gstreamer element) and to move this to a separate\n> task/patch, I agree.\n>\n> Regarding the path forward, I'll post a new version (V4 with cover)\n> based on Jaslo's patch with the diffing functionality removed.\n>\n> G.C.\n>\n> Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi\n> ha scritto:\n> >\n> > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:\n> > > The diff part has been suggested by jmondi and pobrn in IRC chat the very\n> > > same day Jaslo's patch has been posted, also my V1 patch was already just\n> >\n> > Sorry, but that's not what happened\n> >\n> > You suggested we need a CameraConfiguration diff and me and pobrn\n> > tried to suggest how to do it. The fact you need was solely suggested\n> > by you.\n> >\n> > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > 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\n> > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > giaco | I'll do what I can\n> >\n> > Below the full log [*]\n> >\n> > > printing the new orientation and a general message for\n> > > StreamConfigutation(s) and SensorConfiguration. There's no problem in\n> > > replacing/reverting it and use Jaslo's solution. There are no diffing\n> > > method on the relevant objects anyway so a clean solution would not be\n> > > available until then.\n> > > Being this my first patch, what happens now? Should I post a V4 with\n> > > changes in negotiation function deleted and wait for the merge of the two\n> > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?\n> >\n> > If there's something you find useful in Jaslo's patches rebase your\n> > changes on top of his ones and specify in the cover that your series\n> > depend on his one.\n> >\n> >\n> > >\n> > > Thanks,\n> > > G.C.\n> > >\n> > > On Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n> > >\n> > > > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> > >\n> > > > libcamera allows to control the images orientation through the\n> > > > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > > > new gstlibcamera-utils functions:\n> > > > > - gst_video_orientation_to_libcamera_orientation\n> > > > > - libcamera_orientation_to_gst_video_orientation\n> > > > >\n> > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > > > the new Orientation parameter.\n> > > > >\n> > > > > Signed-off-by: Giacomo Cappellini \n> > > > > ---\n> > > > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > > > >\n> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > index a548b0c1..95d3813e 100644\n> > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > @@ -10,6 +10,9 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > +\n> > > > > +#include \n> > > > >\n> > > > > using namespace libcamera;\n> > > > >\n> > > > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > > > >\n> > > > > return cm;\n> > > > > }\n> > > > > +\n> > > > > +static const struct {\n> > > > > + Orientation orientation;\n> > > > > + GstVideoOrientationMethod method;\n> > > > > +} orientation_map[]{\n> > > > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > > > +};\n> > > > > +\n> > > > > +Orientation\n> > > > >\n> > > > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > method)\n> > > > > +{\n> > > > > + for (auto &b : orientation_map) {\n> > > > > + if (b.method == method)\n> > > > > + return b.orientation;\n> > > > > + }\n> > > > > +\n> > > > > + return Orientation::Rotate0;\n> > > > > +}\n> > > > > +\n> > > > > +GstVideoOrientationMethod\n> > > > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > > > +{\n> > > > > + for (auto &a : orientation_map) {\n> > > > > + if (a.orientation == orientation)\n> > > > > + return a.method;\n> > > > > + }\n> > > > > +\n> > > > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > > > +}\n> > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> > > > b/src/gstreamer/gstlibcamera-utils.h\n> > > > > index 5f4e8a0f..bbbd33db 100644\n> > > > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > > > @@ -10,6 +10,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > @@ -92,3 +93,6 @@ public:\n> > > > > private:\n> > > > > GRecMutex *mutex_;\n> > > > > };\n> > > > > +\n> > > > > +libcamera::Orientation\n> > > > gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > method);\n> > > > > +GstVideoOrientationMethod\n> > > > libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> > > > orientation);\n> > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > index 3aca4eed..5f483701 100644\n> > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > @@ -29,6 +29,7 @@\n> > > > >\n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -38,6 +39,7 @@\n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > +#include \n> > > > >\n> > > > > #include \"gstlibcamera-controls.h\"\n> > > > > #include \"gstlibcamera-utils.h\"\n> > > > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > > > GstTask *task;\n> > > > >\n> > > > > gchar *camera_name;\n> > > > > + GstVideoOrientationMethod orientation;\n> > > > >\n> > > > > std::atomic pending_eos;\n> > > > >\n> > > > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > > > enum {\n> > > > > PROP_0,\n> > > > > PROP_CAMERA_NAME,\n> > > > > + PROP_ORIENTATION,\n> > > > > PROP_LAST\n> > > > > };\n> > > > >\n> > > > > @@ -166,8 +170,8 @@ static void\n> > > > gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> > > > GST_TYPE_ELEMENT,\n> > > > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > > > >\n> > > > gst_libcamera_src_child_proxy_init)\n> > > > > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > \"libcamerasrc\", 0,\n> > > > > - \"libcamera Source\"))\n> > > > > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > \"libcamerasrc\", 0,\n> > > > > + \"libcamera\n> > > > Source\"))\n> > > > >\n> > > > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> > > > video/x-bayer\")\n> > > > >\n> > > > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > -void\n> > > > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > {\n> > > > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > > > >\n> > > > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > > > }\n> > > > >\n> > > > > + /* Set orientation control. */\n> > > > > + state->config_->orientation =\n> > > > gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > > > +\n> > > > > + /* Save original configuration for comparison after validation */\n> > > > > + std::vector orig_stream_cfgs;\n> > > > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > > > + std::optional orig_sensor_cfg =\n> > > > state->config_->sensorConfig;\n> > > > > + Orientation orig_orientation = state->config_->orientation;\n> > > > > +\n> > > > > /* Validate the configuration. */\n> > > > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > > > + switch (state->config_->validate()) {\n> > > > > + case CameraConfiguration::Valid:\n> > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > > > + break;\n> > > > > + case CameraConfiguration::Adjusted: {\n> > > > > + bool warned = false;\n> > > > > + // Warn if number of StreamConfigurations changed\n> > > > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > > > + GST_WARNING_OBJECT(self, \"Number of\n> > > > StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > > > + orig_stream_cfgs.size(),\n> > > > state->config_->size());\n> > > > > + warned = true;\n> > > > > + }\n> > > > > + // Warn about changes in each StreamConfiguration\n> > > > > + // TODO implement diffing in StreamConfiguration\n> > > > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> > > > state->config_->size()); i++) {\n> > > > > + if (orig_stream_cfgs[i].toString() !=\n> > > > state->config_->at(i).toString()) {\n> > > > > + GST_WARNING_OBJECT(self,\n> > > > \"StreamConfiguration %zu changed: %s -> %s\",\n> > > > > + i,\n> > > > orig_stream_cfgs[i].toString().c_str(),\n> > > > > +\n> > > > state->config_->at(i).toString().c_str());\n> > > > > + warned = true;\n> > > > > + }\n> > > > > + }\n> > > > > + // Warn about SensorConfiguration changes\n> > > > > + // TODO implement diffing in SensorConfiguration\n> > > > > + if (orig_sensor_cfg.has_value() ||\n> > > > state->config_->sensorConfig.has_value()) {\n> > > > > + const SensorConfiguration *orig =\n> > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > > > + const SensorConfiguration *curr =\n> > > > state->config_->sensorConfig.has_value() ?\n> > > > &state->config_->sensorConfig.value() : nullptr;\n> > > > > + bool sensor_changed = false;\n> > > > > + std::ostringstream diff;\n> > > > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > > > + diff << \"SensorConfiguration presence\n> > > > changed: \"\n> > > > > + << (orig ? \"was present\" : \"was\n> > > > absent\")\n> > > > > + << \" -> \"\n> > > > > + << (curr ? \"present\" : \"absent\");\n> > > > > + sensor_changed = true;\n> > > > > + } else if (orig && curr) {\n> > > > > + if (orig->bitDepth != curr->bitDepth) {\n> > > > > + diff << \"bitDepth: \" <<\n> > > > orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > > > + sensor_changed = true;\n> > > > > + }\n> > > > > + if (orig->analogCrop != curr->analogCrop) {\n> > > > > + diff << \"analogCrop: \" <<\n> > > > orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> > > > \";\n> > > > > + sensor_changed = true;\n> > > > > + }\n> > > > > + if (orig->binning.binX !=\n> > > > curr->binning.binX ||\n> > > > > + orig->binning.binY !=\n> > > > curr->binning.binY) {\n> > > > > + diff << \"binning: (\" <<\n> > > > orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > > > + << curr->binning.binX << \",\"\n> > > > << curr->binning.binY << \"); \";\n> > > > > + sensor_changed = true;\n> > > > > + }\n> > > > > + if (orig->skipping.xOddInc !=\n> > > > curr->skipping.xOddInc ||\n> > > > > + orig->skipping.xEvenInc !=\n> > > > curr->skipping.xEvenInc ||\n> > > > > + orig->skipping.yOddInc !=\n> > > > curr->skipping.yOddInc ||\n> > > > > + orig->skipping.yEvenInc !=\n> > > > curr->skipping.yEvenInc) {\n> > > > > + diff << \"skipping: (\"\n> > > > > + << orig->skipping.xOddInc <<\n> > > > \",\" << orig->skipping.xEvenInc << \",\"\n> > > > > + << orig->skipping.yOddInc <<\n> > > > \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > > > + << curr->skipping.xOddInc <<\n> > > > \",\" << curr->skipping.xEvenInc << \",\"\n> > > > > + << curr->skipping.yOddInc <<\n> > > > \",\" << curr->skipping.yEvenInc << \"); \";\n> > > > > + sensor_changed = true;\n> > > > > + }\n> > > > > + if (orig->outputSize != curr->outputSize) {\n> > > > > + diff << \"outputSize: \" <<\n> > > > orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> > > > \";\n> > > > > + sensor_changed = true;\n> > > > > + }\n> > > > > + }\n> > > > > + if (sensor_changed) {\n> > > > > + GST_WARNING_OBJECT(self,\n> > > > \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > > > + warned = true;\n> > > > > + }\n> > > > > + }\n> > > > > + // Warn about orientation change\n> > > > > + if (orig_orientation != state->config_->orientation) {\n> > > > > + GEnumClass *enum_class = (GEnumClass\n> > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > > > + const char *orig_orientation_str =\n> > > > g_enum_get_value(enum_class,\n> > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > > > + const char *new_orientation_str =\n> > > > g_enum_get_value(enum_class,\n> > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> > > > -> %s\", orig_orientation_str, new_orientation_str);\n> > > > > + warned = true;\n> > > > > + }\n> > > > > + if (!warned) {\n> > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> > > > adjusted, but no significant changes detected.\");\n> > > > > + }\n> > > > > + // Update Gst orientation property to match adjusted config\n> > > > > + self->orientation =\n> > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > > > + break;\n> > > > > + }\n> > > >\n> > > > I am still trying to understand why you need to diff the\n> > > > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > > > configuration has been changed, in a detailed manner?\n> > > >\n> > > > My goal to pointing you to Jaslo's patch was:\n> > > > a) If the configuration is changed, that patch already logs a warning\n> > > > b) If the the adjusted configuration is not acceptable by downstream\n> > > > elements, the pipeline streaming is rejected.\n> > > >\n> > > > If the configuration is changed, could you not simply report the new\n> > > > orientation in the property?\n> > > >\n> > > >\n> > > > > + case CameraConfiguration::Invalid:\n> > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > + (\"Camera configuration is not\n> > > > supported\"),\n> > > > > + (\"CameraConfiguration::validate()\n> > > > returned Invalid\"));\n> > > > > return false;\n> > > > > + }\n> > > > >\n> > > > > int ret = state->cam_->configure(state->config_.get());\n> > > > > if (ret) {\n> > > > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> > > > guint prop_id,\n> > > > > g_free(self->camera_name);\n> > > > > self->camera_name = g_value_dup_string(value);\n> > > > > break;\n> > > > > + case PROP_ORIENTATION:\n> > > > > + self->orientation =\n> > > > (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > > > + break;\n> > > > > default:\n> > > > > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> > > > value, pspec))\n> > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > pspec);\n> > > > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> > > > guint prop_id, GValue *value,\n> > > > > case PROP_CAMERA_NAME:\n> > > > > g_value_set_string(value, self->camera_name);\n> > > > > break;\n> > > > > + case PROP_ORIENTATION:\n> > > > > + g_value_set_enum(value, (gint)self->orientation);\n> > > > > + break;\n> > > > > default:\n> > > > > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> > > > value, pspec))\n> > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > pspec);\n> > > > > @@ -1148,12 +1257,17 @@\n> > > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > >\n> > > > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> > > > Name\",\n> > > > > \"Select by name which\n> > > > camera to use.\", nullptr,\n> > > > > -\n> > > > (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > > > - |\n> > > > G_PARAM_CONSTRUCT\n> > > > > - |\n> > > > G_PARAM_READWRITE\n> > > > > - |\n> > > > G_PARAM_STATIC_STRINGS));\n> > > > > +\n> > > > (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> > > > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> > > > spec);\n> > > > >\n> > > > > + /* Register the orientation enum type. */\n> > > > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > > > + \"Select the orientation of the camera.\",\n> > > > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > > > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> > > > G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> > > > spec);\n> > > > > +\n> > > > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > > > }\n> > > > >\n> > > > > --\n> > > > > 2.43.0\n> > > > >\n> > > >\n> >\n> > [*]\n> >\n> > giaco | I'm doing the \"track which CameraConfiguration attributes have been adjusted after validate()\". How do I know what can be changed and\n> > | what not?\n> > giaco | it seems something validate() should do, not the caller\n> > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out\n> > jmondi | it boils down to a few things, streams' sizes and formats and orientation\n> > giaco | can I assume that the total number of requested streams remains the same?\n> > jmondi | no\n> > giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?\n> > jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415\n> > jmondi | not larger, the pipeline can reduce it to match the number of streams it supports\n> > jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust\n> > jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)\n> > giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted\n> > jmondi | patches are welcome\n> > giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,\n> > | SensorConfiguration and Orientation, too\n> > giaco | and aall turtles down ColorSpace and on\n> > giaco | I think I will should to the user \"hey your config is adjusted here's your string representation of the new one\"\n> > giaco | *shout\n> > jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration\n> > | ?\n> > jmondi | building a proper diff function is not trivial I think\n> > giaco | diff has to be implemented in all the objects required to be diffed\n> > giaco | and share a common structure\n> > giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor\n> > jmondi | we should have somewhere a TODO list for ideas for people to contribute\n> > jmondi | I think we used to\n> > giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable\n> > | number of warnings\n> > 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\n> > | between\n> > giaco | how to get a copy of CameraConfiguration before validation?\n> > pobrn | don't\n> > giaco | then it's impossible to track what validate() does\n> > pobrn | you can make copies of the `StreamConfiguration`s\n> > pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of\n> > | `StreamConfiguration`\n> > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > 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\n> > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > giaco | I'll do what I can\n> > giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but \"libcamerasrc\n> > | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter: error: Failed to generate camera configuration from roles\"\n> > 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\n> > | 1 stream\n> > jmondi | the gstreamer element does not accept less streams than what it had requested\n> > jmondi | if (!config || config->size() != roles.size())\n> > jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 17B20BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 10:20:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D2E769071;\n\tFri, 25 Jul 2025 12:20:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A10E69071\n\tfor ;\n\tFri, 25 Jul 2025 12:20:56 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79CF1C66;\n\tFri, 25 Jul 2025 12:20:13 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QSGtVBey\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753438816;\n\tbh=0L8SIMhktG6cjmgT046irSaqqfb1t0qT1CSo5O+HNSg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QSGtVBeyS4sq+cvUwplC0h7B9rQIxul88GsOlpTZgLaIEWlzqYpva0EX6A7R4bcow\n\tzH+gWdbfnhAxA5HGPZWlyn/2+Pyksvsfp6D6Q2USTJBwOU/thQPR6jktuh6AK37PaP\n\ttJ9G5BM8Jldw0h1vJEQHyiJ4piUSwcDzok/80+bk=","Date":"Fri, 25 Jul 2025 12:20:46 +0200","From":"Jacopo Mondi ","To":"Giacomo Cappellini ","Cc":"Jacopo Mondi , \n\tUmang Jain , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","Message-ID":"","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35128,"web_url":"https://patchwork.libcamera.org/comment/35128/","msgid":"","date":"2025-07-25T11:34:22","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 24. 18:32 keltezéssel, Giacomo Cappellini írta:\n> 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.\n> 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?\n\nI have to admit that unfortunately I haven't read much of the discussion\nregarding this change in detail whether on irc or in email. I was just\ntrying to help with the concrete question at hand. My intention was not to\nsuggest that it should be done. Sorry about that!\n\n\nRegards,\nBarnabás Pőcze\n\n> \n> Thanks,\n> G.C.\n> \n> On Thu, Jul 24, 2025, 05:37 Umang Jain > wrote:\n> \n> On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> \n> > libcamera allows to control the images orientation through the\n> > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > control it. Parameter is mapped internally to libcamera::Orientation via\n> > new gstlibcamera-utils functions:\n> > - gst_video_orientation_to_libcamera_orientation\n> > - libcamera_orientation_to_gst_video_orientation\n> >\n> > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > changes in StreamConfiguration and SensorConfiguration, as well as\n> > the new Orientation parameter.\n> >\n> > Signed-off-by: Giacomo Cappellini >\n> > ---\n> >  src/gstreamer/gstlibcamera-utils.cpp |  39 ++++++++\n> >  src/gstreamer/gstlibcamera-utils.h   |   4 +\n> >  src/gstreamer/gstlibcamerasrc.cpp    | 132 +++++++++++++++++++++++++--\n> >  3 files changed, 166 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index a548b0c1..95d3813e 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -10,6 +10,9 @@\n> >\n> >  #include \n> >  #include \n> > +#include \n> > +\n> > +#include \n> >\n> >  using namespace libcamera;\n> >\n> > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> >\n> >       return cm;\n> >  }\n> > +\n> > +static const struct {\n> > +     Orientation orientation;\n> > +     GstVideoOrientationMethod method;\n> > +} orientation_map[]{\n> > +     { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > +     { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > +     { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > +     { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > +     { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > +     { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > +     { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > +     { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > +};\n> > +\n> > +Orientation\n> > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method)\n> > +{\n> > +     for (auto &b : orientation_map) {\n> > +             if (b.method == method)\n> > +                     return b.orientation;\n> > +     }\n> > +\n> > +     return Orientation::Rotate0;\n> > +}\n> > +\n> > +GstVideoOrientationMethod\n> > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > +{\n> > +     for (auto &a : orientation_map) {\n> > +             if (a.orientation == orientation)\n> > +                     return a.method;\n> > +     }\n> > +\n> > +     return GST_VIDEO_ORIENTATION_IDENTITY;\n> > +}\n> > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> > index 5f4e8a0f..bbbd33db 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.h\n> > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > @@ -10,6 +10,7 @@\n> >\n> >  #include \n> >  #include \n> > +#include \n> >  #include \n> >\n> >  #include \n> > @@ -92,3 +93,6 @@ public:\n> >  private:\n> >       GRecMutex *mutex_;\n> >  };\n> > +\n> > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);\n> > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 3aca4eed..5f483701 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -29,6 +29,7 @@\n> >\n> >  #include \n> >  #include \n> > +#include \n> >  #include \n> >  #include \n> >  #include \n> > @@ -38,6 +39,7 @@\n> >  #include \n> >\n> >  #include \n> > +#include \n> >\n> >  #include \"gstlibcamera-controls.h\"\n> >  #include \"gstlibcamera-utils.h\"\n> > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> >       GstTask *task;\n> >\n> >       gchar *camera_name;\n> > +     GstVideoOrientationMethod orientation;\n> >\n> >       std::atomic pending_eos;\n> >\n> > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> >  enum {\n> >       PROP_0,\n> >       PROP_CAMERA_NAME,\n> > +     PROP_ORIENTATION,\n> >       PROP_LAST\n> >  };\n> >\n> > @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> >  G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> >                       G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> >                                             gst_libcamera_src_child_proxy_init)\n> > -                     GST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> > -                                             \"libcamera Source\"))\n> > +                             GST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> > +                                                     \"libcamera Source\"))\n> >\n> >  #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg; video/x-bayer\")\n> >\n> > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> >       return 0;\n> >  }\n> >\n> > -void\n> > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> >  {\n> >       GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> >\n> > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> >               gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> >       }\n> >\n> > +     /* Set orientation control. */\n> > +     state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > +\n> > +     /* Save original configuration for comparison after validation */\n> > +     std::vector orig_stream_cfgs;\n> > +     for (gsize i = 0; i < state->config_->size(); i++)\n> > +             orig_stream_cfgs.push_back(state->config_->at(i));\n> > +     std::optional orig_sensor_cfg = state->config_->sensorConfig;\n> > +     Orientation orig_orientation = state->config_->orientation;\n> > +\n> >       /* Validate the configuration. */\n> > -     if (state->config_->validate() == CameraConfiguration::Invalid)\n> > +     switch (state->config_->validate()) {\n> > +     case CameraConfiguration::Valid:\n> > +             GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > +             break;\n> > +     case CameraConfiguration::Adjusted: {\n> > +             bool warned = false;\n> > +             // Warn if number of StreamConfigurations changed\n> > +             if (orig_stream_cfgs.size() != state->config_->size()) {\n> > +                     GST_WARNING_OBJECT(self, \"Number of StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > +                                        orig_stream_cfgs.size(), state->config_->size());\n> > +                     warned = true;\n> > +             }\n> > +             // Warn about changes in each StreamConfiguration\n> > +             // TODO implement diffing in StreamConfiguration\n> > +             for (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {\n> > +                     if (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {\n> > +                             GST_WARNING_OBJECT(self, \"StreamConfiguration %zu changed: %s -> %s\",\n> > +                                                i, orig_stream_cfgs[i].toString().c_str(),\n> > +                                                state->config_->at(i).toString().c_str());\n> > +                             warned = true;\n> > +                     }\n> > +             }\n> > +             // Warn about SensorConfiguration changes\n> > +             // TODO implement diffing in SensorConfiguration\n> > +             if (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {\n> > +                     const SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > +                     const SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;\n> > +                     bool sensor_changed = false;\n> > +                     std::ostringstream diff;\n> > +                     if ((orig == nullptr) != (curr == nullptr)) {\n> > +                             diff << \"SensorConfiguration presence changed: \"\n> > +                                  << (orig ? \"was present\" : \"was absent\")\n> > +                                  << \" -> \"\n> > +                                  << (curr ? \"present\" : \"absent\");\n> > +                             sensor_changed = true;\n> > +                     } else if (orig && curr) {\n> > +                             if (orig->bitDepth != curr->bitDepth) {\n> > +                                     diff << \"bitDepth: \" << orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > +                                     sensor_changed = true;\n> > +                             }\n> > +                             if (orig->analogCrop != curr->analogCrop) {\n> > +                                     diff << \"analogCrop: \" << orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \"; \";\n> > +                                     sensor_changed = true;\n> > +                             }\n> > +                             if (orig->binning.binX != curr->binning.binX ||\n> > +                                 orig->binning.binY != curr->binning.binY) {\n> > +                                     diff << \"binning: (\" << orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > +                                          << curr->binning.binX << \",\" << curr->binning.binY << \"); \";\n> > +                                     sensor_changed = true;\n> > +                             }\n> > +                             if (orig->skipping.xOddInc != curr->skipping.xOddInc ||\n> > +                                 orig->skipping.xEvenInc != curr->skipping.xEvenInc ||\n> > +                                 orig->skipping.yOddInc != curr->skipping.yOddInc ||\n> > +                                 orig->skipping.yEvenInc != curr->skipping.yEvenInc) {\n> > +                                     diff << \"skipping: (\"\n> > +                                          << orig->skipping.xOddInc << \",\" << orig->skipping.xEvenInc << \",\"\n> > +                                          << orig->skipping.yOddInc << \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > +                                          << curr->skipping.xOddInc << \",\" << curr->skipping.xEvenInc << \",\"\n> > +                                          << curr->skipping.yOddInc << \",\" << curr->skipping.yEvenInc << \"); \";\n> > +                                     sensor_changed = true;\n> > +                             }\n> > +                             if (orig->outputSize != curr->outputSize) {\n> > +                                     diff << \"outputSize: \" << orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \"; \";\n> > +                                     sensor_changed = true;\n> > +                             }\n> > +                     }\n> > +                     if (sensor_changed) {\n> > +                             GST_WARNING_OBJECT(self, \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > +                             warned = true;\n> > +                     }\n> > +             }\n> > +             // Warn about orientation change\n> > +             if (orig_orientation != state->config_->orientation) {\n> > +                     GEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > +                     const char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > +                     const char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > +                     GST_WARNING_OBJECT(self, \"Orientation changed: %s -> %s\", orig_orientation_str, new_orientation_str);\n> > +                     warned = true;\n> > +             }\n> > +             if (!warned) {\n> > +                     GST_DEBUG_OBJECT(self, \"Camera configuration adjusted, but no significant changes detected.\");\n> > +             }\n> > +             // Update Gst orientation property to match adjusted config\n> > +             self->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > +             break;\n> > +     }\n> \n> I am still trying to understand why you need to diff the\n> CameraConfiguration ? Is it just for logging purposes - to warn that the\n> configuration has been changed, in a detailed manner?\n> \n> My goal to pointing you to Jaslo's patch was:\n> a) If the configuration is changed, that patch already logs a warning\n> b) If the the adjusted configuration is not acceptable by downstream\n>    elements, the pipeline streaming is rejected.\n> \n> If the configuration is changed, could you not simply report the new\n> orientation in the property?\n> \n> \n> > +     case CameraConfiguration::Invalid:\n> > +             GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > +                               (\"Camera configuration is not supported\"),\n> > +                               (\"CameraConfiguration::validate() returned Invalid\"));\n> >               return false;\n> > +     }\n> >\n> >       int ret = state->cam_->configure(state->config_.get());\n> >       if (ret) {\n> > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> >               g_free(self->camera_name);\n> >               self->camera_name = g_value_dup_string(value);\n> >               break;\n> > +     case PROP_ORIENTATION:\n> > +             self->orientation = (GstVideoOrientationMethod)g_value_get_enum(value);\n> > +             break;\n> >       default:\n> >               if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))\n> >                       G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> >       case PROP_CAMERA_NAME:\n> >               g_value_set_string(value, self->camera_name);\n> >               break;\n> > +     case PROP_ORIENTATION:\n> > +             g_value_set_enum(value, (gint)self->orientation);\n> > +             break;\n> >       default:\n> >               if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))\n> >                       G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > @@ -1148,12 +1257,17 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> >\n> >       GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera Name\",\n> >                                              \"Select by name which camera to use.\", nullptr,\n> > -                                            (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > -                                                          | G_PARAM_CONSTRUCT\n> > -                                                          | G_PARAM_READWRITE\n> > -                                                          | G_PARAM_STATIC_STRINGS));\n> > +                                            (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> >       g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> >\n> > +     /* Register the orientation enum type. */\n> > +     spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > +                              \"Select the orientation of the camera.\",\n> > +                              GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > +                              GST_VIDEO_ORIENTATION_IDENTITY,\n> > +                              (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > +     g_object_class_install_property(object_class, PROP_ORIENTATION, spec);\n> > +\n> >       GstCameraControls::installProperties(object_class, PROP_LAST);\n> >  }\n> >\n> > --\n> > 2.43.0\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E5A6BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 11:34:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C77B7690BF;\n\tFri, 25 Jul 2025 13:34:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 13E99690A6\n\tfor ;\n\tFri, 25 Jul 2025 13:34:26 +0200 (CEST)","from [192.168.33.15] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 25B02C66;\n\tFri, 25 Jul 2025 13:33:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TN3Lfak9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753443226;\n\tbh=B3qil7WiasjL44jzF235sxsQyYSz/OVWCTGl3bVDcig=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=TN3Lfak9qOudkvmtQQxD3hkcLSn36vsXEzxF4J7DFxUumaj3uJhazq1CmdsZgtcjf\n\t8OPI4xInox3CMp9Sejhi13pShqo5Rs/aCz62jMNemavjePV/xhMXu7Fxw3R3/zxpdS\n\tPgGbstWqt6cnNYLWQTx2peFeOFK/OqgpxdSusDU8=","Message-ID":"","Date":"Fri, 25 Jul 2025 13:34:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","To":"Giacomo Cappellini ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain ","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35132,"web_url":"https://patchwork.libcamera.org/comment/35132/","msgid":"","date":"2025-07-25T12:15:10","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":231,"url":"https://patchwork.libcamera.org/api/people/231/","name":"Giacomo Cappellini","email":"giacomo.cappellini.87@gmail.com"},"content":"Jacopo Mondi,\n\nI'm sorry that my previous email was perceived as an aggressive or\nargumentative attack. That was absolutely not my intent, and I\nexplicitly stated that my aim was to provide a detailed explanation of\nmy perspective, not to create an argument. I regret that this was not\nclear and considered a waste of time.\n\nI genuinely appreciate the support you've provided, specially as I\nnavigate my first contributions. I understand your frustration and,\nwhile I hope we can move past this misunderstanding, I respect your\ndecision regarding future support, even if this will make harder for\nme to contribute again and experiment with libcamera features.\n\nI've just sent patch V4 based on top of Jaslo's series where his code\ntakes the lead on case CameraConfiguration::Adjusted.\n\nRegards,\n\nG.C.\n\nIl giorno ven 25 lug 2025 alle ore 12:20 Jacopo Mondi\n ha scritto:\n>\n> Listen,\n> you can try to put things as you like but\n>\n> On Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:\n> > It's important to note that this is not meant as an argument, but as a\n> > detailed explanation of my perspective.\n> >\n> > I don't agree with your assertion that the CameraConfiguration diffing\n> > procedure was \"not requested.\" This contradicts the evidence in the\n> > IRC logs provided.\n> > The entire premise of tracking configuration adjustments after\n> > validate() was to provide meaningful information to users,\n> > particularly within frameworks like GStreamer. The conversation\n> > explicitly highlights the need to understand \"what has changed.\" The\n> > log snippet below is particularly pertinent:\n> >\n> > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)\n> > and notifies you about that, what has changed is up to you to figure\n> > out\n> >\n> > This statement places the responsibility on me to determine what has\n> > changed. How is one to \"figure out\" what has changed without a\n> > mechanism to compare the before and after states? The logical and\n> > necessary conclusion, which was subsequently discussed, was a diffing\n> > procedure.\n>\n> All of out bindings report \"Configuration has been adjusted\"\n> in example\n>\n> Android:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734\n>\n> gstreamer:\n> https://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620\n>\n> You decided you want to report \"what has changed\" nobody asked you to\n>\n> >\n> > Furthermore, my direct question:\n> > giaco | build a proper CameraConfiguration diff function, I need to\n> > copy it before validation and compare if adjusted\n> >\n> > was met with:\n> >\n> > jmondi | patches are welcome\n> >\n>\n> You tell me (imperatively) to \"build a proper CameraConfiguration diff\n> function\" and the only thing I can suggest is that you are free to\n> send patches, not because gstreamer need it, because it might be a\n> useful addition to libcamera in general (I also suggested to add a\n> todo list for contributors for that matter).\n>\n> nobody asked you to do so. stop putting words in my mouth please.\n>\n> > This is not a rejection; it is an invitation to develop the\n> > functionality. If the intent was truly that this was not requested or\n> > desired, that would have been the moment to communicate it, not after.\n>\n> It's a useful addition to libcamera. Not a requirement for gstreamer.\n>\n> > To wait until a patch is developed and submitted, only to then claim\n> > it was \"not requested\" is inefficient use of resources. Had this been\n> > communicated upfront, I could have focused my efforts elsewhere.\n>\n> Likewise.\n>\n> I'll make sure not do waste your time anymore by making sure\n> I won't waste mine supporting you like I've always tried to do,\n> don't worry.\n>\n> > If the underlying truth is that is not right time for diffing (as\n> > relevant objects do not provide diffing helpers) not the right place\n> > (negotiation in gstreamer element) and to move this to a separate\n> > task/patch, I agree.\n> >\n> > Regarding the path forward, I'll post a new version (V4 with cover)\n> > based on Jaslo's patch with the diffing functionality removed.\n> >\n> > G.C.\n> >\n> > Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi\n> > ha scritto:\n> > >\n> > > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:\n> > > > The diff part has been suggested by jmondi and pobrn in IRC chat the very\n> > > > same day Jaslo's patch has been posted, also my V1 patch was already just\n> > >\n> > > Sorry, but that's not what happened\n> > >\n> > > You suggested we need a CameraConfiguration diff and me and pobrn\n> > > tried to suggest how to do it. The fact you need was solely suggested\n> > > by you.\n> > >\n> > > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > > 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\n> > > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > > giaco | I'll do what I can\n> > >\n> > > Below the full log [*]\n> > >\n> > > > printing the new orientation and a general message for\n> > > > StreamConfigutation(s) and SensorConfiguration. There's no problem in\n> > > > replacing/reverting it and use Jaslo's solution. There are no diffing\n> > > > method on the relevant objects anyway so a clean solution would not be\n> > > > available until then.\n> > > > Being this my first patch, what happens now? Should I post a V4 with\n> > > > changes in negotiation function deleted and wait for the merge of the two\n> > > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?\n> > >\n> > > If there's something you find useful in Jaslo's patches rebase your\n> > > changes on top of his ones and specify in the cover that your series\n> > > depend on his one.\n> > >\n> > >\n> > > >\n> > > > Thanks,\n> > > > G.C.\n> > > >\n> > > > On Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n> > > >\n> > > > > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> > > >\n> > > > > libcamera allows to control the images orientation through the\n> > > > > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > > > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > > > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > > > > new gstlibcamera-utils functions:\n> > > > > > - gst_video_orientation_to_libcamera_orientation\n> > > > > > - libcamera_orientation_to_gst_video_orientation\n> > > > > >\n> > > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > > > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > > > > the new Orientation parameter.\n> > > > > >\n> > > > > > Signed-off-by: Giacomo Cappellini \n> > > > > > ---\n> > > > > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > > > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > > > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > > > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > index a548b0c1..95d3813e 100644\n> > > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > @@ -10,6 +10,9 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > +\n> > > > > > +#include \n> > > > > >\n> > > > > > using namespace libcamera;\n> > > > > >\n> > > > > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > > > > >\n> > > > > > return cm;\n> > > > > > }\n> > > > > > +\n> > > > > > +static const struct {\n> > > > > > + Orientation orientation;\n> > > > > > + GstVideoOrientationMethod method;\n> > > > > > +} orientation_map[]{\n> > > > > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > > > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > > > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > > > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > > > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > > > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > > > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > > > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > > > > +};\n> > > > > > +\n> > > > > > +Orientation\n> > > > > >\n> > > > > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > > method)\n> > > > > > +{\n> > > > > > + for (auto &b : orientation_map) {\n> > > > > > + if (b.method == method)\n> > > > > > + return b.orientation;\n> > > > > > + }\n> > > > > > +\n> > > > > > + return Orientation::Rotate0;\n> > > > > > +}\n> > > > > > +\n> > > > > > +GstVideoOrientationMethod\n> > > > > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > > > > +{\n> > > > > > + for (auto &a : orientation_map) {\n> > > > > > + if (a.orientation == orientation)\n> > > > > > + return a.method;\n> > > > > > + }\n> > > > > > +\n> > > > > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > > > > +}\n> > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> > > > > b/src/gstreamer/gstlibcamera-utils.h\n> > > > > > index 5f4e8a0f..bbbd33db 100644\n> > > > > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > > > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > > > > @@ -10,6 +10,7 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > >\n> > > > > > #include \n> > > > > > @@ -92,3 +93,6 @@ public:\n> > > > > > private:\n> > > > > > GRecMutex *mutex_;\n> > > > > > };\n> > > > > > +\n> > > > > > +libcamera::Orientation\n> > > > > gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > > method);\n> > > > > > +GstVideoOrientationMethod\n> > > > > libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> > > > > orientation);\n> > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > index 3aca4eed..5f483701 100644\n> > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > @@ -29,6 +29,7 @@\n> > > > > >\n> > > > > > #include \n> > > > > > #include \n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -38,6 +39,7 @@\n> > > > > > #include \n> > > > > >\n> > > > > > #include \n> > > > > > +#include \n> > > > > >\n> > > > > > #include \"gstlibcamera-controls.h\"\n> > > > > > #include \"gstlibcamera-utils.h\"\n> > > > > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > > > > GstTask *task;\n> > > > > >\n> > > > > > gchar *camera_name;\n> > > > > > + GstVideoOrientationMethod orientation;\n> > > > > >\n> > > > > > std::atomic pending_eos;\n> > > > > >\n> > > > > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > > > > enum {\n> > > > > > PROP_0,\n> > > > > > PROP_CAMERA_NAME,\n> > > > > > + PROP_ORIENTATION,\n> > > > > > PROP_LAST\n> > > > > > };\n> > > > > >\n> > > > > > @@ -166,8 +170,8 @@ static void\n> > > > > gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> > > > > GST_TYPE_ELEMENT,\n> > > > > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > > > > >\n> > > > > gst_libcamera_src_child_proxy_init)\n> > > > > > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > > \"libcamerasrc\", 0,\n> > > > > > - \"libcamera Source\"))\n> > > > > > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > > \"libcamerasrc\", 0,\n> > > > > > + \"libcamera\n> > > > > Source\"))\n> > > > > >\n> > > > > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> > > > > video/x-bayer\")\n> > > > > >\n> > > > > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > -void\n> > > > > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > > {\n> > > > > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > > > > >\n> > > > > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > > > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > > > > }\n> > > > > >\n> > > > > > + /* Set orientation control. */\n> > > > > > + state->config_->orientation =\n> > > > > gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > > > > +\n> > > > > > + /* Save original configuration for comparison after validation */\n> > > > > > + std::vector orig_stream_cfgs;\n> > > > > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > > > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > > > > + std::optional orig_sensor_cfg =\n> > > > > state->config_->sensorConfig;\n> > > > > > + Orientation orig_orientation = state->config_->orientation;\n> > > > > > +\n> > > > > > /* Validate the configuration. */\n> > > > > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > > > > + switch (state->config_->validate()) {\n> > > > > > + case CameraConfiguration::Valid:\n> > > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > > > > + break;\n> > > > > > + case CameraConfiguration::Adjusted: {\n> > > > > > + bool warned = false;\n> > > > > > + // Warn if number of StreamConfigurations changed\n> > > > > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > > > > + GST_WARNING_OBJECT(self, \"Number of\n> > > > > StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > > > > + orig_stream_cfgs.size(),\n> > > > > state->config_->size());\n> > > > > > + warned = true;\n> > > > > > + }\n> > > > > > + // Warn about changes in each StreamConfiguration\n> > > > > > + // TODO implement diffing in StreamConfiguration\n> > > > > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> > > > > state->config_->size()); i++) {\n> > > > > > + if (orig_stream_cfgs[i].toString() !=\n> > > > > state->config_->at(i).toString()) {\n> > > > > > + GST_WARNING_OBJECT(self,\n> > > > > \"StreamConfiguration %zu changed: %s -> %s\",\n> > > > > > + i,\n> > > > > orig_stream_cfgs[i].toString().c_str(),\n> > > > > > +\n> > > > > state->config_->at(i).toString().c_str());\n> > > > > > + warned = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > + // Warn about SensorConfiguration changes\n> > > > > > + // TODO implement diffing in SensorConfiguration\n> > > > > > + if (orig_sensor_cfg.has_value() ||\n> > > > > state->config_->sensorConfig.has_value()) {\n> > > > > > + const SensorConfiguration *orig =\n> > > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > > > > + const SensorConfiguration *curr =\n> > > > > state->config_->sensorConfig.has_value() ?\n> > > > > &state->config_->sensorConfig.value() : nullptr;\n> > > > > > + bool sensor_changed = false;\n> > > > > > + std::ostringstream diff;\n> > > > > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > > > > + diff << \"SensorConfiguration presence\n> > > > > changed: \"\n> > > > > > + << (orig ? \"was present\" : \"was\n> > > > > absent\")\n> > > > > > + << \" -> \"\n> > > > > > + << (curr ? \"present\" : \"absent\");\n> > > > > > + sensor_changed = true;\n> > > > > > + } else if (orig && curr) {\n> > > > > > + if (orig->bitDepth != curr->bitDepth) {\n> > > > > > + diff << \"bitDepth: \" <<\n> > > > > orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > > > > + sensor_changed = true;\n> > > > > > + }\n> > > > > > + if (orig->analogCrop != curr->analogCrop) {\n> > > > > > + diff << \"analogCrop: \" <<\n> > > > > orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> > > > > \";\n> > > > > > + sensor_changed = true;\n> > > > > > + }\n> > > > > > + if (orig->binning.binX !=\n> > > > > curr->binning.binX ||\n> > > > > > + orig->binning.binY !=\n> > > > > curr->binning.binY) {\n> > > > > > + diff << \"binning: (\" <<\n> > > > > orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > > > > + << curr->binning.binX << \",\"\n> > > > > << curr->binning.binY << \"); \";\n> > > > > > + sensor_changed = true;\n> > > > > > + }\n> > > > > > + if (orig->skipping.xOddInc !=\n> > > > > curr->skipping.xOddInc ||\n> > > > > > + orig->skipping.xEvenInc !=\n> > > > > curr->skipping.xEvenInc ||\n> > > > > > + orig->skipping.yOddInc !=\n> > > > > curr->skipping.yOddInc ||\n> > > > > > + orig->skipping.yEvenInc !=\n> > > > > curr->skipping.yEvenInc) {\n> > > > > > + diff << \"skipping: (\"\n> > > > > > + << orig->skipping.xOddInc <<\n> > > > > \",\" << orig->skipping.xEvenInc << \",\"\n> > > > > > + << orig->skipping.yOddInc <<\n> > > > > \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > > > > + << curr->skipping.xOddInc <<\n> > > > > \",\" << curr->skipping.xEvenInc << \",\"\n> > > > > > + << curr->skipping.yOddInc <<\n> > > > > \",\" << curr->skipping.yEvenInc << \"); \";\n> > > > > > + sensor_changed = true;\n> > > > > > + }\n> > > > > > + if (orig->outputSize != curr->outputSize) {\n> > > > > > + diff << \"outputSize: \" <<\n> > > > > orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> > > > > \";\n> > > > > > + sensor_changed = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > + if (sensor_changed) {\n> > > > > > + GST_WARNING_OBJECT(self,\n> > > > > \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > > > > + warned = true;\n> > > > > > + }\n> > > > > > + }\n> > > > > > + // Warn about orientation change\n> > > > > > + if (orig_orientation != state->config_->orientation) {\n> > > > > > + GEnumClass *enum_class = (GEnumClass\n> > > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > > > > + const char *orig_orientation_str =\n> > > > > g_enum_get_value(enum_class,\n> > > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > > > > + const char *new_orientation_str =\n> > > > > g_enum_get_value(enum_class,\n> > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > > > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> > > > > -> %s\", orig_orientation_str, new_orientation_str);\n> > > > > > + warned = true;\n> > > > > > + }\n> > > > > > + if (!warned) {\n> > > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> > > > > adjusted, but no significant changes detected.\");\n> > > > > > + }\n> > > > > > + // Update Gst orientation property to match adjusted config\n> > > > > > + self->orientation =\n> > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > > > > + break;\n> > > > > > + }\n> > > > >\n> > > > > I am still trying to understand why you need to diff the\n> > > > > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > > > > configuration has been changed, in a detailed manner?\n> > > > >\n> > > > > My goal to pointing you to Jaslo's patch was:\n> > > > > a) If the configuration is changed, that patch already logs a warning\n> > > > > b) If the the adjusted configuration is not acceptable by downstream\n> > > > > elements, the pipeline streaming is rejected.\n> > > > >\n> > > > > If the configuration is changed, could you not simply report the new\n> > > > > orientation in the property?\n> > > > >\n> > > > >\n> > > > > > + case CameraConfiguration::Invalid:\n> > > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > + (\"Camera configuration is not\n> > > > > supported\"),\n> > > > > > + (\"CameraConfiguration::validate()\n> > > > > returned Invalid\"));\n> > > > > > return false;\n> > > > > > + }\n> > > > > >\n> > > > > > int ret = state->cam_->configure(state->config_.get());\n> > > > > > if (ret) {\n> > > > > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> > > > > guint prop_id,\n> > > > > > g_free(self->camera_name);\n> > > > > > self->camera_name = g_value_dup_string(value);\n> > > > > > break;\n> > > > > > + case PROP_ORIENTATION:\n> > > > > > + self->orientation =\n> > > > > (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > > > > + break;\n> > > > > > default:\n> > > > > > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> > > > > value, pspec))\n> > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > > pspec);\n> > > > > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> > > > > guint prop_id, GValue *value,\n> > > > > > case PROP_CAMERA_NAME:\n> > > > > > g_value_set_string(value, self->camera_name);\n> > > > > > break;\n> > > > > > + case PROP_ORIENTATION:\n> > > > > > + g_value_set_enum(value, (gint)self->orientation);\n> > > > > > + break;\n> > > > > > default:\n> > > > > > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> > > > > value, pspec))\n> > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > > pspec);\n> > > > > > @@ -1148,12 +1257,17 @@\n> > > > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > > >\n> > > > > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> > > > > Name\",\n> > > > > > \"Select by name which\n> > > > > camera to use.\", nullptr,\n> > > > > > -\n> > > > > (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > > > > - |\n> > > > > G_PARAM_CONSTRUCT\n> > > > > > - |\n> > > > > G_PARAM_READWRITE\n> > > > > > - |\n> > > > > G_PARAM_STATIC_STRINGS));\n> > > > > > +\n> > > > > (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> > > > > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> > > > > spec);\n> > > > > >\n> > > > > > + /* Register the orientation enum type. */\n> > > > > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > > > > + \"Select the orientation of the camera.\",\n> > > > > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > > > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > > > > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> > > > > G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> > > > > spec);\n> > > > > > +\n> > > > > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > > > > }\n> > > > > >\n> > > > > > --\n> > > > > > 2.43.0\n> > > > > >\n> > > > >\n> > >\n> > > [*]\n> > >\n> > > giaco | I'm doing the \"track which CameraConfiguration attributes have been adjusted after validate()\". How do I know what can be changed and\n> > > | what not?\n> > > giaco | it seems something validate() should do, not the caller\n> > > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out\n> > > jmondi | it boils down to a few things, streams' sizes and formats and orientation\n> > > giaco | can I assume that the total number of requested streams remains the same?\n> > > jmondi | no\n> > > giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?\n> > > jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415\n> > > jmondi | not larger, the pipeline can reduce it to match the number of streams it supports\n> > > jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust\n> > > jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)\n> > > giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted\n> > > jmondi | patches are welcome\n> > > giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,\n> > > | SensorConfiguration and Orientation, too\n> > > giaco | and aall turtles down ColorSpace and on\n> > > giaco | I think I will should to the user \"hey your config is adjusted here's your string representation of the new one\"\n> > > giaco | *shout\n> > > jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration\n> > > | ?\n> > > jmondi | building a proper diff function is not trivial I think\n> > > giaco | diff has to be implemented in all the objects required to be diffed\n> > > giaco | and share a common structure\n> > > giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor\n> > > jmondi | we should have somewhere a TODO list for ideas for people to contribute\n> > > jmondi | I think we used to\n> > > giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable\n> > > | number of warnings\n> > > 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\n> > > | between\n> > > giaco | how to get a copy of CameraConfiguration before validation?\n> > > pobrn | don't\n> > > giaco | then it's impossible to track what validate() does\n> > > pobrn | you can make copies of the `StreamConfiguration`s\n> > > pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of\n> > > | `StreamConfiguration`\n> > > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > > 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\n> > > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > > giaco | I'll do what I can\n> > > giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but \"libcamerasrc\n> > > | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter: error: Failed to generate camera configuration from roles\"\n> > > 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\n> > > | 1 stream\n> > > jmondi | the gstreamer element does not accept less streams than what it had requested\n> > > jmondi | if (!config || config->size() != roles.size())\n> > > jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19B47BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 12:15:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE54B690CC;\n\tFri, 25 Jul 2025 14:15:26 +0200 (CEST)","from mail-il1-x129.google.com (mail-il1-x129.google.com\n\t[IPv6:2607:f8b0:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3EB44690A6\n\tfor ;\n\tFri, 25 Jul 2025 14:15:24 +0200 (CEST)","by mail-il1-x129.google.com with SMTP id\n\te9e14a558f8ab-3e3c34a9b4cso8920365ab.3\n\tfor ;\n\tFri, 25 Jul 2025 05:15:24 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"BxIvogKt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1753445723; x=1754050523;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=Xb2wGC9mhtcCOoxUhDMrP21fxxf+jJnTI5sJVz7tb94=;\n\tb=BxIvogKtea3uugJzjKRVQ2aqR6qq7dESRCh38O5aCXW78KGk5ioWEDFJ8pMOymgO8s\n\tHPMBrsg7DbLm8K7iLwW0v21if5TC1UekNh9PSt3tIh88iompFQev1Yo6+Rr2UO6ycxyT\n\t3OrzrLJNot+YuJ0Qy9AnJDapovBjGuNttTWt1OGXrGyQgKySyuLXLr7Xl1j9W4CSs+sf\n\tvZbYHfNndxhwV08Ug+6bzhQiyw7PkZNfVWsga/iPx1FAgTR1Yi7obRepW2ZZYXbc60c8\n\t/qolTq9P3uVy0dIwupLvgNys7HYaMddklscv6roaK1Rp91rbXWrg9BmEumrx5gKQg6tB\n\t+Z+A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1753445723; x=1754050523;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=Xb2wGC9mhtcCOoxUhDMrP21fxxf+jJnTI5sJVz7tb94=;\n\tb=RCvUWQibAR7W9pCf0PP3Oqavy1Z1P/TaDbQnxrTdNVYw8MQz7Gdn5xX8NBVLqnIQ8J\n\tjTaY8IVC2HNCNJsqodtHNm8u9FiI63g3GKeJLXHbiDZBT4ibacV9TXkMiQPt9ALkmASO\n\tav8ufRIqyplbZ0pTc3tTPF2IzOgrE5IujFWnRTmjgElpIXG7x3phj0bQNusqm1pd4hbh\n\tc9sWgWlShS1bdcbAwex5K0nAveIhh6LXe6PIGnKgk7XyUyG6FM0zu11lHuAo2cGaCDa2\n\t10BS9G4RyrfSnr+sol3+Bt1R1vHsv+CJir8OvDUS+svW3j4FIUdrR0Q+yrB/b+jlE7Ex\n\tzG1g==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCVwWNqFExcgtqhRByxnfVMrrahqgcw+VpRjf/jdhg4mOloflRrS+KWWEVABtTrDemy931xKrXAfitqoZjodVdk=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yy3Oj1YIxpmG9eaZWJ7xq0PJwp8VzlPR0G2TPE8baIDZUlFuD9Z\n\tWZc+yLyjynUQFU2XYu492Vy9iISwaRhqGDsDnGq+4V8KrdKs2aJ5ujfHN7xyvKIgwVeCThNjERb\n\tXEkZJc3rxLMiha6zZt251ZC6zM5qOJVA=","X-Gm-Gg":"ASbGncvj4MQl0ZEwwVDv3QcHrFOroJkdVywywACN4/H6ewbExrxZXKKXzVCymBLA8QS\n\tnzHNU+Eth7XYFyYq3d1paecjuYokMDG67t1t0+54SPIKUsXN93qmRCgS0WmCTQLAbFCnkY072kk\n\t3Hd0K3wMTO0/TJf65Cn/8I/BR58xAZfv2VzCHQZMTXnirqMoOjPfAmEK+4IRSPZWRe5Ro0XeQXN\n\t5MR7VGz","X-Google-Smtp-Source":"AGHT+IHr1dziV4AdUGPQ6e+fTRLBHFU/0VC+70IyMlmAgw4i2cl5jnELWHxVjfzJ6vqhAicetwrrHwTJxmJF+UHxO2A=","X-Received":"by 2002:a05:6e02:32c1:b0:3e2:9b17:754e with SMTP id\n\te9e14a558f8ab-3e3c52af855mr23840455ab.11.1753445722525;\n\tFri, 25 Jul 2025 05:15:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t\n\t\n\t\n\t","In-Reply-To":"","From":"Giacomo Cappellini ","Date":"Fri, 25 Jul 2025 14:15:10 +0200","X-Gm-Features":"Ac12FXymsHwldi-zuBTG7T18zTV7_7X3ev4_hHpdGqevIDnnok-or6b1iqmO2n0","Message-ID":"","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","To":"Jacopo Mondi ","Cc":"Umang Jain , libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35135,"web_url":"https://patchwork.libcamera.org/comment/35135/","msgid":"","date":"2025-07-25T12:23:57","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":231,"url":"https://patchwork.libcamera.org/api/people/231/","name":"Giacomo Cappellini","email":"giacomo.cappellini.87@gmail.com"},"content":"Hi Barnabás,\n\nThanks for your message and for clarifying. I appreciate you stepping\nin to help with the concrete question. No worries at all about the\nmisunderstanding; it's acknowledged.\n\nThanks again,\nG.C.\n\nIl giorno ven 25 lug 2025 alle ore 13:34 Barnabás Pőcze\n ha scritto:\n>\n> Hi\n>\n> 2025. 07. 24. 18:32 keltezéssel, Giacomo Cappellini írta:\n> > 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.\n> > 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?\n>\n> I have to admit that unfortunately I haven't read much of the discussion\n> regarding this change in detail whether on irc or in email. I was just\n> trying to help with the concrete question at hand. My intention was not to\n> suggest that it should be done. Sorry about that!\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > Thanks,\n> > G.C.\n> >\n> > On Thu, Jul 24, 2025, 05:37 Umang Jain > wrote:\n> >\n> > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> >\n> > > libcamera allows to control the images orientation through the\n> > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > new gstlibcamera-utils functions:\n> > > - gst_video_orientation_to_libcamera_orientation\n> > > - libcamera_orientation_to_gst_video_orientation\n> > >\n> > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > the new Orientation parameter.\n> > >\n> > > Signed-off-by: Giacomo Cappellini >\n> > > ---\n> > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > > index a548b0c1..95d3813e 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > @@ -10,6 +10,9 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > +\n> > > +#include \n> > >\n> > > using namespace libcamera;\n> > >\n> > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > >\n> > > return cm;\n> > > }\n> > > +\n> > > +static const struct {\n> > > + Orientation orientation;\n> > > + GstVideoOrientationMethod method;\n> > > +} orientation_map[]{\n> > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > +};\n> > > +\n> > > +Orientation\n> > > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method)\n> > > +{\n> > > + for (auto &b : orientation_map) {\n> > > + if (b.method == method)\n> > > + return b.orientation;\n> > > + }\n> > > +\n> > > + return Orientation::Rotate0;\n> > > +}\n> > > +\n> > > +GstVideoOrientationMethod\n> > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > +{\n> > > + for (auto &a : orientation_map) {\n> > > + if (a.orientation == orientation)\n> > > + return a.method;\n> > > + }\n> > > +\n> > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > +}\n> > > diff --git a/src/gstreamer/gstlibcamera-utils.h b/src/gstreamer/gstlibcamera-utils.h\n> > > index 5f4e8a0f..bbbd33db 100644\n> > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > @@ -10,6 +10,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -92,3 +93,6 @@ public:\n> > > private:\n> > > GRecMutex *mutex_;\n> > > };\n> > > +\n> > > +libcamera::Orientation gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod method);\n> > > +GstVideoOrientationMethod libcamera_orientation_to_gst_video_orientation(libcamera::Orientation orientation);\n> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > > index 3aca4eed..5f483701 100644\n> > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > @@ -29,6 +29,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -38,6 +39,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \"gstlibcamera-controls.h\"\n> > > #include \"gstlibcamera-utils.h\"\n> > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > GstTask *task;\n> > >\n> > > gchar *camera_name;\n> > > + GstVideoOrientationMethod orientation;\n> > >\n> > > std::atomic pending_eos;\n> > >\n> > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > enum {\n> > > PROP_0,\n> > > PROP_CAMERA_NAME,\n> > > + PROP_ORIENTATION,\n> > > PROP_LAST\n> > > };\n> > >\n> > > @@ -166,8 +170,8 @@ static void gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src, GST_TYPE_ELEMENT,\n> > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > > gst_libcamera_src_child_proxy_init)\n> > > - GST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> > > - \"libcamera Source\"))\n> > > + GST_DEBUG_CATEGORY_INIT(source_debug, \"libcamerasrc\", 0,\n> > > + \"libcamera Source\"))\n> > >\n> > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg; video/x-bayer\")\n> > >\n> > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > return 0;\n> > > }\n> > >\n> > > -void\n> > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > {\n> > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > >\n> > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > }\n> > >\n> > > + /* Set orientation control. */\n> > > + state->config_->orientation = gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > +\n> > > + /* Save original configuration for comparison after validation */\n> > > + std::vector orig_stream_cfgs;\n> > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > + std::optional orig_sensor_cfg = state->config_->sensorConfig;\n> > > + Orientation orig_orientation = state->config_->orientation;\n> > > +\n> > > /* Validate the configuration. */\n> > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > + switch (state->config_->validate()) {\n> > > + case CameraConfiguration::Valid:\n> > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > + break;\n> > > + case CameraConfiguration::Adjusted: {\n> > > + bool warned = false;\n> > > + // Warn if number of StreamConfigurations changed\n> > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > + GST_WARNING_OBJECT(self, \"Number of StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > + orig_stream_cfgs.size(), state->config_->size());\n> > > + warned = true;\n> > > + }\n> > > + // Warn about changes in each StreamConfiguration\n> > > + // TODO implement diffing in StreamConfiguration\n> > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(), state->config_->size()); i++) {\n> > > + if (orig_stream_cfgs[i].toString() != state->config_->at(i).toString()) {\n> > > + GST_WARNING_OBJECT(self, \"StreamConfiguration %zu changed: %s -> %s\",\n> > > + i, orig_stream_cfgs[i].toString().c_str(),\n> > > + state->config_->at(i).toString().c_str());\n> > > + warned = true;\n> > > + }\n> > > + }\n> > > + // Warn about SensorConfiguration changes\n> > > + // TODO implement diffing in SensorConfiguration\n> > > + if (orig_sensor_cfg.has_value() || state->config_->sensorConfig.has_value()) {\n> > > + const SensorConfiguration *orig = orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > + const SensorConfiguration *curr = state->config_->sensorConfig.has_value() ? &state->config_->sensorConfig.value() : nullptr;\n> > > + bool sensor_changed = false;\n> > > + std::ostringstream diff;\n> > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > + diff << \"SensorConfiguration presence changed: \"\n> > > + << (orig ? \"was present\" : \"was absent\")\n> > > + << \" -> \"\n> > > + << (curr ? \"present\" : \"absent\");\n> > > + sensor_changed = true;\n> > > + } else if (orig && curr) {\n> > > + if (orig->bitDepth != curr->bitDepth) {\n> > > + diff << \"bitDepth: \" << orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->analogCrop != curr->analogCrop) {\n> > > + diff << \"analogCrop: \" << orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \"; \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->binning.binX != curr->binning.binX ||\n> > > + orig->binning.binY != curr->binning.binY) {\n> > > + diff << \"binning: (\" << orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > + << curr->binning.binX << \",\" << curr->binning.binY << \"); \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->skipping.xOddInc != curr->skipping.xOddInc ||\n> > > + orig->skipping.xEvenInc != curr->skipping.xEvenInc ||\n> > > + orig->skipping.yOddInc != curr->skipping.yOddInc ||\n> > > + orig->skipping.yEvenInc != curr->skipping.yEvenInc) {\n> > > + diff << \"skipping: (\"\n> > > + << orig->skipping.xOddInc << \",\" << orig->skipping.xEvenInc << \",\"\n> > > + << orig->skipping.yOddInc << \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > + << curr->skipping.xOddInc << \",\" << curr->skipping.xEvenInc << \",\"\n> > > + << curr->skipping.yOddInc << \",\" << curr->skipping.yEvenInc << \"); \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + if (orig->outputSize != curr->outputSize) {\n> > > + diff << \"outputSize: \" << orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \"; \";\n> > > + sensor_changed = true;\n> > > + }\n> > > + }\n> > > + if (sensor_changed) {\n> > > + GST_WARNING_OBJECT(self, \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > + warned = true;\n> > > + }\n> > > + }\n> > > + // Warn about orientation change\n> > > + if (orig_orientation != state->config_->orientation) {\n> > > + GEnumClass *enum_class = (GEnumClass *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > + const char *orig_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > + const char *new_orientation_str = g_enum_get_value(enum_class, libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s -> %s\", orig_orientation_str, new_orientation_str);\n> > > + warned = true;\n> > > + }\n> > > + if (!warned) {\n> > > + GST_DEBUG_OBJECT(self, \"Camera configuration adjusted, but no significant changes detected.\");\n> > > + }\n> > > + // Update Gst orientation property to match adjusted config\n> > > + self->orientation = libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > + break;\n> > > + }\n> >\n> > I am still trying to understand why you need to diff the\n> > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > configuration has been changed, in a detailed manner?\n> >\n> > My goal to pointing you to Jaslo's patch was:\n> > a) If the configuration is changed, that patch already logs a warning\n> > b) If the the adjusted configuration is not acceptable by downstream\n> > elements, the pipeline streaming is rejected.\n> >\n> > If the configuration is changed, could you not simply report the new\n> > orientation in the property?\n> >\n> >\n> > > + case CameraConfiguration::Invalid:\n> > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > + (\"Camera configuration is not supported\"),\n> > > + (\"CameraConfiguration::validate() returned Invalid\"));\n> > > return false;\n> > > + }\n> > >\n> > > int ret = state->cam_->configure(state->config_.get());\n> > > if (ret) {\n> > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object, guint prop_id,\n> > > g_free(self->camera_name);\n> > > self->camera_name = g_value_dup_string(value);\n> > > break;\n> > > + case PROP_ORIENTATION:\n> > > + self->orientation = (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > + break;\n> > > default:\n> > > if (!state->controls_.setProperty(prop_id - PROP_LAST, value, pspec))\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object, guint prop_id, GValue *value,\n> > > case PROP_CAMERA_NAME:\n> > > g_value_set_string(value, self->camera_name);\n> > > break;\n> > > + case PROP_ORIENTATION:\n> > > + g_value_set_enum(value, (gint)self->orientation);\n> > > + break;\n> > > default:\n> > > if (!state->controls_.getProperty(prop_id - PROP_LAST, value, pspec))\n> > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec);\n> > > @@ -1148,12 +1257,17 @@ gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > >\n> > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera Name\",\n> > > \"Select by name which camera to use.\", nullptr,\n> > > - (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > - | G_PARAM_CONSTRUCT\n> > > - | G_PARAM_READWRITE\n> > > - | G_PARAM_STATIC_STRINGS));\n> > > + (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > g_object_class_install_property(object_class, PROP_CAMERA_NAME, spec);\n> > >\n> > > + /* Register the orientation enum type. */\n> > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > + \"Select the orientation of the camera.\",\n> > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > + (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > + g_object_class_install_property(object_class, PROP_ORIENTATION, spec);\n> > > +\n> > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > }\n> > >\n> > > --\n> > > 2.43.0\n> > >\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3EE7CBDCC1\n\tfor ;\n\tFri, 25 Jul 2025 12:24:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 01BF6690DC;\n\tFri, 25 Jul 2025 14:24:14 +0200 (CEST)","from mail-il1-x130.google.com (mail-il1-x130.google.com\n\t[IPv6:2607:f8b0:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E08E690A6\n\tfor ;\n\tFri, 25 Jul 2025 14:24:11 +0200 (CEST)","by mail-il1-x130.google.com with SMTP id\n\te9e14a558f8ab-3e059b14cb1so14492095ab.0\n\tfor ;\n\tFri, 25 Jul 2025 05:24:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"ZQIrdlV9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1753446250; x=1754051050;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=kyYhBBjRSTlroa0AIi4yy3QVmro6pRCzDElYT3qUXxQ=;\n\tb=ZQIrdlV9vBnZSt0vWUIdOuqtrj4m997oirxskrUTWrHBhIlmzY7PV8TsYSmUxacJmD\n\tKBzYJ0ViBf+dphM7s/YE70fJ90eQT4Hzb8fDTunqX9lOGBT5gIlndDE4RUyoV8vpB8/6\n\tB259HkphMDyK2S1vJrBT3tglpTIeFoB97s2dhff8+Ct92RihoXK9P7C3ZWVhMv01IcmG\n\t/3wlL03lY4EuaG5kSQXfnUs8j0i2eTjfV8TgEdfPhdHEXs5IZtHedQ75XKwoGM1XXebU\n\t8f83DryNzbwBaBxmpaFaNOX3Eul+n91sStC8XjZuFUYddbuhaxdQf6dMflc+jc0uYbZ6\n\tIVAQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1753446250; x=1754051050;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=kyYhBBjRSTlroa0AIi4yy3QVmro6pRCzDElYT3qUXxQ=;\n\tb=mMabaVT++HfwU12QCRRSZtiBd3vQrEQOToPmnuAuGzG7SG3AJW6qiTKILT01dUuM1W\n\thBwXH7FFkPC3Eu2Q0ntjEjbIjy7oktOHBRQitgoQbtcFgyXSjAu5CZ7ZrL60PzlxhXem\n\thhcfBx9eXR3iCQwShXXcvRB2691HBYWon0ut3SesOGa/TuHjPC/tibG0Vst88JrRzeJS\n\tTInBnalw6qBGJY5QdsYk4erC9YBun2PbbuEPju9tOfqwtgX4O/uze7I3QJs/pR0wNPOo\n\tC+jGvI4NXbfm7HeE02dDN3c9Yww+dULCtbCy/yWgxuFom7PkzOssFdRDjGRm9dW5aLdr\n\tzPLw==","X-Gm-Message-State":"AOJu0YzXyirz0/5yqjtvqKlyuM4ozdwSwBecdgWq21WQCLKAivLMD3NA\n\tl2/5k4Y2kUVe7hdLpcyI3XMaE+ktLxrPG7aroFgC47oGfjjXA4S13L1sQA04+lgE6f+VzyjUDlv\n\tM9ElRgwA9SEat9MX4tsTBgC9DbXpxS1mQO8V0uho=","X-Gm-Gg":"ASbGnctu1z9kqJ/JjnvfMtIakXPIpknjSuRIy0MOnXiLJEI8VlhNaNdw78p3XIYBOfR\n\tNBeV6Sp5T5m3oI8U/7l91KWQC0H1C+l75qddOrhsfbd1EDrctjKAEKQbIfsPAi/WyWiDKuSwQJs\n\tYlzMPH2aH3obQx/dyRx/S6WMp9ZX7t6AhHtmIrFeWdmJ4/Tz6rboyIeCj4lIvEmwBkwEiu+R6E/\n\t4m4166p3pqG2FhE61k=","X-Google-Smtp-Source":"AGHT+IGl3hj0skeFrpTqA4RKT63dy7wQRDDaECcdLeb49Rj+osWrjMDtYHSHNs6ruGcVxuWc9c9FYMla0Ui3wttu21E=","X-Received":"by 2002:a05:6e02:2490:b0:3e2:9f5c:520f with SMTP id\n\te9e14a558f8ab-3e3c45fb852mr28678345ab.3.1753446249493;\n\tFri, 25 Jul 2025 05:24:09 -0700 (PDT)","MIME-Version":"1.0","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Giacomo Cappellini ","Date":"Fri, 25 Jul 2025 14:23:57 +0200","X-Gm-Features":"Ac12FXymWnXZgd1UTpmzQWhUE2KZEb7254ueUEcbjrJRqDRJbzcCrNfXY16wCEo","Message-ID":"","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":35136,"web_url":"https://patchwork.libcamera.org/comment/35136/","msgid":"","date":"2025-07-25T12:25:45","subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Giacomo\n\nOn Fri, Jul 25, 2025 at 02:15:10PM +0200, Giacomo Cappellini wrote:\n> Jacopo Mondi,\n>\n> I'm sorry that my previous email was perceived as an aggressive or\n> argumentative attack. That was absolutely not my intent, and I\n> explicitly stated that my aim was to provide a detailed explanation of\n> my perspective, not to create an argument. I regret that this was not\n> clear and considered a waste of time.\n>\n> I genuinely appreciate the support you've provided, specially as I\n> navigate my first contributions. I understand your frustration and,\n> while I hope we can move past this misunderstanding, I respect your\n> decision regarding future support, even if this will make harder for\n> me to contribute again and experiment with libcamera features.\n\nThanks for your reconciling email and allow me to say I'm sorry if my\nmessage was too aggressive.\n\nAs always, getting a conversation tone from writing is complex, and\nhere we're mixing emails and irc logs. I'm sorry if I've\nmis-interpreted your message.\n\nThank you again for your interest and contributions to the project.\n\n>\n> I've just sent patch V4 based on top of Jaslo's series where his code\n> takes the lead on case CameraConfiguration::Adjusted.\n>\n> Regards,\n>\n> G.C.\n>\n> Il giorno ven 25 lug 2025 alle ore 12:20 Jacopo Mondi\n> ha scritto:\n> >\n> > Listen,\n> > you can try to put things as you like but\n> >\n> > On Fri, Jul 25, 2025 at 11:51:17AM +0200, Giacomo Cappellini wrote:\n> > > It's important to note that this is not meant as an argument, but as a\n> > > detailed explanation of my perspective.\n> > >\n> > > I don't agree with your assertion that the CameraConfiguration diffing\n> > > procedure was \"not requested.\" This contradicts the evidence in the\n> > > IRC logs provided.\n> > > The entire premise of tracking configuration adjustments after\n> > > validate() was to provide meaningful information to users,\n> > > particularly within frameworks like GStreamer. The conversation\n> > > explicitly highlights the need to understand \"what has changed.\" The\n> > > log snippet below is particularly pertinent:\n> > >\n> > > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can)\n> > > and notifies you about that, what has changed is up to you to figure\n> > > out\n> > >\n> > > This statement places the responsibility on me to determine what has\n> > > changed. How is one to \"figure out\" what has changed without a\n> > > mechanism to compare the before and after states? The logical and\n> > > necessary conclusion, which was subsequently discussed, was a diffing\n> > > procedure.\n> >\n> > All of out bindings report \"Configuration has been adjusted\"\n> > in example\n> >\n> > Android:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n734\n> >\n> > gstreamer:\n> > https://git.libcamera.org/libcamera/libcamera.git/tree/src/gstreamer/gstlibcamerasrc.cpp#n620\n> >\n> > You decided you want to report \"what has changed\" nobody asked you to\n> >\n> > >\n> > > Furthermore, my direct question:\n> > > giaco | build a proper CameraConfiguration diff function, I need to\n> > > copy it before validation and compare if adjusted\n> > >\n> > > was met with:\n> > >\n> > > jmondi | patches are welcome\n> > >\n> >\n> > You tell me (imperatively) to \"build a proper CameraConfiguration diff\n> > function\" and the only thing I can suggest is that you are free to\n> > send patches, not because gstreamer need it, because it might be a\n> > useful addition to libcamera in general (I also suggested to add a\n> > todo list for contributors for that matter).\n> >\n> > nobody asked you to do so. stop putting words in my mouth please.\n> >\n> > > This is not a rejection; it is an invitation to develop the\n> > > functionality. If the intent was truly that this was not requested or\n> > > desired, that would have been the moment to communicate it, not after.\n> >\n> > It's a useful addition to libcamera. Not a requirement for gstreamer.\n> >\n> > > To wait until a patch is developed and submitted, only to then claim\n> > > it was \"not requested\" is inefficient use of resources. Had this been\n> > > communicated upfront, I could have focused my efforts elsewhere.\n> >\n> > Likewise.\n> >\n> > I'll make sure not do waste your time anymore by making sure\n> > I won't waste mine supporting you like I've always tried to do,\n> > don't worry.\n> >\n> > > If the underlying truth is that is not right time for diffing (as\n> > > relevant objects do not provide diffing helpers) not the right place\n> > > (negotiation in gstreamer element) and to move this to a separate\n> > > task/patch, I agree.\n> > >\n> > > Regarding the path forward, I'll post a new version (V4 with cover)\n> > > based on Jaslo's patch with the diffing functionality removed.\n> > >\n> > > G.C.\n> > >\n> > > Il giorno ven 25 lug 2025 alle ore 09:32 Jacopo Mondi\n> > > ha scritto:\n> > > >\n> > > > On Thu, Jul 24, 2025 at 06:32:29PM +0200, Giacomo Cappellini wrote:\n> > > > > The diff part has been suggested by jmondi and pobrn in IRC chat the very\n> > > > > same day Jaslo's patch has been posted, also my V1 patch was already just\n> > > >\n> > > > Sorry, but that's not what happened\n> > > >\n> > > > You suggested we need a CameraConfiguration diff and me and pobrn\n> > > > tried to suggest how to do it. The fact you need was solely suggested\n> > > > by you.\n> > > >\n> > > > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > > > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > > > 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\n> > > > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > > > giaco | I'll do what I can\n> > > >\n> > > > Below the full log [*]\n> > > >\n> > > > > printing the new orientation and a general message for\n> > > > > StreamConfigutation(s) and SensorConfiguration. There's no problem in\n> > > > > replacing/reverting it and use Jaslo's solution. There are no diffing\n> > > > > method on the relevant objects anyway so a clean solution would not be\n> > > > > available until then.\n> > > > > Being this my first patch, what happens now? Should I post a V4 with\n> > > > > changes in negotiation function deleted and wait for the merge of the two\n> > > > > patches, or should I integrate Jaslo's changes into mine, or vice-versa?\n> > > >\n> > > > If there's something you find useful in Jaslo's patches rebase your\n> > > > changes on top of his ones and specify in the cover that your series\n> > > > depend on his one.\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks,\n> > > > > G.C.\n> > > > >\n> > > > > On Thu, Jul 24, 2025, 05:37 Umang Jain wrote:\n> > > > >\n> > > > > > On Wed, Jul 23, 2025 at 04:07:55PM +0200, Giacomo Cappellini wrote:\n> > > > >\n> > > > > > libcamera allows to control the images orientation through the\n> > > > > > > CameraConfiguration::orientation field, expose a GST_PARAM_MUTABLE_READY\n> > > > > > > parameter of type GstVideoOrientationMethod in GstLibcameraSrc to\n> > > > > > > control it. Parameter is mapped internally to libcamera::Orientation via\n> > > > > > > new gstlibcamera-utils functions:\n> > > > > > > - gst_video_orientation_to_libcamera_orientation\n> > > > > > > - libcamera_orientation_to_gst_video_orientation\n> > > > > > >\n> > > > > > > Update CameraConfiguration::Adjusted case in negotiation to warn about\n> > > > > > > changes in StreamConfiguration and SensorConfiguration, as well as\n> > > > > > > the new Orientation parameter.\n> > > > > > >\n> > > > > > > Signed-off-by: Giacomo Cappellini \n> > > > > > > ---\n> > > > > > > src/gstreamer/gstlibcamera-utils.cpp | 39 ++++++++\n> > > > > > > src/gstreamer/gstlibcamera-utils.h | 4 +\n> > > > > > > src/gstreamer/gstlibcamerasrc.cpp | 132 +++++++++++++++++++++++++--\n> > > > > > > 3 files changed, 166 insertions(+), 9 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > > index a548b0c1..95d3813e 100644\n> > > > > > > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > > > > > > @@ -10,6 +10,9 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > +\n> > > > > > > +#include \n> > > > > > >\n> > > > > > > using namespace libcamera;\n> > > > > > >\n> > > > > > > @@ -659,3 +662,39 @@ gst_libcamera_get_camera_manager(int &ret)\n> > > > > > >\n> > > > > > > return cm;\n> > > > > > > }\n> > > > > > > +\n> > > > > > > +static const struct {\n> > > > > > > + Orientation orientation;\n> > > > > > > + GstVideoOrientationMethod method;\n> > > > > > > +} orientation_map[]{\n> > > > > > > + { Orientation::Rotate0, GST_VIDEO_ORIENTATION_IDENTITY },\n> > > > > > > + { Orientation::Rotate90, GST_VIDEO_ORIENTATION_90R },\n> > > > > > > + { Orientation::Rotate180, GST_VIDEO_ORIENTATION_180 },\n> > > > > > > + { Orientation::Rotate270, GST_VIDEO_ORIENTATION_90L },\n> > > > > > > + { Orientation::Rotate0Mirror, GST_VIDEO_ORIENTATION_HORIZ },\n> > > > > > > + { Orientation::Rotate180Mirror, GST_VIDEO_ORIENTATION_VERT },\n> > > > > > > + { Orientation::Rotate90Mirror, GST_VIDEO_ORIENTATION_UL_LR },\n> > > > > > > + { Orientation::Rotate270Mirror, GST_VIDEO_ORIENTATION_UR_LL },\n> > > > > > > +};\n> > > > > > > +\n> > > > > > > +Orientation\n> > > > > > >\n> > > > > > +gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > > > method)\n> > > > > > > +{\n> > > > > > > + for (auto &b : orientation_map) {\n> > > > > > > + if (b.method == method)\n> > > > > > > + return b.orientation;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return Orientation::Rotate0;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > +GstVideoOrientationMethod\n> > > > > > > +libcamera_orientation_to_gst_video_orientation(Orientation orientation)\n> > > > > > > +{\n> > > > > > > + for (auto &a : orientation_map) {\n> > > > > > > + if (a.orientation == orientation)\n> > > > > > > + return a.method;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + return GST_VIDEO_ORIENTATION_IDENTITY;\n> > > > > > > +}\n> > > > > > > diff --git a/src/gstreamer/gstlibcamera-utils.h\n> > > > > > b/src/gstreamer/gstlibcamera-utils.h\n> > > > > > > index 5f4e8a0f..bbbd33db 100644\n> > > > > > > --- a/src/gstreamer/gstlibcamera-utils.h\n> > > > > > > +++ b/src/gstreamer/gstlibcamera-utils.h\n> > > > > > > @@ -10,6 +10,7 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > >\n> > > > > > > #include \n> > > > > > > @@ -92,3 +93,6 @@ public:\n> > > > > > > private:\n> > > > > > > GRecMutex *mutex_;\n> > > > > > > };\n> > > > > > > +\n> > > > > > > +libcamera::Orientation\n> > > > > > gst_video_orientation_to_libcamera_orientation(GstVideoOrientationMethod\n> > > > > > method);\n> > > > > > > +GstVideoOrientationMethod\n> > > > > > libcamera_orientation_to_gst_video_orientation(libcamera::Orientation\n> > > > > > orientation);\n> > > > > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > index 3aca4eed..5f483701 100644\n> > > > > > > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > > > > > > @@ -29,6 +29,7 @@\n> > > > > > >\n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > #include \n> > > > > > > @@ -38,6 +39,7 @@\n> > > > > > > #include \n> > > > > > >\n> > > > > > > #include \n> > > > > > > +#include \n> > > > > > >\n> > > > > > > #include \"gstlibcamera-controls.h\"\n> > > > > > > #include \"gstlibcamera-utils.h\"\n> > > > > > > @@ -146,6 +148,7 @@ struct _GstLibcameraSrc {\n> > > > > > > GstTask *task;\n> > > > > > >\n> > > > > > > gchar *camera_name;\n> > > > > > > + GstVideoOrientationMethod orientation;\n> > > > > > >\n> > > > > > > std::atomic pending_eos;\n> > > > > > >\n> > > > > > > @@ -157,6 +160,7 @@ struct _GstLibcameraSrc {\n> > > > > > > enum {\n> > > > > > > PROP_0,\n> > > > > > > PROP_CAMERA_NAME,\n> > > > > > > + PROP_ORIENTATION,\n> > > > > > > PROP_LAST\n> > > > > > > };\n> > > > > > >\n> > > > > > > @@ -166,8 +170,8 @@ static void\n> > > > > > gst_libcamera_src_child_proxy_init(gpointer g_iface,\n> > > > > > > G_DEFINE_TYPE_WITH_CODE(GstLibcameraSrc, gst_libcamera_src,\n> > > > > > GST_TYPE_ELEMENT,\n> > > > > > > G_IMPLEMENT_INTERFACE(GST_TYPE_CHILD_PROXY,\n> > > > > > >\n> > > > > > gst_libcamera_src_child_proxy_init)\n> > > > > > > - GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > > > \"libcamerasrc\", 0,\n> > > > > > > - \"libcamera Source\"))\n> > > > > > > + GST_DEBUG_CATEGORY_INIT(source_debug,\n> > > > > > \"libcamerasrc\", 0,\n> > > > > > > + \"libcamera\n> > > > > > Source\"))\n> > > > > > >\n> > > > > > > #define TEMPLATE_CAPS GST_STATIC_CAPS(\"video/x-raw; image/jpeg;\n> > > > > > video/x-bayer\")\n> > > > > > >\n> > > > > > > @@ -225,8 +229,7 @@ int GstLibcameraSrcState::queueRequest()\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > -void\n> > > > > > > -GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > > > +void GstLibcameraSrcState::requestCompleted(Request *request)\n> > > > > > > {\n> > > > > > > GST_DEBUG_OBJECT(src_, \"buffers are ready\");\n> > > > > > >\n> > > > > > > @@ -616,9 +619,109 @@ gst_libcamera_src_negotiate(GstLibcameraSrc *self)\n> > > > > > > gst_libcamera_get_framerate_from_caps(caps, element_caps);\n> > > > > > > }\n> > > > > > >\n> > > > > > > + /* Set orientation control. */\n> > > > > > > + state->config_->orientation =\n> > > > > > gst_video_orientation_to_libcamera_orientation(self->orientation);\n> > > > > > > +\n> > > > > > > + /* Save original configuration for comparison after validation */\n> > > > > > > + std::vector orig_stream_cfgs;\n> > > > > > > + for (gsize i = 0; i < state->config_->size(); i++)\n> > > > > > > + orig_stream_cfgs.push_back(state->config_->at(i));\n> > > > > > > + std::optional orig_sensor_cfg =\n> > > > > > state->config_->sensorConfig;\n> > > > > > > + Orientation orig_orientation = state->config_->orientation;\n> > > > > > > +\n> > > > > > > /* Validate the configuration. */\n> > > > > > > - if (state->config_->validate() == CameraConfiguration::Invalid)\n> > > > > > > + switch (state->config_->validate()) {\n> > > > > > > + case CameraConfiguration::Valid:\n> > > > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration is valid\");\n> > > > > > > + break;\n> > > > > > > + case CameraConfiguration::Adjusted: {\n> > > > > > > + bool warned = false;\n> > > > > > > + // Warn if number of StreamConfigurations changed\n> > > > > > > + if (orig_stream_cfgs.size() != state->config_->size()) {\n> > > > > > > + GST_WARNING_OBJECT(self, \"Number of\n> > > > > > StreamConfiguration elements changed: requested=%zu, actual=%zu\",\n> > > > > > > + orig_stream_cfgs.size(),\n> > > > > > state->config_->size());\n> > > > > > > + warned = true;\n> > > > > > > + }\n> > > > > > > + // Warn about changes in each StreamConfiguration\n> > > > > > > + // TODO implement diffing in StreamConfiguration\n> > > > > > > + for (gsize i = 0; i < std::min(orig_stream_cfgs.size(),\n> > > > > > state->config_->size()); i++) {\n> > > > > > > + if (orig_stream_cfgs[i].toString() !=\n> > > > > > state->config_->at(i).toString()) {\n> > > > > > > + GST_WARNING_OBJECT(self,\n> > > > > > \"StreamConfiguration %zu changed: %s -> %s\",\n> > > > > > > + i,\n> > > > > > orig_stream_cfgs[i].toString().c_str(),\n> > > > > > > +\n> > > > > > state->config_->at(i).toString().c_str());\n> > > > > > > + warned = true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > + // Warn about SensorConfiguration changes\n> > > > > > > + // TODO implement diffing in SensorConfiguration\n> > > > > > > + if (orig_sensor_cfg.has_value() ||\n> > > > > > state->config_->sensorConfig.has_value()) {\n> > > > > > > + const SensorConfiguration *orig =\n> > > > > > orig_sensor_cfg.has_value() ? &orig_sensor_cfg.value() : nullptr;\n> > > > > > > + const SensorConfiguration *curr =\n> > > > > > state->config_->sensorConfig.has_value() ?\n> > > > > > &state->config_->sensorConfig.value() : nullptr;\n> > > > > > > + bool sensor_changed = false;\n> > > > > > > + std::ostringstream diff;\n> > > > > > > + if ((orig == nullptr) != (curr == nullptr)) {\n> > > > > > > + diff << \"SensorConfiguration presence\n> > > > > > changed: \"\n> > > > > > > + << (orig ? \"was present\" : \"was\n> > > > > > absent\")\n> > > > > > > + << \" -> \"\n> > > > > > > + << (curr ? \"present\" : \"absent\");\n> > > > > > > + sensor_changed = true;\n> > > > > > > + } else if (orig && curr) {\n> > > > > > > + if (orig->bitDepth != curr->bitDepth) {\n> > > > > > > + diff << \"bitDepth: \" <<\n> > > > > > orig->bitDepth << \" -> \" << curr->bitDepth << \"; \";\n> > > > > > > + sensor_changed = true;\n> > > > > > > + }\n> > > > > > > + if (orig->analogCrop != curr->analogCrop) {\n> > > > > > > + diff << \"analogCrop: \" <<\n> > > > > > orig->analogCrop.toString() << \" -> \" << curr->analogCrop.toString() << \";\n> > > > > > \";\n> > > > > > > + sensor_changed = true;\n> > > > > > > + }\n> > > > > > > + if (orig->binning.binX !=\n> > > > > > curr->binning.binX ||\n> > > > > > > + orig->binning.binY !=\n> > > > > > curr->binning.binY) {\n> > > > > > > + diff << \"binning: (\" <<\n> > > > > > orig->binning.binX << \",\" << orig->binning.binY << \") -> (\"\n> > > > > > > + << curr->binning.binX << \",\"\n> > > > > > << curr->binning.binY << \"); \";\n> > > > > > > + sensor_changed = true;\n> > > > > > > + }\n> > > > > > > + if (orig->skipping.xOddInc !=\n> > > > > > curr->skipping.xOddInc ||\n> > > > > > > + orig->skipping.xEvenInc !=\n> > > > > > curr->skipping.xEvenInc ||\n> > > > > > > + orig->skipping.yOddInc !=\n> > > > > > curr->skipping.yOddInc ||\n> > > > > > > + orig->skipping.yEvenInc !=\n> > > > > > curr->skipping.yEvenInc) {\n> > > > > > > + diff << \"skipping: (\"\n> > > > > > > + << orig->skipping.xOddInc <<\n> > > > > > \",\" << orig->skipping.xEvenInc << \",\"\n> > > > > > > + << orig->skipping.yOddInc <<\n> > > > > > \",\" << orig->skipping.yEvenInc << \") -> (\"\n> > > > > > > + << curr->skipping.xOddInc <<\n> > > > > > \",\" << curr->skipping.xEvenInc << \",\"\n> > > > > > > + << curr->skipping.yOddInc <<\n> > > > > > \",\" << curr->skipping.yEvenInc << \"); \";\n> > > > > > > + sensor_changed = true;\n> > > > > > > + }\n> > > > > > > + if (orig->outputSize != curr->outputSize) {\n> > > > > > > + diff << \"outputSize: \" <<\n> > > > > > orig->outputSize.toString() << \" -> \" << curr->outputSize.toString() << \";\n> > > > > > \";\n> > > > > > > + sensor_changed = true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > + if (sensor_changed) {\n> > > > > > > + GST_WARNING_OBJECT(self,\n> > > > > > \"SensorConfiguration changed: %s\", diff.str().c_str());\n> > > > > > > + warned = true;\n> > > > > > > + }\n> > > > > > > + }\n> > > > > > > + // Warn about orientation change\n> > > > > > > + if (orig_orientation != state->config_->orientation) {\n> > > > > > > + GEnumClass *enum_class = (GEnumClass\n> > > > > > *)g_type_class_ref(GST_TYPE_VIDEO_ORIENTATION_METHOD);\n> > > > > > > + const char *orig_orientation_str =\n> > > > > > g_enum_get_value(enum_class,\n> > > > > > libcamera_orientation_to_gst_video_orientation(orig_orientation))->value_nick;\n> > > > > > > + const char *new_orientation_str =\n> > > > > > g_enum_get_value(enum_class,\n> > > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation))->value_nick;\n> > > > > > > + GST_WARNING_OBJECT(self, \"Orientation changed: %s\n> > > > > > -> %s\", orig_orientation_str, new_orientation_str);\n> > > > > > > + warned = true;\n> > > > > > > + }\n> > > > > > > + if (!warned) {\n> > > > > > > + GST_DEBUG_OBJECT(self, \"Camera configuration\n> > > > > > adjusted, but no significant changes detected.\");\n> > > > > > > + }\n> > > > > > > + // Update Gst orientation property to match adjusted config\n> > > > > > > + self->orientation =\n> > > > > > libcamera_orientation_to_gst_video_orientation(state->config_->orientation);\n> > > > > > > + break;\n> > > > > > > + }\n> > > > > >\n> > > > > > I am still trying to understand why you need to diff the\n> > > > > > CameraConfiguration ? Is it just for logging purposes - to warn that the\n> > > > > > configuration has been changed, in a detailed manner?\n> > > > > >\n> > > > > > My goal to pointing you to Jaslo's patch was:\n> > > > > > a) If the configuration is changed, that patch already logs a warning\n> > > > > > b) If the the adjusted configuration is not acceptable by downstream\n> > > > > > elements, the pipeline streaming is rejected.\n> > > > > >\n> > > > > > If the configuration is changed, could you not simply report the new\n> > > > > > orientation in the property?\n> > > > > >\n> > > > > >\n> > > > > > > + case CameraConfiguration::Invalid:\n> > > > > > > + GST_ELEMENT_ERROR(self, RESOURCE, SETTINGS,\n> > > > > > > + (\"Camera configuration is not\n> > > > > > supported\"),\n> > > > > > > + (\"CameraConfiguration::validate()\n> > > > > > returned Invalid\"));\n> > > > > > > return false;\n> > > > > > > + }\n> > > > > > >\n> > > > > > > int ret = state->cam_->configure(state->config_.get());\n> > > > > > > if (ret) {\n> > > > > > > @@ -926,6 +1029,9 @@ gst_libcamera_src_set_property(GObject *object,\n> > > > > > guint prop_id,\n> > > > > > > g_free(self->camera_name);\n> > > > > > > self->camera_name = g_value_dup_string(value);\n> > > > > > > break;\n> > > > > > > + case PROP_ORIENTATION:\n> > > > > > > + self->orientation =\n> > > > > > (GstVideoOrientationMethod)g_value_get_enum(value);\n> > > > > > > + break;\n> > > > > > > default:\n> > > > > > > if (!state->controls_.setProperty(prop_id - PROP_LAST,\n> > > > > > value, pspec))\n> > > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > > > pspec);\n> > > > > > > @@ -945,6 +1051,9 @@ gst_libcamera_src_get_property(GObject *object,\n> > > > > > guint prop_id, GValue *value,\n> > > > > > > case PROP_CAMERA_NAME:\n> > > > > > > g_value_set_string(value, self->camera_name);\n> > > > > > > break;\n> > > > > > > + case PROP_ORIENTATION:\n> > > > > > > + g_value_set_enum(value, (gint)self->orientation);\n> > > > > > > + break;\n> > > > > > > default:\n> > > > > > > if (!state->controls_.getProperty(prop_id - PROP_LAST,\n> > > > > > value, pspec))\n> > > > > > > G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id,\n> > > > > > pspec);\n> > > > > > > @@ -1148,12 +1257,17 @@\n> > > > > > gst_libcamera_src_class_init(GstLibcameraSrcClass *klass)\n> > > > > > >\n> > > > > > > GParamSpec *spec = g_param_spec_string(\"camera-name\", \"Camera\n> > > > > > Name\",\n> > > > > > > \"Select by name which\n> > > > > > camera to use.\", nullptr,\n> > > > > > > -\n> > > > > > (GParamFlags)(GST_PARAM_MUTABLE_READY\n> > > > > > > - |\n> > > > > > G_PARAM_CONSTRUCT\n> > > > > > > - |\n> > > > > > G_PARAM_READWRITE\n> > > > > > > - |\n> > > > > > G_PARAM_STATIC_STRINGS));\n> > > > > > > +\n> > > > > > (GParamFlags)(GST_PARAM_MUTABLE_READY | G_PARAM_CONSTRUCT |\n> > > > > > G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > > > g_object_class_install_property(object_class, PROP_CAMERA_NAME,\n> > > > > > spec);\n> > > > > > >\n> > > > > > > + /* Register the orientation enum type. */\n> > > > > > > + spec = g_param_spec_enum(\"orientation\", \"Orientation\",\n> > > > > > > + \"Select the orientation of the camera.\",\n> > > > > > > + GST_TYPE_VIDEO_ORIENTATION_METHOD,\n> > > > > > > + GST_VIDEO_ORIENTATION_IDENTITY,\n> > > > > > > + (GParamFlags)(GST_PARAM_MUTABLE_READY |\n> > > > > > G_PARAM_CONSTRUCT | G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));\n> > > > > > > + g_object_class_install_property(object_class, PROP_ORIENTATION,\n> > > > > > spec);\n> > > > > > > +\n> > > > > > > GstCameraControls::installProperties(object_class, PROP_LAST);\n> > > > > > > }\n> > > > > > >\n> > > > > > > --\n> > > > > > > 2.43.0\n> > > > > > >\n> > > > > >\n> > > >\n> > > > [*]\n> > > >\n> > > > giaco | I'm doing the \"track which CameraConfiguration attributes have been adjusted after validate()\". How do I know what can be changed and\n> > > > | what not?\n> > > > giaco | it seems something validate() should do, not the caller\n> > > > jmondi | giaco: validate() adjusts the CameraConfiguration (if it can) and notifies you about that, what has changed is up to you to figure out\n> > > > jmondi | it boils down to a few things, streams' sizes and formats and orientation\n> > > > giaco | can I assume that the total number of requested streams remains the same?\n> > > > jmondi | no\n> > > > giaco | so state->config_->size() before validate() can be larger/smaller than state->config_->size() after validate?\n> > > > jmondi | https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rpi/vc4/vc4.cpp#n415\n> > > > jmondi | not larger, the pipeline can reduce it to match the number of streams it supports\n> > > > jmondi | if you ask for 100 streams and the camera can only produce one, it has to adjust\n> > > > jmondi | same if you ask for an unsupported combination of stream formats (raw + raw in example)\n> > > > giaco | build a proper CameraConfiguration diff function, I need to copy it before validation and compare if adjusted\n> > > > jmondi | patches are welcome\n> > > > giaco | I don't think I know the api enough to implement a diff function. It would required a diff function for StreamConfiguration,\n> > > > | SensorConfiguration and Orientation, too\n> > > > giaco | and aall turtles down ColorSpace and on\n> > > > giaco | I think I will should to the user \"hey your config is adjusted here's your string representation of the new one\"\n> > > > giaco | *shout\n> > > > jmondi | how would a string representation help frameworks like pipewire and gstreamer when they're dynamically negotiating a valid configuration\n> > > > | ?\n> > > > jmondi | building a proper diff function is not trivial I think\n> > > > giaco | diff has to be implemented in all the objects required to be diffed\n> > > > giaco | and share a common structure\n> > > > giaco | I see there's no initial implementation for that, it seems not a task for a first-time contributor\n> > > > jmondi | we should have somewhere a TODO list for ideas for people to contribute\n> > > > jmondi | I think we used to\n> > > > giaco | isn't the negotiation already happening in libcamerasrc? I though this diff function was required only to present to user a reasonable\n> > > > | number of warnings\n> > > > 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\n> > > > | between\n> > > > giaco | how to get a copy of CameraConfiguration before validation?\n> > > > pobrn | don't\n> > > > giaco | then it's impossible to track what validate() does\n> > > > pobrn | you can make copies of the `StreamConfiguration`s\n> > > > pobrn | that will make copies of the `StreamFormats`s, which is highly suboptimal, but unless you want to save each member separately of\n> > > > | `StreamConfiguration`\n> > > > giaco | I'm trying to comply with the request to log to gstreamer users why configuration has been adjusted\n> > > > giaco | the only way to find out what has changed requires pre-validation state of such object, which is non duplicable\n> > > > 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\n> > > > pobrn | I believe the best you can do is copy the public members of each `StreamConfiguration` and compare after validation\n> > > > giaco | I'll do what I can\n> > > > giaco | jmondi: if I request more streams that the available ones I don't get adjusted with 1 stream, but \"libcamerasrc\n> > > > | gstlibcamerasrc.cpp:907:gst_libcamera_src_task_enter: error: Failed to generate camera configuration from roles\"\n> > > > 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\n> > > > | 1 stream\n> > > > jmondi | the gstreamer element does not accept less streams than what it had requested\n> > > > jmondi | if (!config || config->size() != roles.size())\n> > > > jmondi | that's a gstreamer call, I presume if done that way it's because otherwise the pipeline fails to build, dunno","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 57404BDCC1\n\tfor ;\n\tFri, 25 Jul 2025 12:25:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0EC23690A6;\n\tFri, 25 Jul 2025 14:25:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3D05690A6\n\tfor ;\n\tFri, 25 Jul 2025 14:25:49 +0200 (CEST)","from ideasonboard.com (mob-5-90-139-29.net.vodafone.it\n\t[5.90.139.29])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1605CC73;\n\tFri, 25 Jul 2025 14:25:08 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CogoydPy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753446309;\n\tbh=IzUBKCxpfe90mAgAyDzsdCpiNDGmJtU745dbczkdqMY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CogoydPyGoxNb9uj64QbD9ifUcn63zI0TDxBwSp+/MgBUdGvSjW6eXWvKEGSinN+j\n\tmvT+pwgcupwilS6xYtY/L2n5Vvoir4Pgd+pT1mRDRsud/L3Rxef6qKEv1CQNi8Es87\n\tgW68Tb/SGBvK18x5cxRCbQD59u562gVOsK4ePZR0=","Date":"Fri, 25 Jul 2025 14:25:45 +0200","From":"Jacopo Mondi ","To":"Giacomo Cappellini ","Cc":"Jacopo Mondi , \n\tUmang Jain , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3] gstreamer: Add support for Orientation","Message-ID":"","References":"<20250723140801.648652-1-giacomo.cappellini.87@gmail.com>\n\t\n\t\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]