[{"id":33991,"web_url":"https://patchwork.libcamera.org/comment/33991/","msgid":"<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>","date":"2025-04-22T14:42:41","subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Hi,\n\nLe mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :\n> When the libcamerasrc element is stopped and started again, the\n> GstCameraControls::setCamera() function is called, but now it is\n> populated with metadata returned by the previous requests. Because\n> GstCameraControls::setCamera() filters the controls, this would cause\n> warnings to be emitted, as the returned metadata are not valid controls\n> which can be set.\n\nDoes it also fixes on top the issue where controls that get applied\nwith delays get resets and forgotten ?\n\n> \n> This is fixed by introducing a new member which only holds the metadata\n> returned by the requests, so that the controls and the metadata are now\n> completely independent from each other.\n> \n> Signed-off-by: Jaslo Ziska \n> ---\n>  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------\n>  src/gstreamer/gstlibcamera-controls.h      |  4 +++-\n>  2 files changed, 19 insertions(+), 7 deletions(-)\n> \n> diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> index 89c530da..490e835e 100644\n> --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(\n>  bool GstCameraControls::getProperty(guint propId, GValue *value,\n>  \t\t\t\t    [[maybe_unused]] GParamSpec *pspec)\n>  {\n> -\tif (!controls_acc_.contains(propId)) {\n> +\tconst ControlValue *cv;\n> +\tif (controls_acc_.contains(propId)) {\n> +\t\tcv = &controls_acc_.get(propId);\n> +\t} else if (metadata_.contains(propId)) {\n> +\t\tcv = &metadata_.get(propId);\n> +\t} else {\n>  \t\tGST_WARNING(\"Control '%s' is not available, default value will \"\n>  \t\t\t    \"be returned\",\n>  \t\t\t    controls::controls.at(propId)->name().c_str());\n>  \t\treturn true;\n>  \t}\n> -\tconst ControlValue &cv = controls_acc_.get(propId);\n>  \n>  \tswitch (propId) {\n>  {%- for vendor, ctrls in controls %}\n>  {%- for ctrl in ctrls %}\n>  \n>  \tcase controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {\n> -\t\tauto control = cv.get<{{ ctrl.type }}>();\n> +\t\tauto control = cv->get<{{ ctrl.type }}>();\n>  \n>  {%- if ctrl.is_array %}\n>  \t\tfor (size_t i = 0; i < control.size(); ++i) {\n> @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n>  \t\t\t\t    \"camera and will be ignored\",\n>  \t\t\t\t    cid->name().c_str());\n>  \t}\n> -\n>  \tcontrols_acc_ = new_controls;\n>  \tcontrols_ = new_controls;\n> +\n> +\t/*\n> +\t * Clear metadata as this might already be populated if the element has\n> +\t * been running before.\n> +\t */\n> +\tmetadata_.clear();\n>  }\n>  \n>  void GstCameraControls::applyControls(std::unique_ptr &request)\n> @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr &reque\n>  \n>  void GstCameraControls::readMetadata(libcamera::Request *request)\n>  {\n> -\tcontrols_acc_.merge(request->metadata(),\n> -\t\t\t    ControlList::MergePolicy::OverwriteExisting);\n> +\tmetadata_.merge(request->metadata(),\n> +\t\t\tControlList::MergePolicy::OverwriteExisting);\n> +\n>  }\n> diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h\n> index 749220b5..ee97e61e 100644\n> --- a/src/gstreamer/gstlibcamera-controls.h\n> +++ b/src/gstreamer/gstlibcamera-controls.h\n> @@ -36,8 +36,10 @@ private:\n>  \tControlInfoMap capabilities_;\n>  \t/* Set of user modified controls. */\n>  \tControlList controls_;\n> -\t/* Accumulator of all controls ever set and metadata returned by camera */\n> +\t/* Accumulator of all controls ever */\n>  \tControlList controls_acc_;\n> +\t/* Metadata returned by requests */\n> +\tControlList metadata_;\n>  };\n>  \n>  } /* namespace libcamera */\n\nLooks good to me, but I'd like someone from the core to review this\nplease.\n\nNicolas","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 C4FF8BE08B\n\tfor ;\n\tTue, 22 Apr 2025 14:42:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80EF168ACE;\n\tTue, 22 Apr 2025 16:42:44 +0200 (CEST)","from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com\n\t[IPv6:2607:f8b0:4864:20::f2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6579F617E6\n\tfor ;\n\tTue, 22 Apr 2025 16:42:43 +0200 (CEST)","by mail-qv1-xf2d.google.com with SMTP id\n\t6a1803df08f44-6ecfc7ed0c1so47824146d6.3\n\tfor ;\n\tTue, 22 Apr 2025 07:42:43 -0700 (PDT)","from ?IPv6:2606:6d00:15:9913::5ac? ([2606:6d00:15:9913::5ac])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6f2c2b0f80asm58531716d6.45.2025.04.22.07.42.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 22 Apr 2025 07:42:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"PMf/h/WW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1745332962;\n\tx=1745937762; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=woljytBf0ti8/UKhraIZltAFfufO60jAkiX5dOa2w9I=;\n\tb=PMf/h/WWyIbVKLw2WCj8y/ukX9mN/g3sphZyGv1oylxOf70Pdos1+Pze0bX1ElccZ9\n\tgQ1yk5SXCKETMPHjXHblHukBWIL5zbjPeHYaEZM7wYWxtbr1qpH0c02lEVdhTmnR93rf\n\tt0Exurt56BPCI2WAwvUSpM/qf8Oe6SBN9cAD2uLDcsr34X8IyREEAlBBnI0aRaGNNkRD\n\tVCt0oqhYtI+y4quJd/zW5x/al0TAUrXT4VpC229gKrMiSuyBnvo5GbgoYrEnJszWNB/1\n\t3L5d+76yZKtiqQGaBuXZfXT9OgaP8r6CEdAHn9PJI5sK0oX/XPumLo+iNvERbPXTOGaf\n\tc9uA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1745332962; x=1745937762;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:to:from:subject:message-id:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=woljytBf0ti8/UKhraIZltAFfufO60jAkiX5dOa2w9I=;\n\tb=Fd1HvJMRGkcDBaJlpeA2JBxka51hvqZjTUSS6JkOgaGdz8qxiCzECogLFqMZhZiiqQ\n\tMVQ7Tu2ig/H9ue+cpzYiLFYAl1G9DxWOxxbYXmNYx5IUJ1zlHG4rkYTkxZOfrLTrIWXT\n\t3aYOnye4TRgPfK9G5ofTH5ZLR+Mjmov57Z5LUc/duTcPgHPoMceWCzqkQMRSPm2NnIvR\n\tC/jOnWhH2lB58pQf7Mm64pcln25ZPU3XhZRIHJc24TQrZ1FCGRAHS6tKyAnXCjSOWSym\n\tbnMjNGNUrUbQfUqVY7wKTRDKM9JZxAdfVlxljI9y68m5Q8cLmGvAiMu1GfP3KN8eaoAk\n\tUD0g==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUz0UA9fYO98f47PBwCmvust5UOfKfDomCGVGLCgvznSqJ1ADqh5KlWXia72tsHng6+u0kq2/sRx5Oazxfw+dE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw5bcmhi6P/1meA+GNWklUBGMl5RIsx/5vKAK24VPTy/yIzrQnA\n\tz5z8sPIJjMlMzfvijGSArr2e8+n7cepe8d4UjLO+RDdd0EGjd+bGkY1toaBmlmQ=","X-Gm-Gg":"ASbGncv57VK820fILXonnOprKJ2l8bJpX+HmZw7DrQRJPqCJQmtVGf0/7FvXXlzD+2Z\n\tx+XV3793/ohyPJQPOTCxvsQhz2kzOY9v39yWAyytZESBA0KzRP/YcHxq4fkfh+V8AZeToWDUuu4\n\tySeqRrAesoQRZXWohq1K34CZWWNpknIqNY0sU0yEVtSecNvTDPJhSIBHGePmi4DOgi0F2gGDOYt\n\tarJFkfO05rPO/ZJVh5JquQj7pSxklsn2qAbYd8bLbTahvv0VMRPxm+E9qVF7zvcylnD4iw/tr7q\n\tXj4psURPSpwp2vzco0l6yVCf9fg5ri0nvdApQJqWy2KowQ==","X-Google-Smtp-Source":"AGHT+IEROGXDiECzZcO1pIz7++2HdipbWyeqMb0ykm2X9wNXGmXqzQqPg/6THpEGlYr2GKFIs/37ZQ==","X-Received":"by 2002:a05:6214:2588:b0:6ed:15ce:e33e with SMTP id\n\t6a1803df08f44-6f2c4577c35mr295022026d6.27.1745332962375; \n\tTue, 22 Apr 2025 07:42:42 -0700 (PDT)","Message-ID":"<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>","Subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","From":"Nicolas Dufresne ","To":"Jaslo Ziska , libcamera-devel@lists.libcamera.org","Date":"Tue, 22 Apr 2025 10:42:41 -0400","In-Reply-To":"<20250422142010.13064-5-jaslo@ziska.de>","References":"<20250422142010.13064-1-jaslo@ziska.de>\n\t<20250422142010.13064-5-jaslo@ziska.de>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.56.0 (3.56.0-1.fc42) ","MIME-Version":"1.0","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":33992,"web_url":"https://patchwork.libcamera.org/comment/33992/","msgid":"<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>","date":"2025-04-22T15:33:02","subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Nicolas Dufresne (2025-04-22 15:42:41)\n> Hi,\n> \n> Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :\n> > When the libcamerasrc element is stopped and started again, the\n> > GstCameraControls::setCamera() function is called, but now it is\n> > populated with metadata returned by the previous requests. Because\n> > GstCameraControls::setCamera() filters the controls, this would cause\n> > warnings to be emitted, as the returned metadata are not valid controls\n> > which can be set.\n> \n> Does it also fixes on top the issue where controls that get applied\n> with delays get resets and forgotten ?\n> \n> > \n> > This is fixed by introducing a new member which only holds the metadata\n> > returned by the requests, so that the controls and the metadata are now\n> > completely independent from each other.\n> > \n> > Signed-off-by: Jaslo Ziska \n> > ---\n> >  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------\n> >  src/gstreamer/gstlibcamera-controls.h      |  4 +++-\n> >  2 files changed, 19 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > index 89c530da..490e835e 100644\n> > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(\n> >  bool GstCameraControls::getProperty(guint propId, GValue *value,\n> >       [[maybe_unused]] GParamSpec *pspec)\n> >  {\n> > - if (!controls_acc_.contains(propId)) {\n> > + const ControlValue *cv;\n> > + if (controls_acc_.contains(propId)) {\n> > + cv = &controls_acc_.get(propId);\n> > + } else if (metadata_.contains(propId)) {\n> > + cv = &metadata_.get(propId);\n> > + } else {\n> >   GST_WARNING(\"Control '%s' is not available, default value will \"\n> >       \"be returned\",\n> >       controls::controls.at(propId)->name().c_str());\n> >   return true;\n> >   }\n> > - const ControlValue &cv = controls_acc_.get(propId);\n> >  \n> >   switch (propId) {\n> >  {%- for vendor, ctrls in controls %}\n> >  {%- for ctrl in ctrls %}\n> >  \n> >   case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {\n> > - auto control = cv.get<{{ ctrl.type }}>();\n> > + auto control = cv->get<{{ ctrl.type }}>();\n> >  \n> >  {%- if ctrl.is_array %}\n> >   for (size_t i = 0; i < control.size(); ++i) {\n> > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> >       \"camera and will be ignored\",\n> >       cid->name().c_str());\n> >   }\n> > -\n> >   controls_acc_ = new_controls;\n> >   controls_ = new_controls;\n> > +\n> > + /*\n> > + * Clear metadata as this might already be populated if the element has\n> > + * been running before.\n> > + */\n> > + metadata_.clear();\n> >  }\n> >  \n> >  void GstCameraControls::applyControls(std::unique_ptr &request)\n> > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr &reque\n> >  \n> >  void GstCameraControls::readMetadata(libcamera::Request *request)\n> >  {\n> > - controls_acc_.merge(request->metadata(),\n> > -     ControlList::MergePolicy::OverwriteExisting);\n> > + metadata_.merge(request->metadata(),\n> > + ControlList::MergePolicy::OverwriteExisting);\n> > +\n> >  }\n> > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h\n> > index 749220b5..ee97e61e 100644\n> > --- a/src/gstreamer/gstlibcamera-controls.h\n> > +++ b/src/gstreamer/gstlibcamera-controls.h\n> > @@ -36,8 +36,10 @@ private:\n> >   ControlInfoMap capabilities_;\n> >   /* Set of user modified controls. */\n> >   ControlList controls_;\n> > - /* Accumulator of all controls ever set and metadata returned by camera */\n> > + /* Accumulator of all controls ever */\n> >   ControlList controls_acc_;\n\nDo we need a controls_acc_ at all ? What does this do ?\n\nI'm weary because some of the controls are 'triggers' so they should\nonly be set once - not on every frame ... ?\n\n> > + /* Metadata returned by requests */\n> > + ControlList metadata_;\n> >  };\n> >  \n> >  } /* namespace libcamera */\n> \n> Looks good to me, but I'd like someone from the core to review this\n> please.\n> \n> Nicolas","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 7EAC0C327D\n\tfor ;\n\tTue, 22 Apr 2025 15:33:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 76637617E6;\n\tTue, 22 Apr 2025 17:33:08 +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 35D69617E3\n\tfor ;\n\tTue, 22 Apr 2025 17:33:06 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 04416502;\n\tTue, 22 Apr 2025 17:33:04 +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=\"UDxJ9dU3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745335985;\n\tbh=iMgrAi2c1Nnk7M+RUbfRCNkmXHO/zsYa1ElFMEBM3X8=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=UDxJ9dU3+LEoPh/trGPT+s0eg2ip/ctNGuRJU+328KTKTFHBWdA/Aihkham6p6TY5\n\tBHFrC9YdQ0h4wWZvZptLrg42bEb+WjLFHfYuJUvBdezx6otolfHOBh8JKTRhj0p/s4\n\tJDOhZ75aRMnFUBeXOsIQNJQRRmFWo1Umamp0XLIM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>","References":"<20250422142010.13064-1-jaslo@ziska.de>\n\t<20250422142010.13064-5-jaslo@ziska.de>\n\t<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>","Subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","From":"Kieran Bingham ","To":"Jaslo Ziska , Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 22 Apr 2025 16:33:02 +0100","Message-ID":"<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":34001,"web_url":"https://patchwork.libcamera.org/comment/34001/","msgid":"<20250422233409.GY17813@pendragon.ideasonboard.com>","date":"2025-04-22T23:34:09","subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote:\n> Quoting Nicolas Dufresne (2025-04-22 15:42:41)\n> > Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :\n> > > When the libcamerasrc element is stopped and started again, the\n> > > GstCameraControls::setCamera() function is called, but now it is\n> > > populated with metadata returned by the previous requests. Because\n> > > GstCameraControls::setCamera() filters the controls, this would cause\n> > > warnings to be emitted, as the returned metadata are not valid controls\n> > > which can be set.\n> > \n> > Does it also fixes on top the issue where controls that get applied\n> > with delays get resets and forgotten ?\n> > \n> > > This is fixed by introducing a new member which only holds the metadata\n> > > returned by the requests, so that the controls and the metadata are now\n> > > completely independent from each other.\n> > > \n> > > Signed-off-by: Jaslo Ziska \n> > > ---\n> > >  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------\n> > >  src/gstreamer/gstlibcamera-controls.h      |  4 +++-\n> > >  2 files changed, 19 insertions(+), 7 deletions(-)\n> > > \n> > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > index 89c530da..490e835e 100644\n> > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(\n> > >  bool GstCameraControls::getProperty(guint propId, GValue *value,\n> > >       [[maybe_unused]] GParamSpec *pspec)\n> > >  {\n> > > - if (!controls_acc_.contains(propId)) {\n> > > + const ControlValue *cv;\n> > > + if (controls_acc_.contains(propId)) {\n> > > + cv = &controls_acc_.get(propId);\n> > > + } else if (metadata_.contains(propId)) {\n> > > + cv = &metadata_.get(propId);\n\nWhy do controls take precedence here ?\n\nTaking a step back, does GSreamer expect the value of properties to be\nchanged dynamically by the device ? If, for instance, auto-exposure is\nenabled, does GStreamer expect/allow the exposure time and gain\nproperties to be modified to reflect the AGC calculation ?\n\n> > > + } else {\n> > >   GST_WARNING(\"Control '%s' is not available, default value will \"\n> > >       \"be returned\",\n> > >       controls::controls.at(propId)->name().c_str());\n> > >   return true;\n> > >   }\n> > > - const ControlValue &cv = controls_acc_.get(propId);\n> > >  \n> > >   switch (propId) {\n> > >  {%- for vendor, ctrls in controls %}\n> > >  {%- for ctrl in ctrls %}\n> > >  \n> > >   case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {\n> > > - auto control = cv.get<{{ ctrl.type }}>();\n> > > + auto control = cv->get<{{ ctrl.type }}>();\n> > >  \n> > >  {%- if ctrl.is_array %}\n> > >   for (size_t i = 0; i < control.size(); ++i) {\n> > > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > >       \"camera and will be ignored\",\n> > >       cid->name().c_str());\n> > >   }\n> > > -\n\nPlease keep the blank line here.\n\n> > >   controls_acc_ = new_controls;\n> > >   controls_ = new_controls;\n> > > +\n> > > + /*\n> > > + * Clear metadata as this might already be populated if the element has\n> > > + * been running before.\n> > > + */\n> > > + metadata_.clear();\n> > >  }\n> > >  \n> > >  void GstCameraControls::applyControls(std::unique_ptr &request)\n> > > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr &reque\n> > >  \n> > >  void GstCameraControls::readMetadata(libcamera::Request *request)\n> > >  {\n> > > - controls_acc_.merge(request->metadata(),\n> > > -     ControlList::MergePolicy::OverwriteExisting);\n> > > + metadata_.merge(request->metadata(),\n> > > + ControlList::MergePolicy::OverwriteExisting);\n> > > +\n> > >  }\n> > > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h\n> > > index 749220b5..ee97e61e 100644\n> > > --- a/src/gstreamer/gstlibcamera-controls.h\n> > > +++ b/src/gstreamer/gstlibcamera-controls.h\n> > > @@ -36,8 +36,10 @@ private:\n> > >   ControlInfoMap capabilities_;\n> > >   /* Set of user modified controls. */\n> > >   ControlList controls_;\n> > > - /* Accumulator of all controls ever set and metadata returned by camera */\n> > > + /* Accumulator of all controls ever */\n\ns/ever/ever set/\n\n> > >   ControlList controls_acc_;\n> \n> Do we need a controls_acc_ at all ? What does this do ?\n> \n> I'm weary because some of the controls are 'triggers' so they should\n> only be set once - not on every frame ... ?\n\nThere are also dependencies between some controls, and I don't think we\nguarantee that setting A=a, B=b in the same request will have the same\neffect as setting A=a in a first request and B=b in a second request.\nAccumulating the controls may not match the actual state of the device.\n\nFor instance, look at the documentation of ColourGains and\nColourTemperature. They respectively state\n\n If ColourGains is set in a request but ColourTemperature is not, the\n implementation shall calculate and set the ColourTemperature based on\n the ColourGains.\n\nand\n\n If ColourTemperature is set in a request but ColourGains is not, the\n implementation shall calculate and set the ColourGains based on the\n given ColourTemperature.\n\nSetting ColourGains=cg in a first request and and ColourTemperature=ct\nin a second request means that ColourGains will end up being calculated\nas CG(ct). Setting them both in the same request means they will both\nhave the values specified by the user. By accumulating controls, we lose\nthis information.\n\n> > > + /* Metadata returned by requests */\n> > > + ControlList metadata_;\n> > >  };\n> > >  \n> > >  } /* namespace libcamera */\n> > \n> > Looks good to me, but I'd like someone from the core to review this\n> > please.","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 D8CD5BE08B\n\tfor ;\n\tTue, 22 Apr 2025 23:34:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2930F68ACA;\n\tWed, 23 Apr 2025 01:34:14 +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 66EC768AC5\n\tfor ;\n\tWed, 23 Apr 2025 01:34:12 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D13416A;\n\tWed, 23 Apr 2025 01:34:11 +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=\"CYnEh8ve\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1745364851;\n\tbh=w+6HLoakkYBOGeqR9Cz3Gi9PDhoR1kdlnuG7zs/othQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CYnEh8venoJKmzfsUD6qQegV65WEyU0APwAXbtQI/haohyNozVl0Q6eNi772kY9Hs\n\tFVhp1y05c7n60OAxR4vKXRJFGJ8RU/UAax3XRjdfWiSyEKbFTRYUahldElYPkE2xvX\n\tgzt3oDHXc2uVeic2bAdSOdBX7eOnWMn1EuRv8NkY=","Date":"Wed, 23 Apr 2025 02:34:09 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Jaslo Ziska , Nicolas Dufresne ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","Message-ID":"<20250422233409.GY17813@pendragon.ideasonboard.com>","References":"<20250422142010.13064-1-jaslo@ziska.de>\n\t<20250422142010.13064-5-jaslo@ziska.de>\n\t<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>\n\t<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>","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":34013,"web_url":"https://patchwork.libcamera.org/comment/34013/","msgid":"<87v7qvyz86.fsf@ziska.de>","date":"2025-04-23T10:00:09","subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","submitter":{"id":173,"url":"https://patchwork.libcamera.org/api/people/173/","name":"Jaslo Ziska","email":"jaslo@ziska.de"},"content":"Hi Nicolas, Kieran, Laurent,\n\nLaurent Pinchart writes:\n> On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote:\n>> Quoting Nicolas Dufresne (2025-04-22 15:42:41)\n>> > Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :\n>> > > When the libcamerasrc element is stopped and started again, \n>> > > the\n>> > > GstCameraControls::setCamera() function is called, but now \n>> > > it is\n>> > > populated with metadata returned by the previous requests. \n>> > > Because\n>> > > GstCameraControls::setCamera() filters the controls, this \n>> > > would cause\n>> > > warnings to be emitted, as the returned metadata are not \n>> > > valid controls\n>> > > which can be set.\n>> >\n>> > Does it also fixes on top the issue where controls that get \n>> > applied\n>> > with delays get resets and forgotten ?\n\nSorry, I don't know which issue you are referring to.\n\n>> >\n>> > > This is fixed by introducing a new member which only holds \n>> > > the metadata\n>> > > returned by the requests, so that the controls and the \n>> > > metadata are now\n>> > > completely independent from each other.\n>> > >\n>> > > Signed-off-by: Jaslo Ziska \n>> > > ---\n>> > >  src/gstreamer/gstlibcamera-controls.cpp.in | 22 \n>> > > ++++++++++++++++------\n>> > >  src/gstreamer/gstlibcamera-controls.h      |  4 +++-\n>> > >  2 files changed, 19 insertions(+), 7 deletions(-)\n>> > >\n>> > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in \n>> > > b/src/gstreamer/gstlibcamera-controls.cpp.in\n>> > > index 89c530da..490e835e 100644\n>> > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n>> > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n>> > > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(\n>> > >  bool GstCameraControls::getProperty(guint propId, GValue \n>> > > *value,\n>> > >       [[maybe_unused]] \n>> > > GParamSpec *pspec)\n>> > >  {\n>> > > - if (!controls_acc_.contains(propId)) {\n>> > > + const ControlValue *cv;\n>> > > + if (controls_acc_.contains(propId)) {\n>> > > + cv = &controls_acc_.get(propId);\n>> > > + } else if (metadata_.contains(propId)) {\n>> > > + cv = &metadata_.get(propId);\n>\n> Why do controls take precedence here ?\n\nI thought it would make sense, but I was wrong, see below.\n\n>\n> Taking a step back, does GSreamer expect the value of properties \n> to be\n> changed dynamically by the device ? If, for instance, \n> auto-exposure is\n> enabled, does GStreamer expect/allow the exposure time and gain\n> properties to be modified to reflect the AGC calculation ?\n\nIn theory these modified values should be available as properties \nif they are in the metadata returned by the requests. But from the \nrest of your comments I see that there is a mistake in my logic, \nsee below.\n\n>\n>> > > + } else {\n>> > >   GST_WARNING(\"Control '%s' is not available, \n>> > > default value will \"\n>> > >       \"be returned\",\n>> > >       \n>> > > controls::controls.at(propId)->name().c_str());\n>> > >   return true;\n>> > >   }\n>> > > - const ControlValue &cv = controls_acc_.get(propId);\n>> > >\n>> > >   switch (propId) {\n>> > >  {%- for vendor, ctrls in controls %}\n>> > >  {%- for ctrl in ctrls %}\n>> > >\n>> > >   case controls::{{ ctrl.namespace }}{{ \n>> > > ctrl.name|snake_case|upper }}: {\n>> > > - auto control = cv.get<{{ ctrl.type }}>();\n>> > > + auto control = cv->get<{{ ctrl.type }}>();\n>> > >\n>> > >  {%- if ctrl.is_array %}\n>> > >   for (size_t i = 0; i < control.size(); ++i) {\n>> > > @@ -309,9 +313,14 @@ void \n>> > > GstCameraControls::setCamera(const \n>> > > std::shared_ptr &cam)\n>> > >       \"camera and will be \n>> > > ignored\",\n>> > >       cid->name().c_str());\n>> > >   }\n>> > > -\n>\n> Please keep the blank line here.\n>\n>> > >   controls_acc_ = new_controls;\n>> > >   controls_ = new_controls;\n>> > > +\n>> > > + /*\n>> > > + * Clear metadata as this might already be populated \n>> > > if the element has\n>> > > + * been running before.\n>> > > + */\n>> > > + metadata_.clear();\n>> > >  }\n>> > >\n>> > >  void \n>> > > GstCameraControls::applyControls(std::unique_ptr \n>> > > &request)\n>> > > @@ -322,6 +331,7 @@ void \n>> > > GstCameraControls::applyControls(std::unique_ptr \n>> > > &reque\n>> > >\n>> > >  void GstCameraControls::readMetadata(libcamera::Request \n>> > > *request)\n>> > >  {\n>> > > - controls_acc_.merge(request->metadata(),\n>> > > -     \n>> > > ControlList::MergePolicy::OverwriteExisting);\n>> > > + metadata_.merge(request->metadata(),\n>> > > + \n>> > > ControlList::MergePolicy::OverwriteExisting);\n>> > > +\n>> > >  }\n>> > > diff --git a/src/gstreamer/gstlibcamera-controls.h \n>> > > b/src/gstreamer/gstlibcamera-controls.h\n>> > > index 749220b5..ee97e61e 100644\n>> > > --- a/src/gstreamer/gstlibcamera-controls.h\n>> > > +++ b/src/gstreamer/gstlibcamera-controls.h\n>> > > @@ -36,8 +36,10 @@ private:\n>> > >   ControlInfoMap capabilities_;\n>> > >   /* Set of user modified controls. */\n>> > >   ControlList controls_;\n>> > > - /* Accumulator of all controls ever set and metadata \n>> > > returned by camera */\n>> > > + /* Accumulator of all controls ever */\n>\n> s/ever/ever set/\n>\n>> > >   ControlList controls_acc_;\n>>\n>> Do we need a controls_acc_ at all ? What does this do ?\n>>\n>> I'm weary because some of the controls are 'triggers' so they \n>> should\n>> only be set once - not on every frame ... ?\n\nNow I'm confused, I thought all controls should only be set once \nwhen they changed? controls_acc_ was introduced for this. The \ncontrols_ member holds the controls which will be set on the next \nrequest and then gets cleared (so that they only get set once). \nBut the control_acc_ variable accumulates the controls so that \nthey can be returned to GSreamer in getProperty().\n\n>\n> There are also dependencies between some controls, and I don't \n> think we\n> guarantee that setting A=a, B=b in the same request will have \n> the same\n> effect as setting A=a in a first request and B=b in a second \n> request.\n> Accumulating the controls may not match the actual state of the \n> device.\n>\n> For instance, look at the documentation of ColourGains and\n> ColourTemperature. They respectively state\n>\n> If ColourGains is set in a request but ColourTemperature \n> is not, the\n> implementation shall calculate and set the \n> ColourTemperature based on\n> the ColourGains.\n>\n> and\n>\n> If ColourTemperature is set in a request but ColourGains \n> is not, the\n> implementation shall calculate and set the ColourGains \n> based on the\n> given ColourTemperature.\n>\n> Setting ColourGains=cg in a first request and and \n> ColourTemperature=ct\n> in a second request means that ColourGains will end up being \n> calculated\n> as CG(ct). Setting them both in the same request means they will \n> both\n> have the values specified by the user. By accumulating controls, \n> we lose\n> this information.\n\nThanks for the explanation. Then it seems like it was correct the \nway it was before? Because when ColourGains is set in the first \nrequest controls_acc_ holds that value and if at a later point \nColourTemperature is set, ColourGains gets overwritten by the \nvalue returned by the metadata of the request. Or am I mistaken?\n\nBest Regards\nJaslo\n\n>\n>> > > + /* Metadata returned by requests */\n>> > > + ControlList metadata_;\n>> > >  };\n>> > >\n>> > >  } /* namespace libcamera */\n>> >\n>> > Looks good to me, but I'd like someone from the core to \n>> > review this\n>> > please.","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 CB2CDC327D\n\tfor ;\n\tWed, 23 Apr 2025 10:00:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 573B068AC6;\n\tWed, 23 Apr 2025 12:00:14 +0200 (CEST)","from mo4-p00-ob.smtp.rzone.de (mo4-p00-ob.smtp.rzone.de\n\t[81.169.146.162])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9FC7C68AC7\n\tfor ;\n\tWed, 23 Apr 2025 12:00:12 +0200 (CEST)","from archlinux by smtp.strato.de (RZmta 51.3.0 AUTH)\n\twith ESMTPSA id z34f8113NA0AULp\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits))\n\t(Client did not present a certificate);\n\tWed, 23 Apr 2025 12:00:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ziska.de header.i=@ziska.de header.b=\"h648ij2H\";\n\tdkim=permerror (0-bit key) header.d=ziska.de header.i=@ziska.de\n\theader.b=\"RVxF/Fi+\"; dkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1745402410; cv=none;\n\td=strato.com; s=strato-dkim-0002;\n\tb=ELurT2Gs1ztmEd6/JPOH4u7nWNnJ/27u3G6YHuw8rzjtKo7Ovn/bYPmldLCGNBMGAe\n\tEo2xM35CmC7kVbKsEy9Eb0hKl0ZqZOjFX3zhb+yTH45QmDkQKEFPQlBTuDHr+GM92HiR\n\t18jDtMCgEPjblpB6UJjQ45X4a28BblQG14xWSGYztZkBeAQL1YvttHdAcExpunfg+7rO\n\tskYYhgCUb9Jo4Xyvq5SiWCzHqeFQA9w8HmqWKOs3XmawdgTZeMsQLL4Nb1HeexOwUiEn\n\t4lgGrk7rSgOBHYU4qSQMqi2j2r00h7YXXSXiY0HTMfZs4Hc2bgOglrq1cQHhHLPc/gN0\n\tGlpQ==","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; t=1745402410;\n\ts=strato-dkim-0002; d=strato.com;\n\th=Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Cc:Date:\n\tFrom:Subject:Sender;\n\tbh=7m4Lc811AeKDNqLAw3EcHiMoi9U6eAIUs8sYfzjVF4Y=;\n\tb=OuLnXXFrGMXtwC+5Mhsygz4DdyUdoYBxLSaDn0m4q1eYevqNubKlYLRlH9p8quLBi9\n\t+nE9VpToQ7BPq4VIwRywUae0kI2zFMqOznp8An4vJ6553z8bVzrEyo+Kd5klseA0NAzC\n\t7wHpCzeSOFGKijjXnVUnSTyA/miTFguLCGUuoeokMpJyCmWrsBI9Z7iNDC1RmHhmCWwr\n\tHLrdYvDgyizUMUqD/UjHSE1JEH48tJUjsOhiY9t+9OfWi3MR8+ogwkXekvT+VthXJRfP\n\t5NrFP7LEF46/jQek7++2VgImpW9bhLQ1NkxO3sxfAE1Nl44tA7UeLHFgvw37mGrLypfQ\n\tKi3g==","ARC-Authentication-Results":"i=1; strato.com;\n arc=none;\n dkim=none","X-RZG-CLASS-ID":"mo00","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; t=1745402410;\n\ts=strato-dkim-0002; d=ziska.de;\n\th=Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Cc:Date:\n\tFrom:Subject:Sender;\n\tbh=7m4Lc811AeKDNqLAw3EcHiMoi9U6eAIUs8sYfzjVF4Y=;\n\tb=h648ij2H+KHr+/KLRolIdBPCkAjeCVSeOC9g+FgKtaIbNia6KP1NGnSNg2zo5bME0B\n\twqQNtWBowiQnkHYyNJmEotKD4ZlI5ePD8It5/IwuK6Iht07OC8x6+6UEtEhB3u6KygG5\n\t7ArXND0nMAXjC2Vf2C31QwdW2i1MDpQY7GYKDBVWcEDHVylfjo7XpteRODzpTY5PGyIV\n\tyo4Lwz9lNsCLy4S7KgjToR2YVzAU2PSRBa1DrRf5prGxOyZBWMNiW/IA0nour7tvlKsM\n\tlWUGeMVeLBd9wAjG/JMBVbvxoTwObGafQjv/XVRgppgt7ko9VClz3sN+Tuk68ty1W/h0\n\t2Obw==","v=1; a=ed25519-sha256; c=relaxed/relaxed; t=1745402410;\n\ts=strato-dkim-0003; d=ziska.de;\n\th=Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Cc:Date:\n\tFrom:Subject:Sender;\n\tbh=7m4Lc811AeKDNqLAw3EcHiMoi9U6eAIUs8sYfzjVF4Y=;\n\tb=RVxF/Fi+2+09n+PkKxb2pi5iR3BI7kJrKTy5ATXfUXZrT1G6gQ/vfQvhrQ8RL34b9E\n\tqz5d35vuJp8APB62duBA=="],"X-RZG-AUTH":"\":Jm0XeU+IYfb0x77LHmrjN5Wlb7TBwusDqIM6Hizy8VdfzvKi4yoFC9cC1Yq6XPJaRR/P73in+Ce40I+pppMPSbg4/XGF\"","From":"Jaslo Ziska ","To":"Laurent Pinchart ","Cc":"Kieran Bingham , Nicolas Dufresne\n\t, libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","In-Reply-To":"<20250422233409.GY17813@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 23 Apr 2025 02:34:09 +0300\")","References":"<20250422142010.13064-1-jaslo@ziska.de>\n\t<20250422142010.13064-5-jaslo@ziska.de>\n\t<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>\n\t<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>\n\t<20250422233409.GY17813@pendragon.ideasonboard.com>","User-Agent":"mu4e 1.12.9; emacs 30.1","Date":"Wed, 23 Apr 2025 12:00:09 +0200","Message-ID":"<87v7qvyz86.fsf@ziska.de>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8; format=flowed","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":34016,"web_url":"https://patchwork.libcamera.org/comment/34016/","msgid":"<6b2915b24ad6fc9dd0022eea78200903429a0ef6.camel@ndufresne.ca>","date":"2025-04-23T12:56:08","subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","submitter":{"id":30,"url":"https://patchwork.libcamera.org/api/people/30/","name":"Nicolas Dufresne","email":"nicolas@ndufresne.ca"},"content":"Le mercredi 23 avril 2025 à 02:34 +0300, Laurent Pinchart a écrit :\n> On Tue, Apr 22, 2025 at 04:33:02PM +0100, Kieran Bingham wrote:\n> > Quoting Nicolas Dufresne (2025-04-22 15:42:41)\n> > > Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :\n> > > > When the libcamerasrc element is stopped and started again, the\n> > > > GstCameraControls::setCamera() function is called, but now it is\n> > > > populated with metadata returned by the previous requests. Because\n> > > > GstCameraControls::setCamera() filters the controls, this would cause\n> > > > warnings to be emitted, as the returned metadata are not valid controls\n> > > > which can be set.\n> > > \n> > > Does it also fixes on top the issue where controls that get applied\n> > > with delays get resets and forgotten ?\n> > > \n> > > > This is fixed by introducing a new member which only holds the metadata\n> > > > returned by the requests, so that the controls and the metadata are now\n> > > > completely independent from each other.\n> > > > \n> > > > Signed-off-by: Jaslo Ziska \n> > > > ---\n> > > >  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------\n> > > >  src/gstreamer/gstlibcamera-controls.h      |  4 +++-\n> > > >  2 files changed, 19 insertions(+), 7 deletions(-)\n> > > > \n> > > > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > index 89c530da..490e835e 100644\n> > > > --- a/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in\n> > > > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(\n> > > >  bool GstCameraControls::getProperty(guint propId, GValue *value,\n> > > >                                   [[maybe_unused]] GParamSpec *pspec)\n> > > >  {\n> > > > -     if (!controls_acc_.contains(propId)) {\n> > > > +     const ControlValue *cv;\n> > > > +     if (controls_acc_.contains(propId)) {\n> > > > +             cv = &controls_acc_.get(propId);\n> > > > +     } else if (metadata_.contains(propId)) {\n> > > > +             cv = &metadata_.get(propId);\n> \n> Why do controls take precedence here ?\n> \n> Taking a step back, does GSreamer expect the value of properties to be\n> changed dynamically by the device ? If, for instance, auto-exposure is\n> enabled, does GStreamer expect/allow the exposure time and gain\n> properties to be modified to reflect the AGC calculation ?\n\nProperties can be used for that yes. When they change, I'd expect a\ncall to g_object_notify() to notify users of the change.\n\nI think what we are missing is the clear split between the properties\nare are set by the application, vs the one that are set by libcamera.\nThis is why we excluded the second bread in the initial control\nsupport.\n\nNicolas\n\n> \n> > > > +     } else {\n> > > >               GST_WARNING(\"Control '%s' is not available, default value will \"\n> > > >                           \"be returned\",\n> > > >                           controls::controls.at(propId)->name().c_str());\n> > > >               return true;\n> > > >       }\n> > > > -     const ControlValue &cv = controls_acc_.get(propId);\n> > > >  \n> > > >       switch (propId) {\n> > > >  {%- for vendor, ctrls in controls %}\n> > > >  {%- for ctrl in ctrls %}\n> > > >  \n> > > >       case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {\n> > > > -             auto control = cv.get<{{ ctrl.type }}>();\n> > > > +             auto control = cv->get<{{ ctrl.type }}>();\n> > > >  \n> > > >  {%- if ctrl.is_array %}\n> > > >               for (size_t i = 0; i < control.size(); ++i) {\n> > > > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr &cam)\n> > > >                                   \"camera and will be ignored\",\n> > > >                                   cid->name().c_str());\n> > > >       }\n> > > > -\n> \n> Please keep the blank line here.\n> \n> > > >       controls_acc_ = new_controls;\n> > > >       controls_ = new_controls;\n> > > > +\n> > > > +     /*\n> > > > +      * Clear metadata as this might already be populated if the element has\n> > > > +      * been running before.\n> > > > +      */\n> > > > +     metadata_.clear();\n> > > >  }\n> > > >  \n> > > >  void GstCameraControls::applyControls(std::unique_ptr &request)\n> > > > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr &reque\n> > > >  \n> > > >  void GstCameraControls::readMetadata(libcamera::Request *request)\n> > > >  {\n> > > > -     controls_acc_.merge(request->metadata(),\n> > > > -                         ControlList::MergePolicy::OverwriteExisting);\n> > > > +     metadata_.merge(request->metadata(),\n> > > > +                     ControlList::MergePolicy::OverwriteExisting);\n> > > > +\n> > > >  }\n> > > > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h\n> > > > index 749220b5..ee97e61e 100644\n> > > > --- a/src/gstreamer/gstlibcamera-controls.h\n> > > > +++ b/src/gstreamer/gstlibcamera-controls.h\n> > > > @@ -36,8 +36,10 @@ private:\n> > > >       ControlInfoMap capabilities_;\n> > > >       /* Set of user modified controls. */\n> > > >       ControlList controls_;\n> > > > -     /* Accumulator of all controls ever set and metadata returned by camera */\n> > > > +     /* Accumulator of all controls ever */\n> \n> s/ever/ever set/\n> \n> > > >       ControlList controls_acc_;\n> > \n> > Do we need a controls_acc_ at all ? What does this do ?\n> > \n> > I'm weary because some of the controls are 'triggers' so they should\n> > only be set once - not on every frame ... ?\n> \n> There are also dependencies between some controls, and I don't think we\n> guarantee that setting A=a, B=b in the same request will have the same\n> effect as setting A=a in a first request and B=b in a second request.\n> Accumulating the controls may not match the actual state of the device.\n> \n> For instance, look at the documentation of ColourGains and\n> ColourTemperature. They respectively state\n> \n>         If ColourGains is set in a request but ColourTemperature is not, the\n>         implementation shall calculate and set the ColourTemperature based on\n>         the ColourGains.\n> \n> and\n> \n>         If ColourTemperature is set in a request but ColourGains is not, the\n>         implementation shall calculate and set the ColourGains based on the\n>         given ColourTemperature.\n> \n> Setting ColourGains=cg in a first request and and ColourTemperature=ct\n> in a second request means that ColourGains will end up being calculated\n> as CG(ct). Setting them both in the same request means they will both\n> have the values specified by the user. By accumulating controls, we lose\n> this information.\n> \n> > > > +     /* Metadata returned by requests */\n> > > > +     ControlList metadata_;\n> > > >  };\n> > > >  \n> > > >  } /* namespace libcamera */\n> > > \n> > > Looks good to me, but I'd like someone from the core to review this\n> > > please.","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 09556C327D\n\tfor ;\n\tWed, 23 Apr 2025 12:56:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FE3768ACE;\n\tWed, 23 Apr 2025 14:56:12 +0200 (CEST)","from mail-qt1-x835.google.com (mail-qt1-x835.google.com\n\t[IPv6:2607:f8b0:4864:20::835])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E73EA68AC7\n\tfor ;\n\tWed, 23 Apr 2025 14:56:10 +0200 (CEST)","by mail-qt1-x835.google.com with SMTP id\n\td75a77b69052e-4766631a6a4so69106561cf.2\n\tfor ;\n\tWed, 23 Apr 2025 05:56:10 -0700 (PDT)","from ?IPv6:2606:6d00:15:9913::5ac? ([2606:6d00:15:9913::5ac])\n\tby smtp.gmail.com with ESMTPSA id\n\t6a1803df08f44-6f2c2b0cb51sm70077026d6.31.2025.04.23.05.56.08\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 23 Apr 2025 05:56:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ndufresne-ca.20230601.gappssmtp.com\n\theader.i=@ndufresne-ca.20230601.gappssmtp.com\n\theader.b=\"qyz9dwN2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ndufresne-ca.20230601.gappssmtp.com; s=20230601; t=1745412970;\n\tx=1746017770; darn=lists.libcamera.org; \n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:from:to:cc:subject\n\t:date:message-id:reply-to;\n\tbh=YPNLfA7J64Vf0TTfY4g5hGFqO+CBMvlBhtFxG7kx1LE=;\n\tb=qyz9dwN2j+2L+hxLVu4bdEwjEpVmrdOt0Youwh/bJBB2Djl12Z2AR4tFGQjxRboDqP\n\tK9ofK9pkilrQhlsLmsPZvjwasXilTvFzRfIsMo70wjgpAVT0ce1Xe89ndYcx7K5mbaTE\n\tMYnLzLIfrQtX2vXVm7XnOph5m+fr/dniHHx2Wg9VB0bTFF9o3j/JAreMFUmyLfXA1iaH\n\tem2uDg4GN1XAuAAA4+gIkbb2RIzjDnMd8+RMv2fK0YZ7d9di5/G38zNX573vWl52NTUg\n\t3aAiHtBkAicp93/2PbEGTxC3G72j2mQyLYCSbauDht2iEnltPbbTTdL2PiWNlaJw42WS\n\tf7Sw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1745412970; x=1746017770;\n\th=mime-version:user-agent:content-transfer-encoding:references\n\t:in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=YPNLfA7J64Vf0TTfY4g5hGFqO+CBMvlBhtFxG7kx1LE=;\n\tb=rcz8JkyMi24bfBwJo3kWdgpF8wC5Xm0zoTA1X50ebkQ2lpl54ipnl57C8dEZEN6+wJ\n\tzOo5DX7gcNOwHNFLEEt8iPUUGIAp9wcIO6q4jVSjimhGbimnmXO8wer93m8Q2oXnRYU6\n\tQkGeLWc+5x5KHZuLFXCBjq+6PCpl4oiaNRlK0aHZljcBpbhGkTtXxZITzlGjgn3Th50O\n\tecBfKfwkmXHMI1VpSK+LCogvjWt2MUWH6/C0HBZwfyEtSSqOPOAVzouNM5COi7uxeliH\n\t9IqwgVd8X5uxoZ2JNA3diW230EbNoUdL8mX5XzJxi42PF6uFZiSeAqCEkwaZ4lVRpeBh\n\tr6NA==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCUQDyjAQntz2EsZuss471RQczZTv/kiTvBJpAVl+UhHJT0syGifWgM25fUdy0+3hy9CCOqKvZTexoSKwb6tcxs=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzyWKNQ63xPTpy+ZqVuTdTokxrO1mdM7vFw6JeAhtrQ4h2gF8r9\n\tQWIXHjwkSZFnUyWyoxhEoxZfhnJNzk/HvLZti5k8Fl7StRADNC4VWfgZXasUrUnpNwYiGsBv2Bw\n\tgq7U=","X-Gm-Gg":"ASbGncsqyYKRfTLhdS2fviL45wVRQilDleaRUSS5IV7gsv+9gVfhXXMnKIVxzmFDxGK\n\t2p4LKQwRAe3HtsCMu0b0zniZQUXCDcq4ako91v2Azs9Mkc5kEmun0MyXHy6oKNL6GILBRPM5g1j\n\tBxxw0vO+oWMTl+KchZrkwqQPYSTtNHEqT56895ZA8UBDfT6hivSlxNugcqTbkgwnhEhOxcevjmn\n\tKPlBnJz+9EgwaFrL2JmnuIT4T8m0+L5e29jFigpCEHlK8hTrAROr4hyooIjAyuv5CAIgEqHFYmt\n\tOC3KoU8XKDAzSEuMxe97l9DaJpG3Eu+ACn+bNvPgt+k30qDf30vWdeI8","X-Google-Smtp-Source":"AGHT+IGEI/gVCnCuRHPuD0UlBCu852dDCChqTcO+kTGAH4tGX4UnajAwkuNLVmQqaWbHPQ9oXq8Oug==","X-Received":"by 2002:a05:6214:1787:b0:6f4:b265:261 with SMTP id\n\t6a1803df08f44-6f4b26507e5mr60759206d6.8.1745412969620; \n\tWed, 23 Apr 2025 05:56:09 -0700 (PDT)","Message-ID":"<6b2915b24ad6fc9dd0022eea78200903429a0ef6.camel@ndufresne.ca>","Subject":"Re: [PATCH 4/4] gstreamer: Split metadata controls into new member","From":"Nicolas Dufresne ","To":"Laurent Pinchart , Kieran Bingham\n\t","Cc":"Jaslo Ziska , libcamera-devel@lists.libcamera.org","Date":"Wed, 23 Apr 2025 08:56:08 -0400","In-Reply-To":"<20250422233409.GY17813@pendragon.ideasonboard.com>","References":"<20250422142010.13064-1-jaslo@ziska.de>\n\t<20250422142010.13064-5-jaslo@ziska.de>\n\t<5058fc5c3261be58e6d2c916b29e9722bb69cda4.camel@ndufresne.ca>\n\t<174533598226.891349.6199939796201399874@ping.linuxembedded.co.uk>\n\t<20250422233409.GY17813@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","User-Agent":"Evolution 3.56.0 (3.56.0-1.fc42) ","MIME-Version":"1.0","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\" "}}]