| Message ID | 20240522145438.436688-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-05-22 15:54:37) > std::map::merge(source) has the side effect of actually moving items from > 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@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@ideasonboard.com> > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > } > > -- > 2.40.1 >
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@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@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 > >
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@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@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 > > >
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()); *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); }
std::map::merge(source) has the side effect of actually moving items from source to target. In this case the controls where removed from the source maps on the first call to updateControls() and on the second call to updateControls() they where missing in the source maps and therefore also removed from the camera. Fix this by using insert() instead of merge(). This is most likely cheaper than copy-contructing the source map. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/rkisp1.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)