[PATCH v2 3/4] ipa: rkisp1: Fix algorithm controls vanish after configure

[Stefan Klug]

Hi Jacopo,

thanks for the review.

On Thu, May 23, 2024 at 10:07:56AM +0200, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Thu, May 23, 2024 at 12:01:51AM GMT, Kieran Bingham wrote:
> > Quoting Stefan Klug (2024-05-22 15:54:37)
> > > std::map::merge(source) has the side effect of actually moving items from
> 
> It's actually an unordered_map which is the ControlInfoMap base class,
> the behaviour is not different though.
> 
> > > source to target. In this case the controls where removed from the source maps
> >
> > s/where/were/
> >
> > > on the first call to updateControls() and on the second call to
> > > updateControls() they where missing in the source maps and therefore also
> >
> > s/where/were/
> >
> > > removed from the camera. Fix this by using insert() instead of merge(). This is
> > > most likely cheaper than copy-contructing the source map.
> >
> > Hrm, https://libcamera.org/api-html/classlibcamera_1_1ControlList.html#afb929046d68faa5dea1b43ce4028721f
> > states that merge is a copy when working on a ControlList - but I see here
> > we're working on a ControlInfoMap ...
> > https://en.cppreference.com/w/cpp/container/map/merge and the behaviour
> > is different :-(
> >
> >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > >  src/ipa/rkisp1/rkisp1.cpp | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index 6687c91e..17474408 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -427,7 +427,7 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
> > >                                                               frameDurations[1],
> > >                                                               frameDurations[2]);
> > >
> > > -       ctrlMap.merge(context_.ctrlMap);
> > > +       ctrlMap.insert(context_.ctrlMap.begin(), context_.ctrlMap.end());
> >
> > I was going to suggest a comment above the .insert() due to the
> > confusion that took place, but it's hard to think of something suitable
> > so maybe it's not helfpul.
> >
> > If this fixes a bug/issue - please add a fixes tag so I can highlight it
> > in the next release notes.
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> There's one thing that puzzles me
> 
> ControlInfoMap privately inherits from unordered_map
> 
>         class ControlInfoMap : private std::unordered_map<const ControlId *, ControlInfo>
> 
> and we don't expose Map::merge as we do for the iterators and a few
> more functions.
> 
> Shouldn't std::unordered_map::merge be unaccessible for users of
> ControlInfoMap ?

ctrlMap is of type ControlInfoMap::Map, so the underlying unordered_map
is used directly. 

Cheers,
Stefan

> 
> >
> >
> > >         *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls);
> > >  }
> > >
> > > --
> > > 2.40.1
> > >