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

[Nicolas Dufresne]

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 at 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