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

[Hirokazu Honda]

On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Hi Jacopo,
>
> On 03/05/2021 11:41, 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>
>
> With the cost/const fixed
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > ---
> >  include/libcamera/controls.h |  2 ++
> >  src/libcamera/controls.cpp   | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 32 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..d8f104e3734b 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -874,6 +874,36 @@ 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
> inserts
> > + * them in *this. If the \a source contains elements whose key is
> already
> > + * present in *this, then those elements are not overwritten.
> > + *
> > + * 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_);
>

Comparing std::unordered_map is O(NlogN) although the following for-loop is
O(M), where N is the size of idmap_ and M is the size of source.
Would you consider if this is worth doing?


> > +
> > +     for (const auto &ctrl : source) {
> > +             if (contains(ctrl.first)) {
> > +                     const ControlId *id = idmap_->at(ctrl.first);
> > +                     LOG(Controls, Warning)
>

I wonder if this is worth Warning. Debug seems to be natural for me.
Could you explain the reason?

Thanks,
-Hiro

> > +                             << "Control " << id->name() << " not
> overwritten";
> > +                     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
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210506/5b1cb47b/attachment.htm>