| Message ID | 20240304155852.105079-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, Thank you for the patch On 04/03/24 9:28 pm, Stefan Klug wrote: > This is useful in many cases although not included in the stl. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> nit: Subject line, missing (): "libcamera: controls: Add overwriteExisting parameter to ControlList::merge()" Can be applied while merging, no need for v3 in my opinion, LGTM, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > > This version adds a unittest and fixes a doxygen doc. > > include/libcamera/controls.h | 2 +- > src/libcamera/controls.cpp | 9 ++++-- > test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++ > 3 files changed, 57 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..93ce4efe 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 > + * \a 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"; > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index c03f230e..304cabfc 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -196,6 +196,56 @@ protected: > return TestFail; > } > > + /* > + * Create two lists with overlapping controls. Merge them with > + * overwriteExisting = true, verifying that the existing control > + * values *get* overwritten. > + */ > + mergeList.clear(); > + mergeList.set(controls::Brightness, 0.7f); > + mergeList.set(controls::Saturation, 0.4f); > + > + list.clear(); > + list.set(controls::Brightness, 0.5f); > + list.set(controls::Contrast, 1.1f); > + > + mergeList.merge(list, true); > + if (mergeList.size() != 3) { > + cout << "Merged list should contain three elements" << endl; > + return TestFail; > + } > + > + if (list.size() != 2) { > + cout << "The list to merge should contain two elements" > + << endl; > + return TestFail; > + } > + > + if (!mergeList.get(controls::Brightness) || > + !mergeList.get(controls::Contrast) || > + !mergeList.get(controls::Saturation)) { > + cout << "Merged list does not contain all controls" << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Brightness) != 0.5f) { > + cout << "Brightness control value did not change after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Contrast) != 1.1f) { > + cout << "Contrast control value did not change after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Saturation) != 0.4f) { > + cout << "Saturation control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > return TestPass; > } > };
Hi Stefan, Thank you for the patch. On Mon, Mar 04, 2024 at 04:58:53PM +0100, Stefan Klug wrote: > This is useful in many cases although not included in the stl. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > > This version adds a unittest and fixes a doxygen doc. > > include/libcamera/controls.h | 2 +- > src/libcamera/controls.cpp | 9 ++++-- > test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++ > 3 files changed, 57 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); The way to implement a.merge(b, true) with C++ STL would be b.merge(a); b.swap(a); I understand this is not ideal though, and that a nicer API would be, well, nicer. I am however concerned with the bool flag, as it isn't very readable. Consider the following code: a.merge(b); a.merge(b, false); a.merge(b, true); Readers can't easily tell what happens. An alternative would be to use an enum. class ControList { ... enum class MergePolicy { Ignore, Overwrite, }; void merge(const ControlList &source, MergePolicy policy = MergePolicy::Ignore); ... }; (bikeshedding on the names is allowed) With that, reading a.merge(b); a.merge(b, ControlList::MergePolicy::Ignore); a.merge(b, ControlList::MergePolicy::Overwrite); becomes clearer. You still can't tell what the default is (that's the trouble with default parameters), but the second and third line are much more explicit. > > bool contains(unsigned int id) const; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index b808116c..93ce4efe 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 > + * \a 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"; > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index c03f230e..304cabfc 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -196,6 +196,56 @@ protected: > return TestFail; > } > > + /* > + * Create two lists with overlapping controls. Merge them with > + * overwriteExisting = true, verifying that the existing control > + * values *get* overwritten. > + */ > + mergeList.clear(); > + mergeList.set(controls::Brightness, 0.7f); > + mergeList.set(controls::Saturation, 0.4f); > + > + list.clear(); > + list.set(controls::Brightness, 0.5f); > + list.set(controls::Contrast, 1.1f); > + > + mergeList.merge(list, true); > + if (mergeList.size() != 3) { > + cout << "Merged list should contain three elements" << endl; > + return TestFail; > + } > + > + if (list.size() != 2) { > + cout << "The list to merge should contain two elements" > + << endl; > + return TestFail; > + } > + > + if (!mergeList.get(controls::Brightness) || > + !mergeList.get(controls::Contrast) || > + !mergeList.get(controls::Saturation)) { > + cout << "Merged list does not contain all controls" << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Brightness) != 0.5f) { > + cout << "Brightness control value did not change after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Contrast) != 1.1f) { > + cout << "Contrast control value did not change after merging lists" > + << endl; > + return TestFail; > + } > + > + if (mergeList.get(controls::Saturation) != 0.4f) { > + cout << "Saturation control value changed after merging lists" > + << endl; > + return TestFail; > + } > + > return TestPass; > } > };
Hi Laurent, thanks for your comment. Am 05.03.24 um 09:38 schrieb Laurent Pinchart: > Hi Stefan, > > Thank you for the patch. > > On Mon, Mar 04, 2024 at 04:58:53PM +0100, Stefan Klug wrote: >> This is useful in many cases although not included in the stl. >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> --- >> >> This version adds a unittest and fixes a doxygen doc. >> >> include/libcamera/controls.h | 2 +- >> src/libcamera/controls.cpp | 9 ++++-- >> test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 57 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); > > The way to implement a.merge(b, true) with C++ STL would be > > b.merge(a); > b.swap(a); > > I understand this is not ideal though, and that a nicer API would be, > well, nicer. I am however concerned with the bool flag, as it isn't very > readable. Consider the following code: > > a.merge(b); > a.merge(b, false); > a.merge(b, true); > > Readers can't easily tell what happens. An alternative would be to use > an enum.> > class ControList > { > ... > > enum class MergePolicy { > Ignore, > Overwrite, > }; > > void merge(const ControlList &source, > MergePolicy policy = MergePolicy::Ignore); > > ... > }; > > (bikeshedding on the names is allowed) > > With that, reading > > a.merge(b); > a.merge(b, ControlList::MergePolicy::Ignore); > a.merge(b, ControlList::MergePolicy::Overwrite); Yes, I agree that the boolean isn't readable. Another option would be to just create an additional function. I fail to come up with a nice name (maybe mergeWithOverwrite() ). That would have the added benefit to not break the ABI but would add code duplication. The more I think about it, the enum feels better. If we go for the enum, what about calling the first entry "Keep" or even KeepDuplicates and OverwriteDuplicates. I'm fine with either solution. Please give me q quick heads-up which way you'd prefer. Cheers Stefan > > becomes clearer. You still can't tell what the default is (that's the > trouble with default parameters), but the second and third line are much > more explicit. > >> >> bool contains(unsigned int id) const; >> >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index b808116c..93ce4efe 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 >> + * \a 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"; >> diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp >> index c03f230e..304cabfc 100644 >> --- a/test/controls/control_list.cpp >> +++ b/test/controls/control_list.cpp >> @@ -196,6 +196,56 @@ protected: >> return TestFail; >> } >> >> + /* >> + * Create two lists with overlapping controls. Merge them with >> + * overwriteExisting = true, verifying that the existing control >> + * values *get* overwritten. >> + */ >> + mergeList.clear(); >> + mergeList.set(controls::Brightness, 0.7f); >> + mergeList.set(controls::Saturation, 0.4f); >> + >> + list.clear(); >> + list.set(controls::Brightness, 0.5f); >> + list.set(controls::Contrast, 1.1f); >> + >> + mergeList.merge(list, true); >> + if (mergeList.size() != 3) { >> + cout << "Merged list should contain three elements" << endl; >> + return TestFail; >> + } >> + >> + if (list.size() != 2) { >> + cout << "The list to merge should contain two elements" >> + << endl; >> + return TestFail; >> + } >> + >> + if (!mergeList.get(controls::Brightness) || >> + !mergeList.get(controls::Contrast) || >> + !mergeList.get(controls::Saturation)) { >> + cout << "Merged list does not contain all controls" << endl; >> + return TestFail; >> + } >> + >> + if (mergeList.get(controls::Brightness) != 0.5f) { >> + cout << "Brightness control value did not change after merging lists" >> + << endl; >> + return TestFail; >> + } >> + >> + if (mergeList.get(controls::Contrast) != 1.1f) { >> + cout << "Contrast control value did not change after merging lists" >> + << endl; >> + return TestFail; >> + } >> + >> + if (mergeList.get(controls::Saturation) != 0.4f) { >> + cout << "Saturation control value changed after merging lists" >> + << endl; >> + return TestFail; >> + } >> + >> return TestPass; >> } >> }; >
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..93ce4efe 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 + * \a 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"; diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index c03f230e..304cabfc 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -196,6 +196,56 @@ protected: return TestFail; } + /* + * Create two lists with overlapping controls. Merge them with + * overwriteExisting = true, verifying that the existing control + * values *get* overwritten. + */ + mergeList.clear(); + mergeList.set(controls::Brightness, 0.7f); + mergeList.set(controls::Saturation, 0.4f); + + list.clear(); + list.set(controls::Brightness, 0.5f); + list.set(controls::Contrast, 1.1f); + + mergeList.merge(list, true); + if (mergeList.size() != 3) { + cout << "Merged list should contain three elements" << endl; + return TestFail; + } + + if (list.size() != 2) { + cout << "The list to merge should contain two elements" + << endl; + return TestFail; + } + + if (!mergeList.get(controls::Brightness) || + !mergeList.get(controls::Contrast) || + !mergeList.get(controls::Saturation)) { + cout << "Merged list does not contain all controls" << endl; + return TestFail; + } + + if (mergeList.get(controls::Brightness) != 0.5f) { + cout << "Brightness control value did not change after merging lists" + << endl; + return TestFail; + } + + if (mergeList.get(controls::Contrast) != 1.1f) { + cout << "Contrast control value did not change after merging lists" + << endl; + return TestFail; + } + + if (mergeList.get(controls::Saturation) != 0.4f) { + cout << "Saturation control value changed after merging lists" + << endl; + return TestFail; + } + return TestPass; } };
This is useful in many cases although not included in the stl. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- This version adds a unittest and fixes a doxygen doc. include/libcamera/controls.h | 2 +- src/libcamera/controls.cpp | 9 ++++-- test/controls/control_list.cpp | 50 ++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 4 deletions(-)