[libcamera-devel] [PATCH 4/5] libcamera: controls: De-couple Controls from Camera
Jacopo Mondi
jacopo at jmondi.org
Mon Sep 23 13:12:40 CEST 2019
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 at ideasonboard.com>
>
>
> > Signed-off-by: Jacopo Mondi <jacopo at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190923/9ab39c14/attachment.sig>
More information about the libcamera-devel
mailing list