[libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create ControlInfoMap with ControlIdMap
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 9 12:48:22 CEST 2021
Hi Laurent,
On Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jul 28, 2021 at 06:11:12PM +0200, Jacopo Mondi wrote:
> > ControlInfoMap do not have a ControlId map associated, but rather
>
> s/do not/does not/
>
> > create one with the generateIdMap() function at creation time.
>
> s/create/creates/
>
> > As a consequence, when in the need to de-serialize a ControlInfoMap all
> > the ControlId it contains are created by the deserializer instance, not
> > being able to discern if the controls the ControlIdMap refers to are the
> > global libcamera controls (and proeprties) or instances local to the
> > V4L2 device that has first initialized the controls.
> >
> > As a consequence the ControlId stored in a de-serialized map will always
> > be newly created entities, preventing lookup by ControlId reference on a
> > de-serialized ControlInfoMap.
> >
> > In order to make it possible to use globally available ControlId
> > instances whenever possible, create ControlInfoMap with a reference to
> > an externally allocated ControlIdMap instead of generating one
> > internally.
> >
> > Has a consequence the class constructors take and additional argument,
>
> s/Has/As/
>
> > which might be not pleaseant to type in, but enforces the concepts that
> > ControlInfoMap should be created with controls part of the same id map.
>
> It's not that unpleasant, it seems worth it to me.
>
> > As the ControlIdMap the ControlInfoMap refers to need to be allocated
>
> s/need/needs/
>
> > externally:
> > - Use the globally available controls::controls (or
> > properties::properties) id map when referring to libcamera controls
>
> It's interesting to see that we actually don't use
> properties::properties yet.
>
> > - The V4L2 device that creates ControlInfoMap by parsing the device's
> > controls has to allocate a ControlIdMap and ties its lifetime
> > management to the its own one
>
> "to the its own one" hurts a bit :-) You could possibly just drop "and
> ties ...".
>
> > - The ControlSerializer that de-serializes a ControlInfoMap has to
> > create and store the ControlIdMap the de-serialized info map refers to
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/controls.h | 13 ++-
> > .../libcamera/internal/control_serializer.h | 1 +
> > include/libcamera/internal/v4l2_device.h | 1 +
> > include/libcamera/ipa/raspberrypi.h | 40 ++++-----
> > src/libcamera/control_serializer.cpp | 11 ++-
> > src/libcamera/controls.cpp | 82 ++++---------------
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +-
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 3 +-
> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc/vimc.cpp | 2 +-
> > src/libcamera/v4l2_device.cpp | 3 +-
> > .../ipa_data_serializer_test.cpp | 4 +-
> > 12 files changed, 59 insertions(+), 106 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 1bc958a43b22..4a2cdb335870 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -306,12 +306,11 @@ public:
> >
> > ControlInfoMap() = default;
> > ControlInfoMap(const ControlInfoMap &other) = default;
> > - ControlInfoMap(std::initializer_list<Map::value_type> init);
> > - ControlInfoMap(Map &&info);
> > + ControlInfoMap(std::initializer_list<Map::value_type> init,
> > + const ControlIdMap &idmap);
> > + ControlInfoMap(Map &&info, const ControlIdMap &idmap);
> >
> > ControlInfoMap &operator=(const ControlInfoMap &other) = default;
> > - ControlInfoMap &operator=(std::initializer_list<Map::value_type> init);
> > - ControlInfoMap &operator=(Map &&info);
> >
> > using Map::key_type;
> > using Map::mapped_type;
> > @@ -336,12 +335,10 @@ public:
> > iterator find(unsigned int key);
> > const_iterator find(unsigned int key) const;
> >
> > - const ControlIdMap &idmap() const { return idmap_; }
> > + const ControlIdMap &idmap() const { return *idmap_; }
> >
> > private:
> > - void generateIdmap();
> > -
> > - ControlIdMap idmap_;
> > + const ControlIdMap *idmap_;
>
> You can also store a reference instead of a pointer.
Not really.. I mean, I could but then I will have to remove copy
assignements and default constructor, breaking the construction of
CameraData which has a default constructed ControlInfoMap member.
>
> > };
> >
> > class ControlList
> > diff --git a/include/libcamera/internal/control_serializer.h b/include/libcamera/internal/control_serializer.h
> > index 7d4426c95d12..8a66be324138 100644
> > --- a/include/libcamera/internal/control_serializer.h
> > +++ b/include/libcamera/internal/control_serializer.h
> > @@ -48,6 +48,7 @@ private:
> >
> > unsigned int serial_;
> > std::vector<std::unique_ptr<ControlId>> controlIds_;
> > + std::vector<std::unique_ptr<ControlIdMap>> controlIdMaps_;
> > std::map<unsigned int, ControlInfoMap> infoMaps_;
> > std::map<const ControlInfoMap *, unsigned int> infoMapHandles_;
> > };
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index 77b835b3cb80..423c8fb11845 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -69,6 +69,7 @@ private:
> >
> > std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_;
> > std::vector<std::unique_ptr<ControlId>> controlIds_;
> > + ControlIdMap controlIdMap_;
> > ControlInfoMap controls_;
> > std::string deviceNode_;
> > int fd_;
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index a8790000e2d9..521eaecd19b2 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -27,26 +27,26 @@ namespace RPi {
> > * and the pipeline handler may be reverted so that it aborts when an
> > * unsupported control is encountered.
> > */
> > -static const ControlInfoMap Controls = {
> > - { &controls::AeEnable, ControlInfo(false, true) },
> > - { &controls::ExposureTime, ControlInfo(0, 999999) },
> > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > - { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > - { &controls::AwbEnable, ControlInfo(false, true) },
> > - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > - { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > - { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > - { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > -};
> > +static const ControlInfoMap Controls({
> > + { &controls::AeEnable, ControlInfo(false, true) },
> > + { &controls::ExposureTime, ControlInfo(0, 999999) },
> > + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> > + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> > + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> > + { &controls::ExposureValue, ControlInfo(0.0f, 16.0f) },
> > + { &controls::AwbEnable, ControlInfo(false, true) },
> > + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> > + { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
> > + { &controls::Contrast, ControlInfo(0.0f, 32.0f) },
> > + { &controls::Saturation, ControlInfo(0.0f, 32.0f) },
> > + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> > + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> > + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> > + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > + }, controls::controls);
> >
> > } /* namespace RPi */
> >
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index 300466285a57..df6ed93f477e 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -93,6 +93,7 @@ void ControlSerializer::reset()
> > infoMapHandles_.clear();
> > infoMaps_.clear();
> > controlIds_.clear();
> > + controlIdMaps_.clear();
> > }
> >
> > size_t ControlSerializer::binarySize(const ControlValue &value)
> > @@ -376,6 +377,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > }
> >
> > ControlInfoMap::Map ctrls;
> > + controlIdMaps_.emplace_back(std::make_unique<ControlIdMap>());
> > + ControlIdMap *localIdMap = controlIdMaps_.back().get();
>
> With
>
> ControlIdMap &localIdMap = *controlIdMaps_.back().get();
>
> the code below may look better. Or maybe you could use
> localIdMap->at(entry->id) instead of (*localIdMap)[entry->id].
>
> >
> > for (unsigned int i = 0; i < hdr->entries; ++i) {
> > const struct ipa_control_info_entry *entry =
> > @@ -392,6 +395,8 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > * purpose.
> > */
> > controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));
> > + ControlId *controlId = controlIds_.back().get();
> > + (*localIdMap)[entry->id] = controlId;
> >
> > if (entry->offset != values.offset()) {
> > LOG(Serializer, Error)
> > @@ -401,15 +406,15 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
> > }
> >
> > /* Create and store the ControlInfo. */
> > - ctrls.emplace(controlIds_.back().get(),
> > - loadControlInfo(type, values));
> > + ctrls.emplace(controlId, loadControlInfo(type, values));
> > }
> >
> > /*
> > * Create the ControlInfoMap in the cache, and store the map to handle
> > * association.
> > */
> > - ControlInfoMap &map = infoMaps_[hdr->handle] = std::move(ctrls);
> > + infoMaps_[hdr->handle] = ControlInfoMap(std::move(ctrls), *localIdMap);
> > + ControlInfoMap &map = infoMaps_[hdr->handle];
> > infoMapHandles_[&map] = hdr->handle;
> >
> > return map;
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 78109f4130a8..9409f0f9e782 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -616,24 +616,25 @@ std::string ControlInfo::toString() const
>
> The second paragraph of the ControlInfoMap class documentation needs
> to be updated, to explain how the class now needs to be constructed with
> an idmap (and how all elements must refer to the same idmap).
>
> > /**
> > * \brief Construct a ControlInfoMap from an initializer list
> > * \param[in] init The initializer list
> > + * \param[in] idmap The idmap used by the ControlInfoMap
> > */
> > -ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init)
> > - : Map(init)
> > +ControlInfoMap::ControlInfoMap(std::initializer_list<Map::value_type> init,
> > + const ControlIdMap &idmap)
> > + : Map(init), idmap_(&idmap)
> > {
> > - generateIdmap();
> > }
> >
> > /**
> > * \brief Construct a ControlInfoMap from a plain map
> > * \param[in] info The control info plain map
> > + * \param[in] idmap The idmap used by the ControlInfoMap
> > *
> > * Construct a new ControlInfoMap and populate its contents with those of
> > * \a info using move semantics. Upon return the \a info map will be empty.
> > */
> > -ControlInfoMap::ControlInfoMap(Map &&info)
> > - : Map(std::move(info))
> > +ControlInfoMap::ControlInfoMap(Map &&info, const ControlIdMap &idmap)
> > + : Map(std::move(info)), idmap_(&idmap)
> > {
> > - generateIdmap();
> > }
> >
> > /**
> > @@ -643,34 +644,6 @@ ControlInfoMap::ControlInfoMap(Map &&info)
> > * \return A reference to the ControlInfoMap
> > */
> >
> > -/**
> > - * \brief Replace the contents with those from the initializer list
> > - * \param[in] init The initializer list
> > - * \return A reference to the ControlInfoMap
> > - */
> > -ControlInfoMap &ControlInfoMap::operator=(std::initializer_list<Map::value_type> init)
> > -{
> > - Map::operator=(init);
> > - generateIdmap();
> > - return *this;
> > -}
> > -
> > -/**
> > - * \brief Move assignment operator from a plain map
> > - * \param[in] info The control info plain map
> > - *
> > - * Populate the map by replacing its contents with those of \a info using move
> > - * semantics. Upon return the \a info map will be empty.
> > - *
> > - * \return A reference to the populated ControlInfoMap
> > - */
> > -ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> > -{
> > - Map::operator=(std::move(info));
> > - generateIdmap();
> > - return *this;
> > -}
> > -
> > /**
> > * \brief Access specified element by numerical ID
> > * \param[in] id The numerical ID
> > @@ -678,7 +651,7 @@ ControlInfoMap &ControlInfoMap::operator=(Map &&info)
> > */
> > ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> > {
> > - return at(idmap_.at(id));
> > + return at(idmap_->at(id));
> > }
> >
> > /**
> > @@ -688,7 +661,7 @@ ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> > */
> > const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
> > {
> > - return at(idmap_.at(id));
> > + return at(idmap_->at(id));
> > }
> >
> > /**
> > @@ -703,7 +676,7 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> > * entries, we can thus just count the matching entries in idmap to
> > * avoid an additional lookup.
> > */
> > - return idmap_.count(id);
> > + return idmap_->count(id);
> > }
> >
> > /**
> > @@ -714,8 +687,8 @@ ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> > */
> > ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> > {
> > - auto iter = idmap_.find(id);
> > - if (iter == idmap_.end())
> > + auto iter = idmap_->find(id);
> > + if (iter == idmap_->end())
> > return end();
> >
> > return find(iter->second);
> > @@ -729,8 +702,8 @@ ControlInfoMap::iterator ControlInfoMap::find(unsigned int id)
> > */
> > ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> > {
> > - auto iter = idmap_.find(id);
> > - if (iter == idmap_.end())
> > + auto iter = idmap_->find(id);
> > + if (iter == idmap_->end())
> > return end();
> >
> > return find(iter->second);
> > @@ -747,33 +720,6 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> > * \return The ControlId map
> > */
> >
> > -void ControlInfoMap::generateIdmap()
> > -{
> > - idmap_.clear();
> > -
> > - for (const auto &ctrl : *this) {
> > - /*
> > - * For string controls, min and max define the valid
> > - * range for the string size, not for the individual
> > - * values.
> > - */
> > - ControlType rangeType = ctrl.first->type() == ControlTypeString
> > - ? ControlTypeInteger32 : ctrl.first->type();
> > - const ControlInfo &info = ctrl.second;
> > -
> > - if (info.min().type() != rangeType) {
> > - LOG(Controls, Error)
> > - << "Control " << utils::hex(ctrl.first->id())
> > - << " type and info type mismatch";
> > - idmap_.clear();
> > - clear();
> > - return;
> > - }
>
> I think those sanity checks should be kept, You can rename the function
> to validate() (or something similar) and call it from the constructors.
> A useful additional check would be to verify that each entry in the
> control info map is in the idmap.
I will ASSERT() that.
Thanks
j
>
> > -
> > - idmap_[ctrl.first->id()] = ctrl.first;
> > - }
> > -}
> > -
> > /**
> > * \class ControlList
> > * \brief Associate a list of ControlId with their values for an object
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 76c3bb3d8aa9..048993365b44 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -1057,7 +1057,8 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)
> >
> > controls[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);
> >
> > - data->controlInfo_ = std::move(controls);
> > + data->controlInfo_ = ControlInfoMap(std::move(controls),
> > + controls::controls);
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 42911a8fdfdb..710b9309448e 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -935,7 +935,8 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
> > std::forward_as_tuple(&controls::AeEnable),
> > std::forward_as_tuple(false, true));
> >
> > - data->controlInfo_ = std::move(ctrls);
> > + data->controlInfo_ = ControlInfoMap(std::move(ctrls),
> > + controls::controls);
> >
> > data->sensor_ = std::make_unique<CameraSensor>(sensor);
> > ret = data->sensor_->init();
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 0f634b8da609..573d8042c18c 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -535,7 +535,7 @@ int UVCCameraData::init(MediaDevice *media)
> > addControl(cid, info, &ctrls);
> > }
> >
> > - controlInfo_ = std::move(ctrls);
> > + controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
> >
> > return 0;
> > }
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 12f7517fd0ae..d4b041d33eb4 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -512,7 +512,7 @@ int VimcCameraData::init()
> > ctrls.emplace(id, info);
> > }
> >
> > - controlInfo_ = std::move(ctrls);
> > + controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
> >
> > /* Initialize the camera properties. */
> > properties_ = sensor_->properties();
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 98d93a12a7be..6ccd8eb86016 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -611,12 +611,13 @@ void V4L2Device::listControls()
> > << " (" << utils::hex(ctrl.id) << ")";
> >
> > controlIds_.emplace_back(v4l2ControlId(ctrl));
> > + controlIdMap_[ctrl.id] = controlIds_.back().get();
> > controlInfo_.emplace(ctrl.id, ctrl);
> >
> > ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));
> > }
> >
> > - controls_ = std::move(ctrls);
> > + controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);
> > }
> >
> > /**
> > diff --git a/test/serialization/ipa_data_serializer_test.cpp b/test/serialization/ipa_data_serializer_test.cpp
> > index 880bcd02c6be..3d845a129a5f 100644
> > --- a/test/serialization/ipa_data_serializer_test.cpp
> > +++ b/test/serialization/ipa_data_serializer_test.cpp
> > @@ -32,13 +32,13 @@
> > using namespace std;
> > using namespace libcamera;
> >
> > -static const ControlInfoMap Controls = {
> > +static const ControlInfoMap Controls = ControlInfoMap({
> > { &controls::AeEnable, ControlInfo(false, true) },
> > { &controls::ExposureTime, ControlInfo(0, 999999) },
> > { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> > { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> > { &controls::Brightness, ControlInfo(-1.0f, 1.0f) },
>
> Should this be reindented, like in include/libcamera/ipa/raspberrypi.h ?
>
> Overall this looks good to me.
>
> > -};
> > + }, controls::controls);
> >
> > namespace libcamera {
> >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list