[libcamera-devel] [PATCH 5/6] libcamera: controls: Rationalize idMap() function

[Laurent Pinchart]

Hello,

On Fri, Sep 03, 2021 at 12:14:40PM +0100, Kieran Bingham wrote:
> On 03/09/2021 11:24, Jacopo Mondi wrote:
> > 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.

Dereferencing a null pointer is also undefined behaviour. To be
pedantic, dereferencing a null pointer is allowed, but using the
resulting value is undefined behaviour:

	char *p = nullptr;
	sizeof(*p);		// valid
	*p;			// valid
	*p = 0;			// invalid, undefined behaviour

> 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).

If we can guarantee that the return value of idMap() will never be null
here, I think it's fine. What would definitely not be fine is


	ControlIdMap &controls = *subdev_->controls().idMap();

	... code that doesn't use controls ...

	if (&controls == nullptr)
		...

The compiler could drop the check, or do something even worse.

Creating a reference from a pointer essentially binds it to the object
obtained by dereferencing the pointer, which is considered as using the
value, and is thus undefined behaviour if the pointer is null. In the
example above, it's the first line than is considered undefined
behaviour if the pointer can be null.

> > 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.

If we can guarantee that nobody call ControlInfoMap::idmap() on such an
instance that has a null idmap_, then there's no undefined behaviour. It
doesn't meant it's a good API though.

Maybe we could modify the classes that embed an instance of
ControlInfoMap to allocate it dynamically instead ?

> > 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.

Agreed, I don't like this much.

> > 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.

Nope, see above :-)

>    We know it will segfault.

Most likely, but if the compiler can determine the pointer is null, it
can do something else. In practice the range of "something else" is
limited, even if it's allowed by the standard, I don't think any
compiler will compile undefined behaviour to 'rm -rf /' (except perhaps
in Intercal).

>  - Making a reference from a pointer which could potentially be null
>    (not that /is/ null) could be optimised out by the compiler.

It's not that it's optimized out, it's an undefined behaviour.

> > 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/

Doesn't look that bad, but we may need to dynamically allocated the
ControlInfoMap stored in IPAIPU3 and IPARkISP1 to drop the default
constructor for that class.

> > 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)

It's better than no drawbacks I think, it restricts the controls that
can be stored in the control list to what the camera supports, so it
seems to be an improvement. There's a small voice in my head that warns
me of a possible overdesign here though, but so far it looks good.

> > - 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

The ControlInfoMap for the RaspberryPi IPA is statically defined in a
header file, which is actually not good, it should be fixed to create it
dynamically at runtime instead.

I'm starting to wonder if the need to dynamically change min/max/def
values in the ControlInfoMap at configure time will not require much
more work than I initially envisioned, especially if we decide to allow
configure() to remove/add supported controls. I think we should minimize
the cleanups to ControlInfoMap in this series, to avoid doing work that
may get thrown away (due to a full refactoring) later.

> >   - 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!
> > 
> >>>  	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()) {
> >>>

-- 
Regards,

Laurent Pinchart