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

[Jacopo Mondi]

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 at 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 at ideasonboard.com>

Thanks
  j



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