[libcamera-devel] Issues with ControlSerializer

[Kieran Bingham]

Hi Paul/Jacopo,

On 26/07/2021 09:46, Jacopo Mondi wrote:
> Hi Paul,
>    thanks for the analysis of the issue
> 
> On Fri, Jul 23, 2021 at 06:34:55PM +0900, paul.elder at ideasonboard.com wrote:
>> Hi me,
>>
>> On Fri, Jul 23, 2021 at 05:44:42PM +0900, paul.elder at ideasonboard.com wrote:
>>> Hello,
>>>
>>> While Jacopo was trying to send ControlInfoMaps over IPC and actually
>>> try to run predicates on it, it came to our attention that
>>> ControlInfoMaps are serialized by the ControlSerializer in such a way
>>> that some assumptions are broken.
>>>
>>> When the ControlSerializer serializes ControlInfoMaps, it discards the
>>> ControlId's name, and the ControlInfo's default value. Everything else
>>> (ControlId's id and type, and ControlInfo's min and max) are preserved.
>>>
>>>
>>> Let's start with the discarding of the ControlId's name. The consequence
>>> of this is that we are unable to use ControlInfoMap::find() with a
>>> deserialized ControlId and expect it to work.
>>>
>>> Let's look at an example. We pass a ControlInfoMap output argument to an
>>> IPA call, from the pipeline handler, over IPC:
>>>
>>> 	ControlInfoMap ipaControls = {};
>>> 	ipa_->init(&ipaControls);
>>>
>>> The IPA adds a control and returns it:
>>>
>>> 	ipaControls[&controls::ExposureTime] = ControlInfo(41, 33306, 0);
>>>
>>> Back in the pipeline handler, we try to find the control:
>>>
>>> 	const auto exposureInfo = ipaControls_.find(&controls::ExposureTime);
>>>
>>> Here, exposureInfo is a ControlId with id = EXPOSURE_TIME, name = "",
>>> and type = int32_t. Meanwhile controls::ExposureTime is a
>>> Control<int32_t> with id = EXPOSURE_TIME, name = "ExposureTime", and
>>> type = int32_t. Clearly these two are not equal, as the name does not
>>> match, so if we try to find the control from the ControlInfoMap that was
>>
>> Almost, not quite. It's actually a mismatch of pointers. so
>> &controls::ExposureTime is not equal to the pointer of the ControlId in
>> ipaControls that has the same id as ExposureTime. Clearly this is
>> because the deserializer creates new ControlIds (I'm pretty sure).
>>
>> So actually if we do ipaControls_.find(controls::ExposureTime.id());
>> then it works fine.
>>
>> Is this an acceptable solution? Or should we override
>> ControlInfoMap::find(ControlId *) to use ->id()?
> 
> Please note that we already have
> 
> 	iterator find(unsigned int key);
> 	const_iterator find(unsigned int key) const;
> 
> in the ControlInfoMap class.
> 
> Hence it should be enough to remove
>         -        ControlInfoMap::find(const ControlId *);

Is this provided by the

        using Map::find;

Implementation? I don't see it explicitly defined somewhere.

> 
> Or rather redefine it to retrieve the numeric id internally, something
> like:
> 
>         ControlInfoMap::iterator find(const ControlId *controlId)
>         {
>                 return find(controlId->id());
>         }
> 
> I would maintain the current interface for consistency and add this
> tiny wrapper.


I think I recall one reason we were ok comparing pointers was that they
were unique across multiple ControlLists/ControlInfoMaps which might
otherwise have the same IDs ?

(i.e. if there are libcamera controls, and v4l2 controls with both lists
starting with ids based at 0, we don't want to allow a comparator to say
V4L2::ID(0) == libcamera::ID(0)...

Will this be an issue? or can we already ensure that we don't make
comparisons against incompatible lists?


> Thanks
>    j
> 
>>
>>
>> Paul
>>
>>> returned by the IPA, it will fail:
>>>
>>> 	if (exposureInfo == ipaControls_.end()) {
>>> 		LOG(IPU3, Error) << "Exposure control not initializaed by the IPA";
>>> 		return -EINVAL;
>>> 	}
>>>
>>> Since the ControlId name is tied to the ControlId's id, I don't think
>>> it's actually necessary to serialize the name as well. I think a
>>> solution would be to provide a comparator for ControlIds, which ignores
>>> the name and only considers the id and type. Is this a fine solution?
>>>
>>>
>>> Next let's look at the discarding of the ControlInfo's default value.
>>> The consequence of this is that we cannot rely on using the default
>>> value to report any capabilities from the IPA, and we can only use
>>> min/max. This may or may not be an issue, so I just want to prime a
>>> discussion about this. It seems that the default value of a ControlInfo
>>> is not important at all, as it is ignored by the serializer, and it is
>>> also ignored by the comparator. So it seems to me that at the moment
>>> nothing can expect the default value to hold any valuable information,>>> only suggestions. Do we want to change this, or leave it as-is?

Are default values only used to return a default/current value from the
kernel / device?

I suspect there are still cases where we want to know the default value,
but perhaps the IPA doesn't need to know it.

For instance, if a user/app makes a change - or set of changes, it's
common to want to be able to reset back to default values.

Is there anything like that, which the IPA should report? Perhaps
exposure values that it might know the 'right' defaults to use?
I suspect most of the time it would come from the pipeline handler though...




>>> Thanks,
>>>
>>> Paul