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

12140 diff mbox series

From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
[reimplement the function by not using std::unordered_map::merge()]
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/controls.h |  2 ++
 src/libcamera/controls.cpp   | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

Hi Jacopo/Laurent,

On 30/04/2021 17:00, Jacopo Mondi wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> [reimplement the function by not using std::unordered_map::merge()]
> Signed-off-by: Jacopo Mondi <jacopo@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...

> + * 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 paragraph seems to conflict with itself.
It starts off by saying "copies elements from source ..."
And then says "without copying or moving"

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

Hi Jacopo,

Thanks for your work.

On 2021-04-30 18:00:11 +0200, Jacopo Mondi wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> [reimplement the function by not using std::unordered_map::merge()]
> Signed-off-by: Jacopo Mondi <jacopo@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
> + * 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
> + * nodes are updated, without copying or moving the elements.
> + *
> + * 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;

nit1: Should not 'source' have priority over 'this'? If I'm merging 
something to 'this' I would expect the new stuff to overwrite my 
existing state.

nit2: Maybe add a debug log here? It feels like an issue with the 
potential to reduce someone to tears, "I set the control in the IPA but 
my application still sees the old value, why?"

> +
> +		set(ctrl.first, ctrl.second);
> +	}
> +}
> +
>  /**
>   * \brief Check if the list contains a control with the specified \a id
>   * \param[in] id The control ID
> -- 
> 2.31.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@ideasonboard.com>
> > [reimplement the function by not using std::unordered_map::merge()]
> > Signed-off-by: Jacopo Mondi <jacopo@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

Hi Niklas,

On Sat, May 01, 2021 at 08:41:55AM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-04-30 18:00:11 +0200, Jacopo Mondi wrote:
> > From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> > [reimplement the function by not using std::unordered_map::merge()]
> > Signed-off-by: Jacopo Mondi <jacopo@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
> > + * 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
> > + * nodes are updated, without copying or moving the elements.
> > + *
> > + * 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;
>
> nit1: Should not 'source' have priority over 'this'? If I'm merging
> something to 'this' I would expect the new stuff to overwrite my
> existing state.

That's an interesting question... I decided to here maintain the
semantic of std::unordere_map::merge() here, as if we'll provide an
optimized version based on it, the semantic regarding overwriting does
not change.

>
> nit2: Maybe add a debug log here? It feels like an issue with the
> potential to reduce someone to tears, "I set the control in the IPA but
> my application still sees the old value, why?"
>

Yes, I'm not sure what's best... I considered the idea of returning an
integer to report the number of controls that has not been overwritten
(or the number of the overwritten ones alternatively) but that would
not allow you to know which ones are duplicated in the two lists...

A debug message could work equally well, probably better if it doesn't
get too noisy...

I'll add one

Thanks
   j

> > +
> > +		set(ctrl.first, ctrl.second);
> > +	}
> > +}
> > +
> >  /**
> >   * \brief Check if the list contains a control with the specified \a id
> >   * \param[in] id The control ID
> > --
> > 2.31.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund