<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 6, 2021 at 4:44 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Thu, May 06, 2021 at 01:58:34PM +0900, Hirokazu Honda wrote:<br>
> On Tue, May 4, 2021 at 5:11 PM Kieran Bingham <<br>
> <a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>> wrote:<br>
><br>
> > Hi Jacopo,<br>
> ><br>
> > On 03/05/2021 11:41, Jacopo Mondi wrote:<br>
> > > From: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > ><br>
> > > Add a new ControlList::merge() function to merge two control lists by<br>
> > > copying in the values in the list passed as parameters.<br>
> > ><br>
> > > This can be used by pipeline handlers to merge metadata they populate<br>
> > > with metadata received from an IPA.<br>
> > ><br>
> > > Signed-off-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
> > > [reimplement the function by not using std::unordered_map::merge()]<br>
> > > Signed-off-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
> ><br>
> > With the cost/const fixed<br>
> ><br>
> > Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> ><br>
> > > ---<br>
> > >  include/libcamera/controls.h |  2 ++<br>
> > >  src/libcamera/controls.cpp   | 30 ++++++++++++++++++++++++++++++<br>
> > >  2 files changed, 32 insertions(+)<br>
> > ><br>
> > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h<br>
> > > index 1a5690a5ccbe..1c9b37e617bc 100644<br>
> > > --- a/include/libcamera/controls.h<br>
> > > +++ b/include/libcamera/controls.h<br>
> > > @@ -363,7 +363,9 @@ public:<br>
> > ><br>
> > >       bool empty() const { return controls_.empty(); }<br>
> > >       std::size_t size() const { return controls_.size(); }<br>
> > > +<br>
> > >       void clear() { controls_.clear(); }<br>
> > > +     void merge(const ControlList &source);<br>
> > ><br>
> > >       bool contains(const ControlId &id) const;<br>
> > >       bool contains(unsigned int id) const;<br>
> > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp<br>
> > > index c58ed3946f3b..d8f104e3734b 100644<br>
> > > --- a/src/libcamera/controls.cpp<br>
> > > +++ b/src/libcamera/controls.cpp<br>
> > > @@ -874,6 +874,36 @@ ControlList::ControlList(const ControlInfoMap<br>
> > &infoMap, ControlValidator *valida<br>
> > >   * \brief Removes all controls from the list<br>
> > >   */<br>
> > ><br>
> > > +/**<br>
> > > + * \brief Merge the \a source into the ControlList<br>
> > > + * \param[in] source The ControlList to merge into this object<br>
> > > + *<br>
> > > + * Merging two control lists copies elements from the \a source and<br>
> > inserts<br>
> > > + * them in *this. If the \a source contains elements whose key is<br>
> > already<br>
> > > + * present in *this, then those elements are not overwritten.<br>
> > > + *<br>
> > > + * Only control lists created from the same ControlIdMap or<br>
> > ControlInfoMap may<br>
> > > + * be merged. Attempting to do otherwise results in undefined behaviour.<br>
> > > + *<br>
> > > + * \todo Reimplement or implement an overloaded version which<br>
> > internally uses<br>
> > > + * std::unordered_map::merge() and accepts a non-cost argument.<br>
> > > + */<br>
> > > +void ControlList::merge(const ControlList &source)<br>
> > > +{<br>
> > > +     ASSERT(idmap_ == source.idmap_);<br>
> ><br>
><br>
> Comparing std::unordered_map is O(NlogN) although the following for-loop is<br>
> O(M), where N is the size of idmap_ and M is the size of source.<br>
> Would you consider if this is worth doing?<br>
<br>
Not sure I got this. Is your concern due to the idmap comparison ? Do<br>
you wonder if it is necessary ?<br>
<br>
It seems to me if we skip the comparison, things might seem to work<br>
well, as the control values mapped to ids are stored in controls_ not<br>
in idmap_.<br>
<br>
Thing is, if you construct two control lists from two different<br>
controls maps you have a risk of id collision, in two different<br>
maps the same ControlId can be mapped to a different unsigned int, or<br>
the other way around, two different ControlId can be mapped to the<br>
same numeric identifier.<br>
<br>
How likely is this to happen at the current development state in<br>
mainline libcamera ? Close to 0 I would say.<br>
<br>
However we should consider downstream implementation, which might use<br>
custom controls and control maps, especially in IPA. They could be<br>
tempted to mix them with lists constructed with the canonical<br>
controls::controls and if they're not particularly careful bad things<br>
can happen and can be quite difficult to debug.<br>
<br>
Considering the usual length of the control lists we deal with, I<br>
would say this check is worth it, but maybe I'm being paranoid and we<br>
can safely skip it.<br>
<br>
Or there might be better way to handle this, like<br>
<br>
        for (const auto &ctrl : source) {<br>
                if (idmap_[ctrl.first] != source.idmap_[ctrl.first])<br>
                        /* Error out ludly */<br>
<br>
<br>
        }<br>
<br>
Although operator[] on an unordered map is linear in complexity in the<br>
worst case, so we would get a O(NM) complexity, which is slightly<br>
better than O(NlogNM). We're really talking details here, as with<br>
lists of up to a few tens of items, I don't think the difference is<br>
anyway relevant, but I appreciate the concern on staying as efficient<br>
as we can...<br>
<br></blockquote><div><br></div><div>I missed the time complexity of std::unordered_map.</div></div><blockquote style="margin:0 0 0 40px;border:none;padding:0px"><div class="gmail_quote"><div>Comparing std::unordered_map is O(NlogN) although the following for-loop is</div></div><div class="gmail_quote"><div>O(M), where N is the size of idmap_ and M is the size of source.</div></div></blockquote>This should be O(N) and O(M), respectively.<div>So the original code is better than your proposal one.</div><div><br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
><br>
><br>
> > > +<br>
> > > +     for (const auto &ctrl : source) {<br>
> > > +             if (contains(ctrl.first)) {<br>
> > > +                     const ControlId *id = idmap_->at(ctrl.first);<br>
> > > +                     LOG(Controls, Warning)<br>
> ><br>
><br>
> I wonder if this is worth Warning. Debug seems to be natural for me.<br>
> Could you explain the reason?<br>
<br>
Good question, I went for Warning because I think this is a condition<br>
that should not happen, as an attempt to overwrite an existing control<br>
through merging is suspicious and most probably not desired ? I<br>
refrained from making this an error because it might be intentional,<br>
but to me it's a condition that developers (mostly pipeline<br>
developers) should be aware without going through the extensive Debug<br>
log. As Warnings are less frequent, but ordinarily masked, I considered<br>
this the right level. I can happily change this is if other debug<br>
levels are considered better.<br>
<br></blockquote><div><br></div><div>Ack.</div><div><br></div><div>Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
   j<br>
<br>
><br>
> Thanks,<br>
> -Hiro<br>
><br>
> > > +                             << "Control " << id->name() << " not<br>
> > overwritten";<br>
> > > +                     continue;<br>
> > > +             }<br>
> > > +<br>
> > > +             set(ctrl.first, ctrl.second);<br>
> > > +     }<br>
> > > +}<br>
> > > +<br>
> > >  /**<br>
> > >   * \brief Check if the list contains a control with the specified \a id<br>
> > >   * \param[in] id The control ID<br>
> > ><br>
> ><br>
> > --<br>
> > Regards<br>
> > --<br>
> > Kieran<br>
> > _______________________________________________<br>
> > libcamera-devel mailing list<br>
> > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div></div>