| Message ID | 20240229165753.154936-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, Thank you for the patch. On 29/02/24 10:27 pm, Stefan Klug wrote: > This is useful in many cases although not included in the stl. > > A typical usecase would be: > ContolList controls = getDefaults() > controls.merge(someSpecialValues, true) > ... > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > Hey, > > this is my first email patch - hurray :-) Great ! > Cheers, > Stefan > > include/libcamera/controls.h | 2 +- > src/libcamera/controls.cpp | 9 ++++++--- > 2 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index cf942055..0bf778d8 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -368,7 +368,7 @@ public: > std::size_t size() const { return controls_.size(); } > > void clear() { controls_.clear(); } > - void merge(const ControlList &source); > + void merge(const ControlList &source, bool overwriteExisting = false); > > bool contains(unsigned int id) const; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index b808116c..8ef29a96 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -910,10 +910,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap, > /** > * \brief Merge the \a source into the ControlList > * \param[in] source The ControlList to merge into this object > + * \param[in] overwriteExisting Control if existing elements in *this shall be > + * overwritten > * > * Merging two control lists copies 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 not overwritten. > + * present in *this, then those elements are only overwritten if > + * overwriteExisting is true. s/overwriteExisting/\a overwriteExisting/ > * > * Only control lists created from the same ControlIdMap or ControlInfoMap may > * be merged. Attempting to do otherwise results in undefined behaviour. > @@ -921,7 +924,7 @@ ControlList::ControlList(const ControlInfoMap &infoMap, > * \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) > +void ControlList::merge(const ControlList &source, bool overwriteExisting) > { > /** > * \todo ASSERT that the current and source ControlList are derived > @@ -936,7 +939,7 @@ void ControlList::merge(const ControlList &source) > */ > > for (const auto &ctrl : source) { > - if (contains(ctrl.first)) { > + if (!overwriteExisting && contains(ctrl.first)) { Looks good to me. Stefan, can a small unit test can cover this as well? I see, /* * Create a new list with a new control and merge it with the * existing one, verifying that the existing controls * values don't get overwritten. */ in test/controls/control_list.cpp, so probably worth adding this new behaviour as well, to that test. > const ControlId *id = idmap_->at(ctrl.first); > LOG(Controls, Warning) > << "Control " << id->name() << " not overwritten";
Hi Umang, thanks for your review. I'll post a updated patch soon. Cheers, Stefan Am 04.03.24 um 05:48 schrieb Umang Jain: > Hi Stefan, > > Thank you for the patch. > > On 29/02/24 10:27 pm, Stefan Klug wrote: >> This is useful in many cases although not included in the stl. >> >> A typical usecase would be: >> ContolList controls = getDefaults() >> controls.merge(someSpecialValues, true) >> ... >> >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> --- >> >> Hey, >> >> this is my first email patch - hurray :-) > > Great ! >> Cheers, >> Stefan >> >> include/libcamera/controls.h | 2 +- >> src/libcamera/controls.cpp | 9 ++++++--- >> 2 files changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h >> index cf942055..0bf778d8 100644 >> --- a/include/libcamera/controls.h >> +++ b/include/libcamera/controls.h >> @@ -368,7 +368,7 @@ public: >> std::size_t size() const { return controls_.size(); } >> void clear() { controls_.clear(); } >> - void merge(const ControlList &source); >> + void merge(const ControlList &source, bool overwriteExisting = >> false); >> bool contains(unsigned int id) const; >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index b808116c..8ef29a96 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -910,10 +910,13 @@ ControlList::ControlList(const ControlInfoMap >> &infoMap, >> /** >> * \brief Merge the \a source into the ControlList >> * \param[in] source The ControlList to merge into this object >> + * \param[in] overwriteExisting Control if existing elements in *this >> shall be >> + * overwritten >> * >> * Merging two control lists copies 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 not overwritten. >> + * present in *this, then those elements are only overwritten if >> + * overwriteExisting is true. > > s/overwriteExisting/\a overwriteExisting/ >> * >> * Only control lists created from the same ControlIdMap or >> ControlInfoMap may >> * be merged. Attempting to do otherwise results in undefined >> behaviour. >> @@ -921,7 +924,7 @@ ControlList::ControlList(const ControlInfoMap >> &infoMap, >> * \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) >> +void ControlList::merge(const ControlList &source, bool >> overwriteExisting) >> { >> /** >> * \todo ASSERT that the current and source ControlList are derived >> @@ -936,7 +939,7 @@ void ControlList::merge(const ControlList &source) >> */ >> for (const auto &ctrl : source) { >> - if (contains(ctrl.first)) { >> + if (!overwriteExisting && contains(ctrl.first)) { > > Looks good to me. > > Stefan, can a small unit test can cover this as well? I see, > > /* > * Create a new list with a new control and merge it > with the > * existing one, verifying that the existing controls > * values don't get overwritten. > */ > > in test/controls/control_list.cpp, so probably worth adding this new > behaviour as well, to that test. >> const ControlId *id = idmap_->at(ctrl.first); >> LOG(Controls, Warning) >> << "Control " << id->name() << " not overwritten"; >
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index cf942055..0bf778d8 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -368,7 +368,7 @@ public: std::size_t size() const { return controls_.size(); } void clear() { controls_.clear(); } - void merge(const ControlList &source); + void merge(const ControlList &source, bool overwriteExisting = false); bool contains(unsigned int id) const; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index b808116c..8ef29a96 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -910,10 +910,13 @@ ControlList::ControlList(const ControlInfoMap &infoMap, /** * \brief Merge the \a source into the ControlList * \param[in] source The ControlList to merge into this object + * \param[in] overwriteExisting Control if existing elements in *this shall be + * overwritten * * Merging two control lists copies 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 not overwritten. + * present in *this, then those elements are only overwritten if + * overwriteExisting is true. * * Only control lists created from the same ControlIdMap or ControlInfoMap may * be merged. Attempting to do otherwise results in undefined behaviour. @@ -921,7 +924,7 @@ ControlList::ControlList(const ControlInfoMap &infoMap, * \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) +void ControlList::merge(const ControlList &source, bool overwriteExisting) { /** * \todo ASSERT that the current and source ControlList are derived @@ -936,7 +939,7 @@ void ControlList::merge(const ControlList &source) */ for (const auto &ctrl : source) { - if (contains(ctrl.first)) { + if (!overwriteExisting && contains(ctrl.first)) { const ControlId *id = idmap_->at(ctrl.first); LOG(Controls, Warning) << "Control " << id->name() << " not overwritten";
This is useful in many cases although not included in the stl. A typical usecase would be: ContolList controls = getDefaults() controls.merge(someSpecialValues, true) ... Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Hey, this is my first email patch - hurray :-) Cheers, Stefan include/libcamera/controls.h | 2 +- src/libcamera/controls.cpp | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-)