[v1] libcamera: controls: Fix `ControlInfoMap::count(unsigned int)`

23107 diff mbox series

The two overloads of `find()` and `at()` have the same behaviour
regardless of the argument type: `unsigned int` or `const ControlId *`.
However, `count()` is not so because `count(unsigned int)` only checks
the `ControlIdMap`, and it does not check if the given id is actually
present in the map storing the `ControlInfo` objects.

So `count()` returns 1 for every control id that is present in the
associated `ControlIdMap` regardless of whether there is an actual
entry for the `ControlId` associated with the given numeric id.

Fix that by simply using `find()` to determine the return value.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/controls.cpp | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

Hi Barnabás

On Wed, Apr 02, 2025 at 01:50:10PM +0200, Barnabás Pőcze wrote:
> The two overloads of `find()` and `at()` have the same behaviour
> regardless of the argument type: `unsigned int` or `const ControlId *`.

Looking at

        ControlInfoMap::at(const ControlId *id);
        ControlInfoMap::at(unsigned int key);

the first checks for the presence of *id
in std::unordered_map<const ControlId *, ControlInfo> while the
latter checks the idmap just to retrieve the ControlId *

ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
{
	ASSERT(idmap_);

	return at(idmap_->at(id));
}

The count() implementation says

	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
	 * entries

to justify why it doesn't call count(const ControlId *)

Is your reasoning that there's no guarantee the ControlInfoMap and
idmap are actually in sync, and ids in the idmap might not have a
corresponding ControlId * in the ControlInfoMap ?

Because I have the same understanding


> However, `count()` is not so because `count(unsigned int)` only checks
> the `ControlIdMap`, and it does not check if the given id is actually
> present in the map storing the `ControlInfo` objects.
>
> So `count()` returns 1 for every control id that is present in the
> associated `ControlIdMap` regardless of whether there is an actual
> entry for the `ControlId` associated with the given numeric id.
>
> Fix that by simply using `find()` to determine the return value.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/libcamera/controls.cpp | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index 70f6f6092..98fa7583d 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -857,15 +857,7 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
>   */
>  ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>  {
> -	if (!idmap_)
> -		return 0;
> -
> -	/*
> -	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
> -	 * entries, we can thus just count the matching entries in idmap to
> -	 * avoid an additional lookup.
> -	 */
> -	return idmap_->count(id);
> +	return find(id) != end();

This returns a boolean while the function should actually 'count'. As
this is an unordered map with unique keys, this is probably ok.

Otherwise, alternatively, it could be open coded.

ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
{
	if (!idmap_)
		return 0;

	auto it = idmap_->find(id);
	if (it == idmap_->end())
		return 0;

	return count(it->second);
}

However this does one lookup more.

As we can't have multiple entries with the same key, what you have is
fine. I would however record the reasoning with a tiny comment

        /*
         * The ControlInfoMap underlaying type is an unordered_map
         * which stores unique keys, so we can just use find() here
         * to avoid an additional lookup.
         */

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j



>  }
>
>  /**
> --
> 2.49.0
>

Hi


2025. 04. 02. 15:20 keltezéssel, Jacopo Mondi írta:
> Hi Barnabás
> 
> On Wed, Apr 02, 2025 at 01:50:10PM +0200, Barnabás Pőcze wrote:
>> The two overloads of `find()` and `at()` have the same behaviour
>> regardless of the argument type: `unsigned int` or `const ControlId *`.
> 
> Looking at
> 
>          ControlInfoMap::at(const ControlId *id);
>          ControlInfoMap::at(unsigned int key);
> 
> the first checks for the presence of *id
> in std::unordered_map<const ControlId *, ControlInfo> while the
> latter checks the idmap just to retrieve the ControlId *
> 
> ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id)
> {
> 	ASSERT(idmap_);
> 
> 	return at(idmap_->at(id));
> }
> 
> The count() implementation says
> 
> 	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
> 	 * entries
> 
> to justify why it doesn't call count(const ControlId *)
> 
> Is your reasoning that there's no guarantee the ControlInfoMap and
> idmap are actually in sync, and ids in the idmap might not have a
> corresponding ControlId * in the ControlInfoMap ?
> 
> Because I have the same understanding

I think so, yes. The motivation is `Camera::queueRequest()`:

	/* Pre-process AeEnable. */
	ControlList &controls = request->controls();
	const auto &aeEnable = controls.get(controls::AeEnable);
	if (aeEnable) {
		if (_d()->controlInfo_.count(controls::AnalogueGainMode.id()) &&
		    !controls.contains(controls::AnalogueGainMode.id())) {
			controls.set(controls::AnalogueGainMode,
				     *aeEnable ? controls::AnalogueGainModeAuto
					       : controls::AnalogueGainModeManual);
		}


To me the intention is most likely that `_d()->controlInfo_.count(controls::AnalogueGainMode.id())`
is should determine if the camera supports the control.
However, it does not do that; it checks if the `ControlIdMap` of
the `ControlInfoMap` knows about `AnalogueGainMode`, which will
be true since the `ControlIdMap` is `controls::controls` for all
camera `ControlInfoMap`s.

However, if the code had `_d()->controlInfo_.count(&controls::AnalogueGainMode)`
then it would work "as expected", and indeed check if the camera
supports the control. This patch is to get rid of this inconsistency,
and consequently fix the check in `Camera::queueRequest()`.


Regards,
Barnabás Pőcze

> 
> 
>> However, `count()` is not so because `count(unsigned int)` only checks
>> the `ControlIdMap`, and it does not check if the given id is actually
>> present in the map storing the `ControlInfo` objects.
>>
>> So `count()` returns 1 for every control id that is present in the
>> associated `ControlIdMap` regardless of whether there is an actual
>> entry for the `ControlId` associated with the given numeric id.
>>
>> Fix that by simply using `find()` to determine the return value.
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/libcamera/controls.cpp | 10 +---------
>>   1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
>> index 70f6f6092..98fa7583d 100644
>> --- a/src/libcamera/controls.cpp
>> +++ b/src/libcamera/controls.cpp
>> @@ -857,15 +857,7 @@ const ControlInfoMap::mapped_type &ControlInfoMap::at(unsigned int id) const
>>    */
>>   ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
>>   {
>> -	if (!idmap_)
>> -		return 0;
>> -
>> -	/*
>> -	 * The ControlInfoMap and its idmap have a 1:1 mapping between their
>> -	 * entries, we can thus just count the matching entries in idmap to
>> -	 * avoid an additional lookup.
>> -	 */
>> -	return idmap_->count(id);
>> +	return find(id) != end();
> 
> This returns a boolean while the function should actually 'count'. As
> this is an unordered map with unique keys, this is probably ok.
> 
> Otherwise, alternatively, it could be open coded.
> 
> ControlInfoMap::size_type ControlInfoMap::count(unsigned int id) const
> {
> 	if (!idmap_)
> 		return 0;
> 
> 	auto it = idmap_->find(id);
> 	if (it == idmap_->end())
> 		return 0;
> 
> 	return count(it->second);
> }
> 
> However this does one lookup more.
> 
> As we can't have multiple entries with the same key, what you have is
> fine. I would however record the reasoning with a tiny comment
> 
>          /*
>           * The ControlInfoMap underlaying type is an unordered_map
>           * which stores unique keys, so we can just use find() here
>           * to avoid an additional lookup.
>           */
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Thanks
>    j
> 
> 
> 
>>   }
>>
>>   /**
>> --
>> 2.49.0
>>