[4/4] gstreamer: Split metadata controls into new member
diff mbox

Message ID 20250422142010.13064-5-jaslo@ziska.de
State New
Headers show

Commit Message

Jaslo Ziska April 22, 2025, 2:11 p.m. UTC
When the libcamerasrc element is stopped and started again, the
GstCameraControls::setCamera() function is called, but now it is
populated with metadata returned by the previous requests. Because
GstCameraControls::setCamera() filters the controls, this would cause
warnings to be emitted, as the returned metadata are not valid controls
which can be set.

This is fixed by introducing a new member which only holds the metadata
returned by the requests, so that the controls and the metadata are now
completely independent from each other.

Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
---
 src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------
 src/gstreamer/gstlibcamera-controls.h      |  4 +++-
 2 files changed, 19 insertions(+), 7 deletions(-)

Comments

Nicolas Dufresne April 22, 2025, 2:42 p.m. UTC | #1
Hi,

Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :
> When the libcamerasrc element is stopped and started again, the
> GstCameraControls::setCamera() function is called, but now it is
> populated with metadata returned by the previous requests. Because
> GstCameraControls::setCamera() filters the controls, this would cause
> warnings to be emitted, as the returned metadata are not valid controls
> which can be set.

Does it also fixes on top the issue where controls that get applied
with delays get resets and forgotten ?

> 
> This is fixed by introducing a new member which only holds the metadata
> returned by the requests, so that the controls and the metadata are now
> completely independent from each other.
> 
> Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
> ---
>  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------
>  src/gstreamer/gstlibcamera-controls.h      |  4 +++-
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in
> index 89c530da..490e835e 100644
> --- a/src/gstreamer/gstlibcamera-controls.cpp.in
> +++ b/src/gstreamer/gstlibcamera-controls.cpp.in
> @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(
>  bool GstCameraControls::getProperty(guint propId, GValue *value,
>  				    [[maybe_unused]] GParamSpec *pspec)
>  {
> -	if (!controls_acc_.contains(propId)) {
> +	const ControlValue *cv;
> +	if (controls_acc_.contains(propId)) {
> +		cv = &controls_acc_.get(propId);
> +	} else if (metadata_.contains(propId)) {
> +		cv = &metadata_.get(propId);
> +	} else {
>  		GST_WARNING("Control '%s' is not available, default value will "
>  			    "be returned",
>  			    controls::controls.at(propId)->name().c_str());
>  		return true;
>  	}
> -	const ControlValue &cv = controls_acc_.get(propId);
>  
>  	switch (propId) {
>  {%- for vendor, ctrls in controls %}
>  {%- for ctrl in ctrls %}
>  
>  	case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {
> -		auto control = cv.get<{{ ctrl.type }}>();
> +		auto control = cv->get<{{ ctrl.type }}>();
>  
>  {%- if ctrl.is_array %}
>  		for (size_t i = 0; i < control.size(); ++i) {
> @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam)
>  				    "camera and will be ignored",
>  				    cid->name().c_str());
>  	}
> -
>  	controls_acc_ = new_controls;
>  	controls_ = new_controls;
> +
> +	/*
> +	 * Clear metadata as this might already be populated if the element has
> +	 * been running before.
> +	 */
> +	metadata_.clear();
>  }
>  
>  void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request)
> @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque
>  
>  void GstCameraControls::readMetadata(libcamera::Request *request)
>  {
> -	controls_acc_.merge(request->metadata(),
> -			    ControlList::MergePolicy::OverwriteExisting);
> +	metadata_.merge(request->metadata(),
> +			ControlList::MergePolicy::OverwriteExisting);
> +
>  }
> diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h
> index 749220b5..ee97e61e 100644
> --- a/src/gstreamer/gstlibcamera-controls.h
> +++ b/src/gstreamer/gstlibcamera-controls.h
> @@ -36,8 +36,10 @@ private:
>  	ControlInfoMap capabilities_;
>  	/* Set of user modified controls. */
>  	ControlList controls_;
> -	/* Accumulator of all controls ever set and metadata returned by camera */
> +	/* Accumulator of all controls ever */
>  	ControlList controls_acc_;
> +	/* Metadata returned by requests */
> +	ControlList metadata_;
>  };
>  
>  } /* namespace libcamera */

Looks good to me, but I'd like someone from the core to review this
please.

Nicolas
Kieran Bingham April 22, 2025, 3:33 p.m. UTC | #2
Quoting Nicolas Dufresne (2025-04-22 15:42:41)
> Hi,
> 
> Le mardi 22 avril 2025 à 16:11 +0200, Jaslo Ziska a écrit :
> > When the libcamerasrc element is stopped and started again, the
> > GstCameraControls::setCamera() function is called, but now it is
> > populated with metadata returned by the previous requests. Because
> > GstCameraControls::setCamera() filters the controls, this would cause
> > warnings to be emitted, as the returned metadata are not valid controls
> > which can be set.
> 
> Does it also fixes on top the issue where controls that get applied
> with delays get resets and forgotten ?
> 
> > 
> > This is fixed by introducing a new member which only holds the metadata
> > returned by the requests, so that the controls and the metadata are now
> > completely independent from each other.
> > 
> > Signed-off-by: Jaslo Ziska <jaslo@ziska.de>
> > ---
> >  src/gstreamer/gstlibcamera-controls.cpp.in | 22 ++++++++++++++++------
> >  src/gstreamer/gstlibcamera-controls.h      |  4 +++-
> >  2 files changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in
> > index 89c530da..490e835e 100644
> > --- a/src/gstreamer/gstlibcamera-controls.cpp.in
> > +++ b/src/gstreamer/gstlibcamera-controls.cpp.in
> > @@ -153,20 +153,24 @@ g_param_spec_{{ ctrl.gtype }}(
> >  bool GstCameraControls::getProperty(guint propId, GValue *value,
> >                                   [[maybe_unused]] GParamSpec *pspec)
> >  {
> > -     if (!controls_acc_.contains(propId)) {
> > +     const ControlValue *cv;
> > +     if (controls_acc_.contains(propId)) {
> > +             cv = &controls_acc_.get(propId);
> > +     } else if (metadata_.contains(propId)) {
> > +             cv = &metadata_.get(propId);
> > +     } else {
> >               GST_WARNING("Control '%s' is not available, default value will "
> >                           "be returned",
> >                           controls::controls.at(propId)->name().c_str());
> >               return true;
> >       }
> > -     const ControlValue &cv = controls_acc_.get(propId);
> >  
> >       switch (propId) {
> >  {%- for vendor, ctrls in controls %}
> >  {%- for ctrl in ctrls %}
> >  
> >       case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {
> > -             auto control = cv.get<{{ ctrl.type }}>();
> > +             auto control = cv->get<{{ ctrl.type }}>();
> >  
> >  {%- if ctrl.is_array %}
> >               for (size_t i = 0; i < control.size(); ++i) {
> > @@ -309,9 +313,14 @@ void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam)
> >                                   "camera and will be ignored",
> >                                   cid->name().c_str());
> >       }
> > -
> >       controls_acc_ = new_controls;
> >       controls_ = new_controls;
> > +
> > +     /*
> > +      * Clear metadata as this might already be populated if the element has
> > +      * been running before.
> > +      */
> > +     metadata_.clear();
> >  }
> >  
> >  void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request)
> > @@ -322,6 +331,7 @@ void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque
> >  
> >  void GstCameraControls::readMetadata(libcamera::Request *request)
> >  {
> > -     controls_acc_.merge(request->metadata(),
> > -                         ControlList::MergePolicy::OverwriteExisting);
> > +     metadata_.merge(request->metadata(),
> > +                     ControlList::MergePolicy::OverwriteExisting);
> > +
> >  }
> > diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h
> > index 749220b5..ee97e61e 100644
> > --- a/src/gstreamer/gstlibcamera-controls.h
> > +++ b/src/gstreamer/gstlibcamera-controls.h
> > @@ -36,8 +36,10 @@ private:
> >       ControlInfoMap capabilities_;
> >       /* Set of user modified controls. */
> >       ControlList controls_;
> > -     /* Accumulator of all controls ever set and metadata returned by camera */
> > +     /* Accumulator of all controls ever */
> >       ControlList controls_acc_;

Do we need a controls_acc_ at all ? What does this do ?

I'm weary because some of the controls are 'triggers' so they should
only be set once - not on every frame ... ?

> > +     /* Metadata returned by requests */
> > +     ControlList metadata_;
> >  };
> >  
> >  } /* namespace libcamera */
> 
> Looks good to me, but I'd like someone from the core to review this
> please.
> 
> Nicolas

Patch
diff mbox

diff --git a/src/gstreamer/gstlibcamera-controls.cpp.in b/src/gstreamer/gstlibcamera-controls.cpp.in
index 89c530da..490e835e 100644
--- a/src/gstreamer/gstlibcamera-controls.cpp.in
+++ b/src/gstreamer/gstlibcamera-controls.cpp.in
@@ -153,20 +153,24 @@  g_param_spec_{{ ctrl.gtype }}(
 bool GstCameraControls::getProperty(guint propId, GValue *value,
 				    [[maybe_unused]] GParamSpec *pspec)
 {
-	if (!controls_acc_.contains(propId)) {
+	const ControlValue *cv;
+	if (controls_acc_.contains(propId)) {
+		cv = &controls_acc_.get(propId);
+	} else if (metadata_.contains(propId)) {
+		cv = &metadata_.get(propId);
+	} else {
 		GST_WARNING("Control '%s' is not available, default value will "
 			    "be returned",
 			    controls::controls.at(propId)->name().c_str());
 		return true;
 	}
-	const ControlValue &cv = controls_acc_.get(propId);
 
 	switch (propId) {
 {%- for vendor, ctrls in controls %}
 {%- for ctrl in ctrls %}
 
 	case controls::{{ ctrl.namespace }}{{ ctrl.name|snake_case|upper }}: {
-		auto control = cv.get<{{ ctrl.type }}>();
+		auto control = cv->get<{{ ctrl.type }}>();
 
 {%- if ctrl.is_array %}
 		for (size_t i = 0; i < control.size(); ++i) {
@@ -309,9 +313,14 @@  void GstCameraControls::setCamera(const std::shared_ptr<libcamera::Camera> &cam)
 				    "camera and will be ignored",
 				    cid->name().c_str());
 	}
-
 	controls_acc_ = new_controls;
 	controls_ = new_controls;
+
+	/*
+	 * Clear metadata as this might already be populated if the element has
+	 * been running before.
+	 */
+	metadata_.clear();
 }
 
 void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &request)
@@ -322,6 +331,7 @@  void GstCameraControls::applyControls(std::unique_ptr<libcamera::Request> &reque
 
 void GstCameraControls::readMetadata(libcamera::Request *request)
 {
-	controls_acc_.merge(request->metadata(),
-			    ControlList::MergePolicy::OverwriteExisting);
+	metadata_.merge(request->metadata(),
+			ControlList::MergePolicy::OverwriteExisting);
+
 }
diff --git a/src/gstreamer/gstlibcamera-controls.h b/src/gstreamer/gstlibcamera-controls.h
index 749220b5..ee97e61e 100644
--- a/src/gstreamer/gstlibcamera-controls.h
+++ b/src/gstreamer/gstlibcamera-controls.h
@@ -36,8 +36,10 @@  private:
 	ControlInfoMap capabilities_;
 	/* Set of user modified controls. */
 	ControlList controls_;
-	/* Accumulator of all controls ever set and metadata returned by camera */
+	/* Accumulator of all controls ever */
 	ControlList controls_acc_;
+	/* Metadata returned by requests */
+	ControlList metadata_;
 };
 
 } /* namespace libcamera */