| Message ID | 20250924124713.3361707-4-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. Quoting Barnabás Pőcze (2025-09-24 14:47:08) > Add an overload of `ControlList::merge()` that takes the source `ControlList` > object via an rvalue. In contrast to the other overload, this one does not > make copies, it uses `std::unordered_map::merge()` to move key-value pairs > from one map to the other. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/controls.h | 1 + > src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- > 2 files changed, 40 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index 32fb31f78..5d4a53c46 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -432,6 +432,7 @@ public: > > void clear() { controls_.clear(); } > void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); > + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); > > bool contains(unsigned int id) const; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 1e1b49e6b..54cd3b703 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, > * > * 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-const argument. This is interesting. My understanding of that comment was that it might be useful to have a implementation of merge() that receives a non-const list that get's filled with the overwritten or skipped controls the same way std::unordered_map::merge does. Now thinking of that, implementing such a version would modify the behavior and potentially introduce misbehavior in code that worked fine before. So maybe we should decide if we want such a version better sooner than later. > */ > void ControlList::merge(const ControlList &source, MergePolicy policy) > { > @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) > } > } > > +/** > + * \brief Merge the \a source into the ControlList > + * \param[in] source The ControlList to merge into this object > + * \param[in] policy Controls if existing elements in *this shall be > + * overwritten > + * > + * Merging two control lists moves 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 only overwritten if > + * \a policy is MergePolicy::OverwriteExisting. > + * > + * Only control lists created from the same ControlIdMap or ControlInfoMap may > + * be merged. Attempting to do otherwise results in undefined behaviour. > + */ > +void ControlList::merge(ControlList &&source, MergePolicy policy) > +{ > + /** > + * \todo ASSERT that the current and source ControlList are derived > + * from a compatible ControlIdMap, to prevent undefined behaviour due to > + * id collisions. > + * > + * This can not currently be a direct pointer comparison due to the > + * duplication of the ControlIdMaps in the isolated IPA use cases. > + * Furthermore, manually checking each entry of the id map is identical > + * is expensive. > + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details > + */ > + > + switch (policy) { > + case MergePolicy::KeepExisting: > + controls_.merge(source.controls_); > + break; > + case MergePolicy::OverwriteExisting: > + source.controls_.merge(controls_); > + controls_.swap(source.controls_); > + break; > + } > +} > + That looks fine. Best regards, Stefan > /** > * \brief Check if the list contains a control with the specified \a id > * \param[in] id The control numerical ID > -- > 2.51.0 >
Hi 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta: > Hi Barnabás, > > Thank you for the patch. > > Quoting Barnabás Pőcze (2025-09-24 14:47:08) >> Add an overload of `ControlList::merge()` that takes the source `ControlList` >> object via an rvalue. In contrast to the other overload, this one does not >> make copies, it uses `std::unordered_map::merge()` to move key-value pairs >> from one map to the other. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/controls.h | 1 + >> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- >> 2 files changed, 40 insertions(+), 3 deletions(-) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index 32fb31f78..5d4a53c46 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -432,6 +432,7 @@ public: >> >> void clear() { controls_.clear(); } >> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); >> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); >> >> bool contains(unsigned int id) const; >> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index 1e1b49e6b..54cd3b703 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, >> * >> * 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-const argument. > > This is interesting. My understanding of that comment was that it might > be useful to have a implementation of merge() that receives a non-const > list that get's filled with the overwritten or skipped controls the same > way std::unordered_map::merge does. Now thinking of that, implementing > such a version would modify the behavior and potentially introduce > misbehavior in code that worked fine before. So maybe we should decide > if we want such a version better sooner than later. Sorry, can you clarify? I thought this new overload satisfies the TODO comment. It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc. But otherwise `source` is usable after a call to this new overload of `merge()` and it will contain the not overwritten / overwritten (depending on `policy`) items. Although the above behaviour is intentionally not described in the documentation. Regards, Barnabás Pőcze > >> */ >> void ControlList::merge(const ControlList &source, MergePolicy policy) >> { >> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) >> } >> } >> >> +/** >> + * \brief Merge the \a source into the ControlList >> + * \param[in] source The ControlList to merge into this object >> + * \param[in] policy Controls if existing elements in *this shall be >> + * overwritten >> + * >> + * Merging two control lists moves 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 only overwritten if >> + * \a policy is MergePolicy::OverwriteExisting. >> + * >> + * Only control lists created from the same ControlIdMap or ControlInfoMap may >> + * be merged. Attempting to do otherwise results in undefined behaviour. >> + */ >> +void ControlList::merge(ControlList &&source, MergePolicy policy) >> +{ >> + /** >> + * \todo ASSERT that the current and source ControlList are derived >> + * from a compatible ControlIdMap, to prevent undefined behaviour due to >> + * id collisions. >> + * >> + * This can not currently be a direct pointer comparison due to the >> + * duplication of the ControlIdMaps in the isolated IPA use cases. >> + * Furthermore, manually checking each entry of the id map is identical >> + * is expensive. >> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details >> + */ >> + >> + switch (policy) { >> + case MergePolicy::KeepExisting: >> + controls_.merge(source.controls_); >> + break; >> + case MergePolicy::OverwriteExisting: >> + source.controls_.merge(controls_); >> + controls_.swap(source.controls_); >> + break; >> + } >> +} >> + > > That looks fine. > > Best regards, > Stefan > >> /** >> * \brief Check if the list contains a control with the specified \a id >> * \param[in] id The control numerical ID >> -- >> 2.51.0 >>
2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta: > Hi > > 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta: >> Hi Barnabás, >> >> Thank you for the patch. >> >> Quoting Barnabás Pőcze (2025-09-24 14:47:08) >>> Add an overload of `ControlList::merge()` that takes the source `ControlList` >>> object via an rvalue. In contrast to the other overload, this one does not >>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs >>> from one map to the other. >>> >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>> --- >>> include/libcamera/controls.h | 1 + >>> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- >>> 2 files changed, 40 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>> index 32fb31f78..5d4a53c46 100644 >>> --- a/include/libcamera/controls.h >>> +++ b/include/libcamera/controls.h >>> @@ -432,6 +432,7 @@ public: >>> void clear() { controls_.clear(); } >>> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); >>> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); >>> bool contains(unsigned int id) const; >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >>> index 1e1b49e6b..54cd3b703 100644 >>> --- a/src/libcamera/controls.cpp >>> +++ b/src/libcamera/controls.cpp >>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, >>> * >>> * 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-const argument. >> >> This is interesting. My understanding of that comment was that it might >> be useful to have a implementation of merge() that receives a non-const >> list that get's filled with the overwritten or skipped controls the same >> way std::unordered_map::merge does. Now thinking of that, implementing >> such a version would modify the behavior and potentially introduce >> misbehavior in code that worked fine before. So maybe we should decide >> if we want such a version better sooner than later. > > Sorry, can you clarify? I thought this new overload satisfies the TODO comment. > It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc. > But otherwise `source` is usable after a call to this new overload of `merge()` > and it will contain the not overwritten / overwritten (depending on `policy`) items. More specifically: Let *this = A u B source = A' u C where A and A' are the sets of items whose keys are present in both sets. If KeepExisting, then source = A', if OverwriteExisting, then source = A, after the call to merge. > Although the above behaviour is intentionally not described in the documentation. > > > Regards, > Barnabás Pőcze > > >> >>> */ >>> void ControlList::merge(const ControlList &source, MergePolicy policy) >>> { >>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) >>> } >>> } >>> +/** >>> + * \brief Merge the \a source into the ControlList >>> + * \param[in] source The ControlList to merge into this object >>> + * \param[in] policy Controls if existing elements in *this shall be >>> + * overwritten >>> + * >>> + * Merging two control lists moves 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 only overwritten if >>> + * \a policy is MergePolicy::OverwriteExisting. >>> + * >>> + * Only control lists created from the same ControlIdMap or ControlInfoMap may >>> + * be merged. Attempting to do otherwise results in undefined behaviour. >>> + */ >>> +void ControlList::merge(ControlList &&source, MergePolicy policy) >>> +{ >>> + /** >>> + * \todo ASSERT that the current and source ControlList are derived >>> + * from a compatible ControlIdMap, to prevent undefined behaviour due to >>> + * id collisions. >>> + * >>> + * This can not currently be a direct pointer comparison due to the >>> + * duplication of the ControlIdMaps in the isolated IPA use cases. >>> + * Furthermore, manually checking each entry of the id map is identical >>> + * is expensive. >>> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details >>> + */ >>> + >>> + switch (policy) { >>> + case MergePolicy::KeepExisting: >>> + controls_.merge(source.controls_); >>> + break; >>> + case MergePolicy::OverwriteExisting: >>> + source.controls_.merge(controls_); >>> + controls_.swap(source.controls_); >>> + break; >>> + } >>> +} >>> + >> >> That looks fine. >> >> Best regards, >> Stefan >> >>> /** >>> * \brief Check if the list contains a control with the specified \a id >>> * \param[in] id The control numerical ID >>> -- >>> 2.51.0 >>> >
Quoting Barnabás Pőcze (2025-10-06 15:37:02) > 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta: > > Hi > > > > 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta: > >> Hi Barnabás, > >> > >> Thank you for the patch. > >> > >> Quoting Barnabás Pőcze (2025-09-24 14:47:08) > >>> Add an overload of `ControlList::merge()` that takes the source `ControlList` > >>> object via an rvalue. In contrast to the other overload, this one does not > >>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs > >>> from one map to the other. > >>> > >>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>> --- > >>> include/libcamera/controls.h | 1 + > >>> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- > >>> 2 files changed, 40 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>> index 32fb31f78..5d4a53c46 100644 > >>> --- a/include/libcamera/controls.h > >>> +++ b/include/libcamera/controls.h > >>> @@ -432,6 +432,7 @@ public: > >>> void clear() { controls_.clear(); } > >>> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); > >>> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); > >>> bool contains(unsigned int id) const; > >>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > >>> index 1e1b49e6b..54cd3b703 100644 > >>> --- a/src/libcamera/controls.cpp > >>> +++ b/src/libcamera/controls.cpp > >>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, > >>> * > >>> * 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-const argument. > >> > >> This is interesting. My understanding of that comment was that it might > >> be useful to have a implementation of merge() that receives a non-const > >> list that get's filled with the overwritten or skipped controls the same > >> way std::unordered_map::merge does. Now thinking of that, implementing > >> such a version would modify the behavior and potentially introduce > >> misbehavior in code that worked fine before. So maybe we should decide > >> if we want such a version better sooner than later. > > > > Sorry, can you clarify? I thought this new overload satisfies the TODO comment. > > It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc. > > But otherwise `source` is usable after a call to this new overload of `merge()` > > and it will contain the not overwritten / overwritten (depending on `policy`) items. > > More specifically: > > Let > *this = A u B > source = A' u C > where A and A' are the sets of items whose keys are present in both sets. > > If KeepExisting, then source = A', > if OverwriteExisting, then source = A, > after the call to merge. Sorry for my ignorance (or a lack of C++ knowledge :-)). How would you use that? Lets say: ControlList l1, l2; l1.set(A, ...); l1.set(B, ...); l2.set(A', ...) // Is this right? l1.merge(std::move(l2),...); Am I allowed to access the data in l2 now? I think I never used move semantics in that way. Best regards, Stefan > > > > Although the above behaviour is intentionally not described in the documentation. > > > > > > Regards, > > Barnabás Pőcze > > > > > >> > >>> */ > >>> void ControlList::merge(const ControlList &source, MergePolicy policy) > >>> { > >>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) > >>> } > >>> } > >>> +/** > >>> + * \brief Merge the \a source into the ControlList > >>> + * \param[in] source The ControlList to merge into this object > >>> + * \param[in] policy Controls if existing elements in *this shall be > >>> + * overwritten > >>> + * > >>> + * Merging two control lists moves 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 only overwritten if > >>> + * \a policy is MergePolicy::OverwriteExisting. > >>> + * > >>> + * Only control lists created from the same ControlIdMap or ControlInfoMap may > >>> + * be merged. Attempting to do otherwise results in undefined behaviour. > >>> + */ > >>> +void ControlList::merge(ControlList &&source, MergePolicy policy) > >>> +{ > >>> + /** > >>> + * \todo ASSERT that the current and source ControlList are derived > >>> + * from a compatible ControlIdMap, to prevent undefined behaviour due to > >>> + * id collisions. > >>> + * > >>> + * This can not currently be a direct pointer comparison due to the > >>> + * duplication of the ControlIdMaps in the isolated IPA use cases. > >>> + * Furthermore, manually checking each entry of the id map is identical > >>> + * is expensive. > >>> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details > >>> + */ > >>> + > >>> + switch (policy) { > >>> + case MergePolicy::KeepExisting: > >>> + controls_.merge(source.controls_); > >>> + break; > >>> + case MergePolicy::OverwriteExisting: > >>> + source.controls_.merge(controls_); > >>> + controls_.swap(source.controls_); > >>> + break; > >>> + } > >>> +} > >>> + > >> > >> That looks fine. > >> > >> Best regards, > >> Stefan > >> > >>> /** > >>> * \brief Check if the list contains a control with the specified \a id > >>> * \param[in] id The control numerical ID > >>> -- > >>> 2.51.0 > >>> > > >
2025. 10. 06. 20:19 keltezéssel, Stefan Klug írta: > Quoting Barnabás Pőcze (2025-10-06 15:37:02) >> 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta: >>> Hi >>> >>> 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta: >>>> Hi Barnabás, >>>> >>>> Thank you for the patch. >>>> >>>> Quoting Barnabás Pőcze (2025-09-24 14:47:08) >>>>> Add an overload of `ControlList::merge()` that takes the source `ControlList` >>>>> object via an rvalue. In contrast to the other overload, this one does not >>>>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs >>>>> from one map to the other. >>>>> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>>> --- >>>>> include/libcamera/controls.h | 1 + >>>>> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- >>>>> 2 files changed, 40 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >>>>> index 32fb31f78..5d4a53c46 100644 >>>>> --- a/include/libcamera/controls.h >>>>> +++ b/include/libcamera/controls.h >>>>> @@ -432,6 +432,7 @@ public: >>>>> void clear() { controls_.clear(); } >>>>> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); >>>>> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); >>>>> bool contains(unsigned int id) const; >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >>>>> index 1e1b49e6b..54cd3b703 100644 >>>>> --- a/src/libcamera/controls.cpp >>>>> +++ b/src/libcamera/controls.cpp >>>>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, >>>>> * >>>>> * 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-const argument. >>>> >>>> This is interesting. My understanding of that comment was that it might >>>> be useful to have a implementation of merge() that receives a non-const >>>> list that get's filled with the overwritten or skipped controls the same >>>> way std::unordered_map::merge does. Now thinking of that, implementing >>>> such a version would modify the behavior and potentially introduce >>>> misbehavior in code that worked fine before. So maybe we should decide >>>> if we want such a version better sooner than later. >>> >>> Sorry, can you clarify? I thought this new overload satisfies the TODO comment. >>> It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc. >>> But otherwise `source` is usable after a call to this new overload of `merge()` >>> and it will contain the not overwritten / overwritten (depending on `policy`) items. >> >> More specifically: >> >> Let >> *this = A u B >> source = A' u C >> where A and A' are the sets of items whose keys are present in both sets. >> >> If KeepExisting, then source = A', >> if OverwriteExisting, then source = A, >> after the call to merge. > > Sorry for my ignorance (or a lack of C++ knowledge :-)). How would you > use that? > > Lets say: > > ControlList l1, l2; > l1.set(A, ...); > l1.set(B, ...); > l2.set(A', ...) > > // Is this right? > l1.merge(std::move(l2),...); > > Am I allowed to access the data in l2 now? > I think I never used move semantics in that way. A moved-from instance of a type is assumed to be in a "valid but unspecified" state. At minimum the destructor must be able to run successfully. Certain types may provide more guarantees about what operations are valid in the moved-from state. But in any case, there isn't any "moving" happening here. The rvalue reference here acts as an lvalue reference for all intents and purposes. It is only an rvalue reference because using an lvalue reference would make it a lot harder to deduce which overload is actually getting called. So yes, in this case accessing `l2` is perfectly fine, it is not even in a moved-from state. Regards, Barnabás Pőcze > > Best regards, > Stefan > >> >> >>> Although the above behaviour is intentionally not described in the documentation. >>> >>> >>> Regards, >>> Barnabás Pőcze >>> >>> >>>> >>>>> */ >>>>> void ControlList::merge(const ControlList &source, MergePolicy policy) >>>>> { >>>>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) >>>>> } >>>>> } >>>>> +/** >>>>> + * \brief Merge the \a source into the ControlList >>>>> + * \param[in] source The ControlList to merge into this object >>>>> + * \param[in] policy Controls if existing elements in *this shall be >>>>> + * overwritten >>>>> + * >>>>> + * Merging two control lists moves 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 only overwritten if >>>>> + * \a policy is MergePolicy::OverwriteExisting. >>>>> + * >>>>> + * Only control lists created from the same ControlIdMap or ControlInfoMap may >>>>> + * be merged. Attempting to do otherwise results in undefined behaviour. >>>>> + */ >>>>> +void ControlList::merge(ControlList &&source, MergePolicy policy) >>>>> +{ >>>>> + /** >>>>> + * \todo ASSERT that the current and source ControlList are derived >>>>> + * from a compatible ControlIdMap, to prevent undefined behaviour due to >>>>> + * id collisions. >>>>> + * >>>>> + * This can not currently be a direct pointer comparison due to the >>>>> + * duplication of the ControlIdMaps in the isolated IPA use cases. >>>>> + * Furthermore, manually checking each entry of the id map is identical >>>>> + * is expensive. >>>>> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details >>>>> + */ >>>>> + >>>>> + switch (policy) { >>>>> + case MergePolicy::KeepExisting: >>>>> + controls_.merge(source.controls_); >>>>> + break; >>>>> + case MergePolicy::OverwriteExisting: >>>>> + source.controls_.merge(controls_); >>>>> + controls_.swap(source.controls_); >>>>> + break; >>>>> + } >>>>> +} >>>>> + >>>> >>>> That looks fine. >>>> >>>> Best regards, >>>> Stefan >>>> >>>>> /** >>>>> * \brief Check if the list contains a control with the specified \a id >>>>> * \param[in] id The control numerical ID >>>>> -- >>>>> 2.51.0 >>>>> >>> >>
Hi, Quoting Barnabás Pőcze (2025-10-07 10:46:29) > 2025. 10. 06. 20:19 keltezéssel, Stefan Klug írta: > > Quoting Barnabás Pőcze (2025-10-06 15:37:02) > >> 2025. 10. 06. 15:30 keltezéssel, Barnabás Pőcze írta: > >>> Hi > >>> > >>> 2025. 10. 06. 15:02 keltezéssel, Stefan Klug írta: > >>>> Hi Barnabás, > >>>> > >>>> Thank you for the patch. > >>>> > >>>> Quoting Barnabás Pőcze (2025-09-24 14:47:08) > >>>>> Add an overload of `ControlList::merge()` that takes the source `ControlList` > >>>>> object via an rvalue. In contrast to the other overload, this one does not > >>>>> make copies, it uses `std::unordered_map::merge()` to move key-value pairs > >>>>> from one map to the other. > >>>>> > >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > >>>>> --- > >>>>> include/libcamera/controls.h | 1 + > >>>>> src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- > >>>>> 2 files changed, 40 insertions(+), 3 deletions(-) > >>>>> > >>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > >>>>> index 32fb31f78..5d4a53c46 100644 > >>>>> --- a/include/libcamera/controls.h > >>>>> +++ b/include/libcamera/controls.h > >>>>> @@ -432,6 +432,7 @@ public: > >>>>> void clear() { controls_.clear(); } > >>>>> void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); > >>>>> + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); > >>>>> bool contains(unsigned int id) const; > >>>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > >>>>> index 1e1b49e6b..54cd3b703 100644 > >>>>> --- a/src/libcamera/controls.cpp > >>>>> +++ b/src/libcamera/controls.cpp > >>>>> @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, > >>>>> * > >>>>> * 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-const argument. > >>>> > >>>> This is interesting. My understanding of that comment was that it might > >>>> be useful to have a implementation of merge() that receives a non-const > >>>> list that get's filled with the overwritten or skipped controls the same > >>>> way std::unordered_map::merge does. Now thinking of that, implementing > >>>> such a version would modify the behavior and potentially introduce > >>>> misbehavior in code that worked fine before. So maybe we should decide > >>>> if we want such a version better sooner than later. > >>> > >>> Sorry, can you clarify? I thought this new overload satisfies the TODO comment. > >>> It takes an rvalue reference because adding a `ControlList&` overload would wreak havoc. > >>> But otherwise `source` is usable after a call to this new overload of `merge()` > >>> and it will contain the not overwritten / overwritten (depending on `policy`) items. > >> > >> More specifically: > >> > >> Let > >> *this = A u B > >> source = A' u C > >> where A and A' are the sets of items whose keys are present in both sets. > >> > >> If KeepExisting, then source = A', > >> if OverwriteExisting, then source = A, > >> after the call to merge. > > > > Sorry for my ignorance (or a lack of C++ knowledge :-)). How would you > > use that? > > > > Lets say: > > > > ControlList l1, l2; > > l1.set(A, ...); > > l1.set(B, ...); > > l2.set(A', ...) > > > > // Is this right? > > l1.merge(std::move(l2),...); > > > > Am I allowed to access the data in l2 now? > > I think I never used move semantics in that way. > > A moved-from instance of a type is assumed to be in a "valid but unspecified" state. > At minimum the destructor must be able to run successfully. Certain types may provide > more guarantees about what operations are valid in the moved-from state. > > But in any case, there isn't any "moving" happening here. The rvalue reference here > acts as an lvalue reference for all intents and purposes. It is only an rvalue reference > because using an lvalue reference would make it a lot harder to deduce which overload > is actually getting called. > > So yes, in this case accessing `l2` is perfectly fine, it is not even in a moved-from state. That makes all sense now. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > > > Regards, > Barnabás Pőcze > > > > > Best regards, > > Stefan > > > >> > >> > >>> Although the above behaviour is intentionally not described in the documentation. > >>> > >>> > >>> Regards, > >>> Barnabás Pőcze > >>> > >>> > >>>> > >>>>> */ > >>>>> void ControlList::merge(const ControlList &source, MergePolicy policy) > >>>>> { > >>>>> @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) > >>>>> } > >>>>> } > >>>>> +/** > >>>>> + * \brief Merge the \a source into the ControlList > >>>>> + * \param[in] source The ControlList to merge into this object > >>>>> + * \param[in] policy Controls if existing elements in *this shall be > >>>>> + * overwritten > >>>>> + * > >>>>> + * Merging two control lists moves 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 only overwritten if > >>>>> + * \a policy is MergePolicy::OverwriteExisting. > >>>>> + * > >>>>> + * Only control lists created from the same ControlIdMap or ControlInfoMap may > >>>>> + * be merged. Attempting to do otherwise results in undefined behaviour. > >>>>> + */ > >>>>> +void ControlList::merge(ControlList &&source, MergePolicy policy) > >>>>> +{ > >>>>> + /** > >>>>> + * \todo ASSERT that the current and source ControlList are derived > >>>>> + * from a compatible ControlIdMap, to prevent undefined behaviour due to > >>>>> + * id collisions. > >>>>> + * > >>>>> + * This can not currently be a direct pointer comparison due to the > >>>>> + * duplication of the ControlIdMaps in the isolated IPA use cases. > >>>>> + * Furthermore, manually checking each entry of the id map is identical > >>>>> + * is expensive. > >>>>> + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details > >>>>> + */ > >>>>> + > >>>>> + switch (policy) { > >>>>> + case MergePolicy::KeepExisting: > >>>>> + controls_.merge(source.controls_); > >>>>> + break; > >>>>> + case MergePolicy::OverwriteExisting: > >>>>> + source.controls_.merge(controls_); > >>>>> + controls_.swap(source.controls_); > >>>>> + break; > >>>>> + } > >>>>> +} > >>>>> + > >>>> > >>>> That looks fine. > >>>> > >>>> Best regards, > >>>> Stefan > >>>> > >>>>> /** > >>>>> * \brief Check if the list contains a control with the specified \a id > >>>>> * \param[in] id The control numerical ID > >>>>> -- > >>>>> 2.51.0 > >>>>> > >>> > >> >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index 32fb31f78..5d4a53c46 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -432,6 +432,7 @@ public: void clear() { controls_.clear(); } void merge(const ControlList &source, MergePolicy policy = MergePolicy::KeepExisting); + void merge(ControlList &&source, MergePolicy policy = MergePolicy::KeepExisting); bool contains(unsigned int id) const; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 1e1b49e6b..54cd3b703 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -1005,9 +1005,6 @@ ControlList::ControlList(const ControlInfoMap &infoMap, * * 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-const argument. */ void ControlList::merge(const ControlList &source, MergePolicy policy) { @@ -1035,6 +1032,45 @@ void ControlList::merge(const ControlList &source, MergePolicy policy) } } +/** + * \brief Merge the \a source into the ControlList + * \param[in] source The ControlList to merge into this object + * \param[in] policy Controls if existing elements in *this shall be + * overwritten + * + * Merging two control lists moves 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 only overwritten if + * \a policy is MergePolicy::OverwriteExisting. + * + * Only control lists created from the same ControlIdMap or ControlInfoMap may + * be merged. Attempting to do otherwise results in undefined behaviour. + */ +void ControlList::merge(ControlList &&source, MergePolicy policy) +{ + /** + * \todo ASSERT that the current and source ControlList are derived + * from a compatible ControlIdMap, to prevent undefined behaviour due to + * id collisions. + * + * This can not currently be a direct pointer comparison due to the + * duplication of the ControlIdMaps in the isolated IPA use cases. + * Furthermore, manually checking each entry of the id map is identical + * is expensive. + * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details + */ + + switch (policy) { + case MergePolicy::KeepExisting: + controls_.merge(source.controls_); + break; + case MergePolicy::OverwriteExisting: + source.controls_.merge(controls_); + controls_.swap(source.controls_); + break; + } +} + /** * \brief Check if the list contains a control with the specified \a id * \param[in] id The control numerical ID
Add an overload of `ControlList::merge()` that takes the source `ControlList` object via an rvalue. In contrast to the other overload, this one does not make copies, it uses `std::unordered_map::merge()` to move key-value pairs from one map to the other. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/controls.h | 1 + src/libcamera/controls.cpp | 42 +++++++++++++++++++++++++++++++++--- 2 files changed, 40 insertions(+), 3 deletions(-)