[libcamera-devel,5/6] libcamera: controls: Rationalize idMap() function
diff mbox series

Message ID 20210901143800.119118-6-jacopo@jmondi.org
State New
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: control serializer fixes
Related show

Commit Message

Jacopo Mondi Sept. 1, 2021, 2:37 p.m. UTC
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@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(-)

Comments

Kieran Bingham Sept. 2, 2021, 3 p.m. UTC | #1
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@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()

But without you having to change all the uses of controls to controls->
instead of controls. ?

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





>  	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()) {
>
Jacopo Mondi Sept. 3, 2021, 10:24 a.m. UTC | #2
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@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()) {
> >
Kieran Bingham Sept. 3, 2021, 11:14 a.m. UTC | #3
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@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()) {
>>>
Laurent Pinchart Sept. 3, 2021, 12:38 p.m. UTC | #4
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@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()) {
> >>>

Patch
diff mbox series

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();
 	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();
 	for (const auto &control : controls) {
 		const auto &it = idmap.find(control.first);
 		if (it == idmap.end()) {