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

[Jacopo Mondi]

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 ?

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