[libcamera-devel] [PATCH v2 1/5] libcamera: controls: Create ControlInfoMap with ControlIdMap
Jacopo Mondi
jacopo at jmondi.org
Mon Aug 9 14:43:21 CEST 2021
Hi Laurent,
sorry one more comment I missed in previous reply
On Tue, Aug 03, 2021 at 06:43:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
[snip]
> > @@ -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].
>
Not really in this case, as I use operator[] to insert a new element,
while std::unsorted_map::at() only allows to retrieve the existing
ones.
I could use a reference, but I need a pointer in the next patch, so I
guess we'll have to stick to the (*)[] syntax
> >
> > 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.
>
> > -
> > - 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