[libcamera-devel] [PATCH v4 01/16] libcamera: controls: Add a function to merge two control lists

[Jacopo Mondi]

Hi Kieran,

On Fri, Apr 30, 2021 at 08:04:47PM +0100, Kieran Bingham wrote:
> Hi Jacopo/Laurent,
>
> On 30/04/2021 17:00, Jacopo Mondi wrote:
> > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Add a new ControlList::merge() function to merge two control lists by
> > copying in the values in the list passed as parameters.
> >
> > This can be used by pipeline handlers to merge metadata they populate
> > with metadata received from an IPA.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > [reimplement the function by not using std::unordered_map::merge()]
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/controls.h |  2 ++
> >  src/libcamera/controls.cpp   | 28 ++++++++++++++++++++++++++++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 1a5690a5ccbe..1c9b37e617bc 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -363,7 +363,9 @@ public:
> >
> >  	bool empty() const { return controls_.empty(); }
> >  	std::size_t size() const { return controls_.size(); }
> > +
> >  	void clear() { controls_.clear(); }
> > +	void merge(const ControlList &source);
> >
> >  	bool contains(const ControlId &id) const;
> >  	bool contains(unsigned int id) const;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index c58ed3946f3b..154ca7bfe7c1 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -874,6 +874,34 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida
> >   * \brief Removes all controls from the list
> >   */
> >
> > +/**
> > + * \brief Merge the \a source into the ControlList
> > + * \param[in] source The ControlList to merge into this object
> > + *
> > + * Merging two control lists copies elements from the \a source and insert
>
> s/insert/inserts/
>
> > + * them in *this. If the \a source contains elements whose key is already
> > + * present in *this, then those elements are not overwritten.
> > + * identical to std::unordered_map::merge() in that only internal pointers to
>
> Is there something missing before 'identical'? It's following a period,
> but doesn't seem to be an opening statement for a sentence...

Sorry, my bad, I kept the older version documentation around and
forgot to clean it up, and clearly forgot to re-read carefully enough.

This last lines, and the below one will have to be removed.

>
> > + * nodes are updated, without copying or moving the elements.
>
> This sounds concerning. Can we leak memory or rather point to memory
> which we don't own, and could therefore be released ?

This statement describes the std::unordered_map::merge() semantic. It
will be removed

>
> This paragraph seems to conflict with itself.
> It starts off by saying "copies elements from source ..."
> And then says "without copying or moving"
>

my bad :(

> > + *
> > + * 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-cost argument.
> > + */
> > +void ControlList::merge(const ControlList &source)
> > +{
> > +	ASSERT(idmap_ == source.idmap_);
> > +
> > +	for (const auto &ctrl : source) {
> > +		if (contains(ctrl.first))
> > +			continue;
> > +
> > +		set(ctrl.first, ctrl.second);
> > +	}
> > +}
> > +
> >  /**
> >   * \brief Check if the list contains a control with the specified \a id
> >   * \param[in] id The control ID
> >
>
> --
> Regards
> --
> Kieran