[libcamera-devel] [PATCH] libcamera: controls: Remove merge assertion

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Fri, May 07, 2021 at 01:44:44PM +0100, Kieran Bingham wrote:
> The ControlList merge operation is protected with an ASSERT to guarantee
> that the two lists are compatible.
> 
> Unfortunately this assertion fails when we run IPAs in an isolated case
> as while the lists are compatible, the isolated IPA has a unique
> instance of the id map. This breaks the pointer comparison, and the
> assertion fails with a false positive.

Paul, is this caused by the deserializer using a deserialized idmap
instead of a cached pointer to an idmap previously serialized ?

> Remove the assertion, leaving only a todo in it's place as this breaks
> active users of the library.
> 
> Bugzilla: https://bugs.libcamera.org/show_bug.cgi?id=31

How about "Bug:" to not make it depend on any particular tool ?

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> ---
>  src/libcamera/controls.cpp | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp
> index b763148d4391..5aef4e7145bd 100644
> --- a/src/libcamera/controls.cpp
> +++ b/src/libcamera/controls.cpp
> @@ -890,7 +890,17 @@ ControlList::ControlList(const ControlInfoMap &infoMap, ControlValidator *valida
>   */
>  void ControlList::merge(const ControlList &source)
>  {
> -	ASSERT(idmap_ == source.idmap_);
> +	/**
> +	 * \todo: ASSERT that the current and source ControlList are derived
> +	 * from a compatible ControlIdMap, to prevent undefined behaviour due to
> +	 * id collisions.
> +	 *
> +	 * This can not currently be a direct pointer comparison due to the
> +	 * duplication of the ControlIdMaps in the isolated IPA use cases.
> +	 * Furthermore, manually checking each entry of the id map is identical
> +	 * is expensive.
> +	 * See https://bugs.libcamera.org/show_bug.cgi?id=31 for further details
> +	 */
>  
>  	for (const auto &ctrl : source) {
>  		if (contains(ctrl.first)) {

-- 
Regards,

Laurent Pinchart