[libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize idMap() function
Jacopo Mondi
jacopo at jmondi.org
Fri Sep 3 12:24:29 CEST 2021
Hi Kieran,
On Thu, Sep 02, 2021 at 04:00:13PM +0100, Kieran Bingham wrote:
> On 01/09/2021 15:37, Jacopo Mondi wrote:
> > The ControlList class has a pair of accessor functions with a similar
> > signature:
> >
> > const ControlInfoMap *infoMap() const { return infoMap_; }
> > const ControlIdMap *idMap() const { return idmap_; }
> >
> > As ControlList::infoMap_ can be nullptr, the two functions returns the
> > class members as pointers and not references.
> >
> > The ControlInfoMap class has instead:
> >
> > const ControlIdMap &idmap() const { return *idmap_; }
> >
> > Which is disturbingly different in the signature and return type.
> >
> > Rationalize the accessor function names by stabilizing on:
> >
> > const ControlIdMap *idMap() const { return idmap_; }
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > include/libcamera/controls.h | 2 +-
> > src/libcamera/camera_sensor.cpp | 2 +-
> > src/libcamera/control_serializer.cpp | 4 ++--
> > src/libcamera/controls.cpp | 4 ++--
> > src/libcamera/delayed_controls.cpp | 4 ++--
> > 5 files changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index 8e5bc8b70b94..de6faf600e19 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -338,7 +338,7 @@ 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:
> > bool validate();
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 876685097f76..48af3a617ee1 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -176,7 +176,7 @@ int CameraSensor::validateSensorDriver()
> > V4L2_CID_CAMERA_SENSOR_ROTATION,
> > };
> >
> > - const ControlIdMap &controls = subdev_->controls().idmap();
> > + const ControlIdMap &controls = *subdev_->controls().idMap();
>
> Aye? Is this legal?
>
> Is that declaring a reference against a dereferenced pointer?
>
> I can't tell if that's making a copy - or ... time for a compile-explorer:
>
> https://godbolt.org/z/j75669Mrf
>
> Ok - so it's not making a copy, and it generates identical code to:
>
> const ControlIdMap *controls = subdev_->controls().idMap()
Thanks for checking, this was my expectation
>
> But without you having to change all the uses of controls to controls->
> instead of controls. ?
Yes, which I wouldn't mind to be honest
>
> But given there are only 3 ... ?
>
>
> https://isocpp.org/wiki/faq/references#refs-vs-ptrs
> states
> > "Use references when you can, and pointers when you have to."
>
> But https://isocpp.org/wiki/faq/references#refs-not-null
> makes it clear:
>
> > It means this is illegal:
> >
> > T* p = nullptr;
> > T& r = *p; // illegal
>
> So we have to be /really/ sure that idMap() would not return a nullptr,
> or we get into undefined behaviour, and the cpu turns into a black hole.
>
> I suspect a reference use here is dangerous ...
Also using an unchecked pointer is, as we would immediately dereference
it causing a segfault.
Unfortunately we need to construct empty ControlInfoMap with idmap_ ==
nullptr, as there are instances of that class as class members of
others, so a default constructor is required.
We could:
1) Default construct ControlInfoMap with idmap_ pointing to a known
valid idmap like controls::controls. We would avoid segfaults or
undefined behaviour but accessing a default constructed
ControlInfoMap's idmap would return a reference to a possibly ids
2) Make
ControlInfoMap::idmap() const
{
ASSERT(idmap_);
return idmap_;
}
Note that the same problem exists with ControlList as we have the
ControlList::ControlList()
: validator_(nullptr), idmap_(nullptr), infoMap_(nullptr)
{
}
constructor and un-guarded accessors
const ControlInfoMap *infoMap() const { return infoMap_; }
const ControlIdMap *idMap() const { return idmap_; }
so I guess deferencing a nullptr there is theoretically possible.
Also note that it is possible to construct a ControlList with a
nullptr infoMap
ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)
: validator_(validator), idmap_(&idmap), infoMap_(nullptr)
{
}
so it's legal to have it as nullptr, and ASSERT might be quite harsh.
Actually some parts of our code (serializers in particular) have to
constantly deal with the if (!list->infoMap()) case.
I think the fragile part comes from the fact a ControlList might be
only optionally associated with a ControlInfoMap and that's not been made
a stricter requirement.
I think we should rather try to impose that, as that would allow to
implement controls auto-validation against their limits and if any
piece of code cretes a ControlList, it should have access to the
controls limits.
This is what it might look like (on top of this series) making it a
requirement to contruct a ControlList with a ControlInfoMap and
removing the possibility to contruct it with a ControlIdMap:
https://paste.debian.net/1210212/
In brief:
- Request now creates controls_ and metadata_ with the
controls::controls id map
- Use camera_->controls() as the request has access to it.
I see no drawbacks, and the validator_ does exactly this to validate
controls (we can make a ControlList auto-validate if we impose to
be constructed with a ControlInfoMap)
- IPAs creates control lists with controls::controls id map
IPA should be in charge of initializaing/updating some of the camera
controls. They have access to the ControlInfoMap of controls they
handle, and they can use it to construct ControlList. It might be
challanging to keep the map in sync between the PH and the IPA
- IPU3 is ok-ish: it initializes ControlsInfoMap of camera controls
in init(), it does not yet update limits if configure() but has
all it needs to do so
- RaspberryPi is as usual top-class: it already receives the camera controls
in configure(). We might want to move the initialization of some
controls to the IPA in future, but that shouldn't make a
difference
- RkISP1 needs a lot of love. It does not handle camera controls at
all atm, but it's trivial to provide them at configure() time as
RPi does.
- Properties:
../src/libcamera/camera_sensor.cpp:58:11: error: no matching function for call to ‘libcamera::ControlList::ControlList(const ControlIdMap&)’
58 | properties_(properties::properties)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Properties are expressed as ControlList but there is a non-enforced
rule that they are not mutable, or better it is only enforced
towards application by returning properties as const & from the
Camera class).
For properties it is fair to contruct them with an id map as
they are read only so creating a ControlInfoMap to associate it to
a non-modifiable control list doesn't make much sense.
I wonder if properties should not be made a read-only sub-class of
ControlList and only allow them to be initialized by ControlIdMap.
A possible challange would be that serializing them would require
more effort, but for now, there doesn't seem a need to serialize
properties lists between IPA and PH.
All in all, I think it's worth a try, but I might have missed some
reasons why I shouldn't go there!
Thanks
j
>
>
>
>
>
> > for (uint32_t ctrl : optionalControls) {
> > if (!controls.count(ctrl))
> > LOG(CameraSensor, Debug)
> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> > index d4377c024079..7eef041a16ee 100644
> > --- a/src/libcamera/control_serializer.cpp
> > +++ b/src/libcamera/control_serializer.cpp
> > @@ -190,7 +190,7 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
> > for (const auto &ctrl : infoMap)
> > valuesSize += binarySize(ctrl.second);
> >
> > - const ControlIdMap *idmap = &infoMap.idmap();
> > + const ControlIdMap *idmap = infoMap.idMap();
> > enum ipa_controls_id_map_type idMapType;
> > if (idmap == &controls::controls)
> > idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
> > @@ -530,7 +530,7 @@ ControlList ControlSerializer::deserialize<ControlList>(ByteStreamBuffer &buffer
> > }
> >
> > const ControlInfoMap *infoMap = iter->first;
> > - idMap = &infoMap->idmap();
> > + idMap = infoMap->idMap();
> > } else {
> > switch (hdr->id_map_type) {
> > case IPA_CONTROL_ID_MAP_CONTROLS:
> > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> > index 96625edb1380..9b9bad212db3 100644
> > --- a/src/libcamera/controls.cpp
> > +++ b/src/libcamera/controls.cpp
> > @@ -778,7 +778,7 @@ ControlInfoMap::const_iterator ControlInfoMap::find(unsigned int id) const
> > }
> >
> > /**
> > - * \fn const ControlIdMap &ControlInfoMap::idmap() const
> > + * \fn const ControlIdMap *ControlInfoMap::idMap() const
> > * \brief Retrieve the ControlId map
> > *
> > * Constructing ControlList instances for V4L2 controls requires a ControlIdMap
> > @@ -832,7 +832,7 @@ ControlList::ControlList(const ControlIdMap &idmap, ControlValidator *validator)
> > * \param[in] validator The validator (may be null)
> > */
> > ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *validator)
> > - : validator_(validator), idmap_(&infoMap.idmap()), infoMap_(&infoMap)
> > + : validator_(validator), idmap_(infoMap.idMap()), infoMap_(&infoMap)
> > {
> > }
> >
> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp
> > index 90ce7e0b5b52..763021ef09bb 100644
> > --- a/src/libcamera/delayed_controls.cpp
> > +++ b/src/libcamera/delayed_controls.cpp
> > @@ -130,7 +130,7 @@ void DelayedControls::reset()
> > /* Seed the control queue with the controls reported by the device. */
> > values_.clear();
> > for (const auto &ctrl : controls) {
> > - const ControlId *id = device_->controls().idmap().at(ctrl.first);
> > + const ControlId *id = device_->controls().idMap()->at(ctrl.first);
> > /*
> > * Do not mark this control value as updated, it does not need
> > * to be written to to device on startup.
> > @@ -158,7 +158,7 @@ bool DelayedControls::push(const ControlList &controls)
> > }
> >
> > /* Update with new controls. */
> > - const ControlIdMap &idmap = device_->controls().idmap();
> > + const ControlIdMap &idmap = *device_->controls().idMap();
>
> Another location of taking a reference from a pointer.
> I think this is likely to flag up the UBSAN, even if it compiles cleanly.
>
>
> > for (const auto &control : controls) {
> > const auto &it = idmap.find(control.first);
> > if (it == idmap.end()) {
> >
More information about the libcamera-devel
mailing list