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

[Laurent Pinchart]

On Wed, Oct 09, 2024 at 02:05:29PM +0000, Barnabás Pőcze wrote:
> 2024. október 9., szerda 15:10 keltezéssel, Stefan Klug í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.

I was going to mention the same :-)

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

-- 
Regards,

Laurent Pinchart