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

[Stefan Klug]

On Wed, Oct 09, 2024 at 05:21:42PM +0300, Laurent Pinchart wrote:
> 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 :-)

Yes I thought about that too, but the todo was so explicit that I
did that version first. And who knows, maybe having access to
the remaining control values is of some use... 

Anyways, if everyone is fine with an rvalue, I'll do that.

Cheers,
Stefan

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