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

[Kieran Bingham]

Hi Jacopo,

On 08/05/2021 07:32, Jacopo Mondi wrote:
> Hi Kieran,
>    thanks for handling this and sorry for introducing it in first
>    place

No problem,

> On Fri, May 07, 2021 at 07:11:50PM +0300, Laurent Pinchart wrote:
>> 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>
> 
> With either Bug: or Bugzilla: (with a slight preference for the first)
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

But I'm afraid I had already merged it...

--
Kieran

> 
> Thanks
>    j
> 
> 
>>
>>> ---
>>>  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
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran