| Message ID | 20190918103424.14536-5-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 18/09/2019 11:34, Jacopo Mondi wrote: > ControlList requires a Camera class instance at construction time, > preventing it from being re-constructed from serialized binary data. > > De-couple ControlList from Camera by internally storing a reference to > the Camera's ControlInfoList. Interesting, I guess this allows us to just serialise the ControlInfoList, and not the whole camera. No major concern, and only really some discussion points below. I'm not fond of std::pair type obfuscation; the ".first .second" inline syntax is far too easy to misinterpret (at least for me). But I'll not object to that. I don't think we have any coding style ruling on it. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/controls.h | 4 +-- > src/libcamera/controls.cpp | 54 +++++++++++++++++----------------- > src/libcamera/request.cpp | 4 +-- > test/controls/control_list.cpp | 4 +-- > 4 files changed, 33 insertions(+), 33 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index e46cd8a78679..d3065c0f3f16 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -48,7 +48,7 @@ private: > using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; > > public: > - ControlList(Camera *camera); > + ControlList(const ControlInfoMap &infoMap); > > using iterator = ControlListMap::iterator; > using const_iterator = ControlListMap::const_iterator; > @@ -70,7 +70,7 @@ public: > void update(const ControlList &list); > > private: > - Camera *camera_; > + const ControlInfoMap &infoMap_; > ControlListMap controls_; > }; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index 2d8adde5688e..184d544c05bc 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const > > /** > * \brief Construct a ControlList with a reference to the Camera it applies on > - * \param[in] camera The camera > + * \param[in] infoMap The ControlInfoMap of the camera the control list refers to > */ > -ControlList::ControlList(Camera *camera) > - : camera_(camera) > +ControlList::ControlList(const ControlInfoMap &infoMap) > + : infoMap_(infoMap) > { > } > > @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera) > */ > bool ControlList::contains(ControlId id) const > { > - const ControlInfoMap &controls = camera_->controls(); > - const auto iter = controls.find(id); > - if (iter == controls.end()) { > - LOG(Controls, Error) > - << "Camera " << camera_->name() > - << " does not support control " << id; > + const auto info = infoMap_.find(id); > + if (info == infoMap_.end()) { > + LOG(Controls, Error) << "Control " << id << " not supported"; > > return false; > } > > - return controls_.find(&iter->second) != controls_.end(); > + return controls_.find(&info->second) != controls_.end(); > } > > /** > @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const > */ > DataValue &ControlList::operator[](ControlId id) > { > - const ControlInfoMap &controls = camera_->controls(); > - const auto iter = controls.find(id); > - if (iter == controls.end()) { > - LOG(Controls, Error) > - << "Camera " << camera_->name() > - << " does not support control " << id; > + const auto info = infoMap_.find(id); > + if (info == infoMap_.end()) { > + LOG(Controls, Error) << "Control " << id << " not supported"; > > static DataValue empty; > return empty; > } > > - return controls_[&iter->second]; > + return controls_[&info->second]; > } > > /** > @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id) > */ > void ControlList::update(const ControlList &other) > { > - if (other.camera_ != camera_) { > - LOG(Controls, Error) > - << "Can't update ControlList from a different camera"; > - return; > + /* > + * Make sure if all controls in other's list are supported by this "Make sure that all controls in the other list are " > + * Camera. This is guaranteed to be true if the two lists refer to > + * the same Camera. > + */ > + for (auto &control : other) { > + ControlId id = control.first->id(); > + > + if (infoMap_.find(id) == infoMap_.end()) { > + LOG(Controls, Error) > + << "Failed to update control list with control: " > + << id; > + return; > + } We get a performance penalty here, as now we must search the list, rather than make sure the camera pointer is identical. Would there be any value in storing an opaque handle for the camera (essentially just the pointer, which could be serialised easily) to optimise this out again? I don't (yet) expect this to be a hot-path - so we could look at this later. But if you think it might be worth considering, could you add a todo: to document the possibility? > } > > - for (auto it : other) { > - const ControlInfo *info = it.first; > - const DataValue &value = it.second; > - > - controls_[info] = value; > - } > + for (auto &control : other) > + controls_[control.first] = control.second; Ugh, I really do dislike std::pair obfuscation through .first/.second. I hope one day we can move to C++17, and have the structured bindings, but until then I'm going to close my eyes. Would it be overkill to specify the pair types explicitly? No requirement I don't think though. > } > > } /* namespace libcamera */ > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index ee2158fc7a9c..2b3e1870094e 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request) > * > */ > Request::Request(Camera *camera, uint64_t cookie) > - : camera_(camera), controls_(camera), cookie_(cookie), > - status_(RequestPending), cancelled_(false) > + : camera_(camera), controls_(camera->controls()), > + cookie_(cookie), status_(RequestPending), cancelled_(false) > { > } > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index f1d79ff8fcfd..3c6eb40c091b 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -39,7 +39,7 @@ protected: > > int run() > { > - ControlList list(camera_.get()); > + ControlList list(camera_->controls()); > > /* Test that the list is initially empty. */ > if (!list.empty()) { > @@ -155,7 +155,7 @@ protected: > * the old list. Verify that the new list is empty and that the > * new list contains the expected items and values. > */ > - ControlList newList(camera_.get()); > + ControlList newList(camera_->controls()); > > newList[Brightness] = 128; > newList[Saturation] = 255; > -- > 2.23.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi Kieran, On Fri, Sep 20, 2019 at 12:32:32PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 18/09/2019 11:34, Jacopo Mondi wrote: > > ControlList requires a Camera class instance at construction time, > > preventing it from being re-constructed from serialized binary data. > > > > De-couple ControlList from Camera by internally storing a reference to > > the Camera's ControlInfoList. > > Interesting, I guess this allows us to just serialise the > ControlInfoList, and not the whole camera. > Yes, serializing the whole camera means transporting a ton of data, pointers and complext stuff. We'll serialize the ControlInfoMap and the ControlList, to be re-constructed by the IPA modules separately. > > No major concern, and only really some discussion points below. > > I'm not fond of std::pair type obfuscation; the ".first .second" inline > syntax is far too easy to misinterpret (at least for me). I'm not a fan either, but, well, C++ > > But I'll not object to that. I don't think we have any coding style > ruling on it. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/libcamera/controls.h | 4 +-- > > src/libcamera/controls.cpp | 54 +++++++++++++++++----------------- > > src/libcamera/request.cpp | 4 +-- > > test/controls/control_list.cpp | 4 +-- > > 4 files changed, 33 insertions(+), 33 deletions(-) > > > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > > index e46cd8a78679..d3065c0f3f16 100644 > > --- a/include/libcamera/controls.h > > +++ b/include/libcamera/controls.h > > @@ -48,7 +48,7 @@ private: > > using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; > > > > public: > > - ControlList(Camera *camera); > > + ControlList(const ControlInfoMap &infoMap); > > > > using iterator = ControlListMap::iterator; > > using const_iterator = ControlListMap::const_iterator; > > @@ -70,7 +70,7 @@ public: > > void update(const ControlList &list); > > > > private: > > - Camera *camera_; > > + const ControlInfoMap &infoMap_; > > ControlListMap controls_; > > }; > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > index 2d8adde5688e..184d544c05bc 100644 > > --- a/src/libcamera/controls.cpp > > +++ b/src/libcamera/controls.cpp > > @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const > > > > /** > > * \brief Construct a ControlList with a reference to the Camera it applies on > > - * \param[in] camera The camera > > + * \param[in] infoMap The ControlInfoMap of the camera the control list refers to > > */ > > -ControlList::ControlList(Camera *camera) > > - : camera_(camera) > > +ControlList::ControlList(const ControlInfoMap &infoMap) > > + : infoMap_(infoMap) > > { > > } > > > > @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera) > > */ > > bool ControlList::contains(ControlId id) const > > { > > - const ControlInfoMap &controls = camera_->controls(); > > - const auto iter = controls.find(id); > > - if (iter == controls.end()) { > > - LOG(Controls, Error) > > - << "Camera " << camera_->name() > > - << " does not support control " << id; > > + const auto info = infoMap_.find(id); > > + if (info == infoMap_.end()) { > > + LOG(Controls, Error) << "Control " << id << " not supported"; > > > > return false; > > } > > > > - return controls_.find(&iter->second) != controls_.end(); > > + return controls_.find(&info->second) != controls_.end(); > > } > > > > /** > > @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const > > */ > > DataValue &ControlList::operator[](ControlId id) > > { > > - const ControlInfoMap &controls = camera_->controls(); > > - const auto iter = controls.find(id); > > - if (iter == controls.end()) { > > - LOG(Controls, Error) > > - << "Camera " << camera_->name() > > - << " does not support control " << id; > > + const auto info = infoMap_.find(id); > > + if (info == infoMap_.end()) { > > + LOG(Controls, Error) << "Control " << id << " not supported"; > > > > static DataValue empty; > > return empty; > > } > > > > - return controls_[&iter->second]; > > + return controls_[&info->second]; > > } > > > > /** > > @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id) > > */ > > void ControlList::update(const ControlList &other) > > { > > - if (other.camera_ != camera_) { > > - LOG(Controls, Error) > > - << "Can't update ControlList from a different camera"; > > - return; > > + /* > > + * Make sure if all controls in other's list are supported by this > > "Make sure that all controls in the other list are " > > > + * Camera. This is guaranteed to be true if the two lists refer to > > + * the same Camera. > > + */ > > + for (auto &control : other) { > > + ControlId id = control.first->id(); > > + > > + if (infoMap_.find(id) == infoMap_.end()) { > > + LOG(Controls, Error) > > + << "Failed to update control list with control: " > > + << id; > > + return; > > + } > > We get a performance penalty here, as now we must search the list, > rather than make sure the camera pointer is identical. This will really only happens when merging two control lists... > > Would there be any value in storing an opaque handle for the camera > (essentially just the pointer, which could be serialised easily) to > optimise this out again? As we serialize raw values and info, without any class specific header, adding one value for a field would require specializing the serialization format for this class. Not a huge deal, but I would rather pay this small penalty > > I don't (yet) expect this to be a hot-path - so we could look at this later. > > But if you think it might be worth considering, could you add a todo: to > document the possibility? > > > > > } > > > > - for (auto it : other) { > > - const ControlInfo *info = it.first; > > - const DataValue &value = it.second; > > - > > - controls_[info] = value; > > - } > > + for (auto &control : other) > > + controls_[control.first] = control.second; > > Ugh, I really do dislike std::pair obfuscation through .first/.second. > > I hope one day we can move to C++17, and have the structured bindings, > but until then I'm going to close my eyes. > > Would it be overkill to specify the pair types explicitly? Yes :) I tried, it's quite long, but yes, first/second is obfuscating. > No requirement I don't think though. > > > } > > > > } /* namespace libcamera */ > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index ee2158fc7a9c..2b3e1870094e 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request) > > * > > */ > > Request::Request(Camera *camera, uint64_t cookie) > > - : camera_(camera), controls_(camera), cookie_(cookie), > > - status_(RequestPending), cancelled_(false) > > + : camera_(camera), controls_(camera->controls()), > > + cookie_(cookie), status_(RequestPending), cancelled_(false) > > { > > } > > > > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > > index f1d79ff8fcfd..3c6eb40c091b 100644 > > --- a/test/controls/control_list.cpp > > +++ b/test/controls/control_list.cpp > > @@ -39,7 +39,7 @@ protected: > > > > int run() > > { > > - ControlList list(camera_.get()); > > + ControlList list(camera_->controls()); > > > > /* Test that the list is initially empty. */ > > if (!list.empty()) { > > @@ -155,7 +155,7 @@ protected: > > * the old list. Verify that the new list is empty and that the > > * new list contains the expected items and values. > > */ > > - ControlList newList(camera_.get()); > > + ControlList newList(camera_->controls()); > > > > newList[Brightness] = 128; > > newList[Saturation] = 255; > > -- > > 2.23.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index e46cd8a78679..d3065c0f3f16 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -48,7 +48,7 @@ private: using ControlListMap = std::unordered_map<const ControlInfo *, DataValue>; public: - ControlList(Camera *camera); + ControlList(const ControlInfoMap &infoMap); using iterator = ControlListMap::iterator; using const_iterator = ControlListMap::const_iterator; @@ -70,7 +70,7 @@ public: void update(const ControlList &list); private: - Camera *camera_; + const ControlInfoMap &infoMap_; ControlListMap controls_; }; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index 2d8adde5688e..184d544c05bc 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -175,10 +175,10 @@ std::string ControlInfo::toString() const /** * \brief Construct a ControlList with a reference to the Camera it applies on - * \param[in] camera The camera + * \param[in] infoMap The ControlInfoMap of the camera the control list refers to */ -ControlList::ControlList(Camera *camera) - : camera_(camera) +ControlList::ControlList(const ControlInfoMap &infoMap) + : infoMap_(infoMap) { } @@ -229,17 +229,14 @@ ControlList::ControlList(Camera *camera) */ bool ControlList::contains(ControlId id) const { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(id); - if (iter == controls.end()) { - LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id; + const auto info = infoMap_.find(id); + if (info == infoMap_.end()) { + LOG(Controls, Error) << "Control " << id << " not supported"; return false; } - return controls_.find(&iter->second) != controls_.end(); + return controls_.find(&info->second) != controls_.end(); } /** @@ -283,18 +280,15 @@ bool ControlList::contains(const ControlInfo *info) const */ DataValue &ControlList::operator[](ControlId id) { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(id); - if (iter == controls.end()) { - LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id; + const auto info = infoMap_.find(id); + if (info == infoMap_.end()) { + LOG(Controls, Error) << "Control " << id << " not supported"; static DataValue empty; return empty; } - return controls_[&iter->second]; + return controls_[&info->second]; } /** @@ -322,18 +316,24 @@ DataValue &ControlList::operator[](ControlId id) */ void ControlList::update(const ControlList &other) { - if (other.camera_ != camera_) { - LOG(Controls, Error) - << "Can't update ControlList from a different camera"; - return; + /* + * Make sure if all controls in other's list are supported by this + * Camera. This is guaranteed to be true if the two lists refer to + * the same Camera. + */ + for (auto &control : other) { + ControlId id = control.first->id(); + + if (infoMap_.find(id) == infoMap_.end()) { + LOG(Controls, Error) + << "Failed to update control list with control: " + << id; + return; + } } - for (auto it : other) { - const ControlInfo *info = it.first; - const DataValue &value = it.second; - - controls_[info] = value; - } + for (auto &control : other) + controls_[control.first] = control.second; } } /* namespace libcamera */ diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index ee2158fc7a9c..2b3e1870094e 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -55,8 +55,8 @@ LOG_DEFINE_CATEGORY(Request) * */ Request::Request(Camera *camera, uint64_t cookie) - : camera_(camera), controls_(camera), cookie_(cookie), - status_(RequestPending), cancelled_(false) + : camera_(camera), controls_(camera->controls()), + cookie_(cookie), status_(RequestPending), cancelled_(false) { } diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index f1d79ff8fcfd..3c6eb40c091b 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -39,7 +39,7 @@ protected: int run() { - ControlList list(camera_.get()); + ControlList list(camera_->controls()); /* Test that the list is initially empty. */ if (!list.empty()) { @@ -155,7 +155,7 @@ protected: * the old list. Verify that the new list is empty and that the * new list contains the expected items and values. */ - ControlList newList(camera_.get()); + ControlList newList(camera_->controls()); newList[Brightness] = 128; newList[Saturation] = 255;
ControlList requires a Camera class instance at construction time, preventing it from being re-constructed from serialized binary data. De-couple ControlList from Camera by internally storing a reference to the Camera's ControlInfoList. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/controls.h | 4 +-- src/libcamera/controls.cpp | 54 +++++++++++++++++----------------- src/libcamera/request.cpp | 4 +-- test/controls/control_list.cpp | 4 +-- 4 files changed, 33 insertions(+), 33 deletions(-) -- 2.23.0