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

[Barnabás Pőcze]

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

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

You would still be able to access the remaining control values.


Regards,
Barnabás Pőcze


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