[PATCH v1] libcamera: controls: Add ControlList::merge overload that moves entries

[Barnabás Pőcze]

Hi


2024. október 9., szerda 15:10 keltezéssel, Stefan Klug <stefan.klug at ideasonboard.com> írta:

> When merging two control lists, copies of the entries are made. Add a
> overload of the merge function that accepts a non-const source and
> provides a merge without copy. This was deemed important in the light of
> debug metadata were the data should not be copied more often than
> necessary.
> 
> This change does not break the API, but it changes the behavior of
> applications because the source list now gets modified if it is
> non-const (which might be a common case).
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> 
> ---
> 
> Todo: After we agreed on a approach, the tests need to be adapted
> accordingly.
> ---
>  include/libcamera/controls.h |  1 +
>  src/libcamera/controls.cpp   | 32 +++++++++++++++++++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ca60bbacad17..2c28ab9447a8 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -401,6 +401,7 @@ public:
> 
>  	void clear() { controls_.clear(); }
>  	void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);
> +	void merge(ControlList &source, MergePolicy policy = MergePolicy::KeepExisting);

Why not use an rvalue ref, i.e. `ControlList&&`? I think that would be a
more natural way to express this.


> 
>  	bool contains(unsigned int id) const;
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 62185d643ecd..cb977e1212d8 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -963,11 +963,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap,
>   * present in *this, then those elements are only overwritten if
>   * \a policy is MergePolicy::OverwriteExisting.
>   *
> + * If \a source is non-const, it will contain the dropped entries after the
> + * merge. These are either the overwritten ones if policy was
> + * MergePolicy::OverwriteExisting or the skipped ones if policy was
> + * MergePolicy::KeepExisting
> + *
>   * Only control lists created from the same ControlIdMap or ControlInfoMap may
>   * be merged. Attempting to do otherwise results in undefined behaviour.
> - *
> - * \todo Reimplement or implement an overloaded version which internally uses
> - * std::unordered_map::merge() and accepts a non-const argument.
>   */
>  void ControlList::merge(const ControlList &source, MergePolicy policy)
>  {
> @@ -995,6 +997,30 @@ void ControlList::merge(const ControlList &source, MergePolicy policy)
>  	}
>  }
> 
> +/**
> + * \copydoc ControlList::merge(const ControlList &source, MergePolicy policy)
> + */
> +void ControlList::merge(ControlList &source, MergePolicy policy)
> +{
> +	/**
> +	 * \todo ASSERT that the current and source ControlList are derived
> +	 * from a compatible ControlIdMap, to prevent undefined behaviour due to
> +	 * id collisions.
> +	 *
> +	 * This can not currently be a direct pointer comparison due to the
> +	 * duplication of the ControlIdMaps in the isolated IPA use cases.
> +	 * Furthermore, manually checking each entry of the id map is identical
> +	 * is expensive.
> +	 * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details
> +	 */
> +
> +	if (policy == MergePolicy::OverwriteExisting) {
> +		source.controls_.merge(controls_);
> +		source.controls_.swap(controls_);
> +	} else
> +		controls_.merge(source.controls_);
> +}
> +
>  /**
>   * \brief Check if the list contains a control with the specified \a id
>   * \param[in] id The control numerical ID
> --
> 2.43.0
> 


Regards,
Barnabás Pőcze