[libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize idMap() function
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Sep 3 13:14:40 CEST 2021
On 03/09/2021 11:24, Jacopo Mondi wrote:
> 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.
Yes, but at least that is defined behaviour, vs undefined with a route
to dereferencing a null-reference.
If the compiler spots that could happen it might simply decide to
optimise out the whole function to a return 0; (or ...something worse
perhaps).
> 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
I think this might just hide bugs, or cause them indeed.
>
> 2) Make
> ControlInfoMap::idmap() const
> {
> ASSERT(idmap_);
I think that might be a good idea anyway...
> 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.
That's ok, The distinction is ...
- Dereferencing a null pointer is defined behaviour.
We know it will segfault.
- Making a reference from a pointer which could potentially be null
(not that /is/ null) could be optimised out by the compiler.
> 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)
Ok, that's very bad.
If we can call
ControlList(nullptr, nullptr);
on that constructor (ControlIdMap &idmap) - Then, I believe the compiler
can do anything it likes. Including not construct (or allocate memory?)
at all.
> {
> }
>
> so it's legal to have it as nullptr, and ASSERT might be quite harsh.
If we can choose to pass in a nullptr here, then it's important that
it's not taken as a reference.
> 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.
Ok - this is a slightly separate thing, so just to make sure I'm clear,
I am opposed to anything where we pass a pointer and store it as a
reference.
But
- I'm ok updating to only use pointers as pointers or
- I'm ok updating the code to only pass references into references ;-)
> 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