[libcamera-devel] Issues with ControlSerializer

[paul.elder at ideasonboard.com]

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()?


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?
> 
> 
> Thanks,
> 
> Paul