[libcamera-devel] Issues with ControlSerializer

[Jacopo Mondi]

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 *);

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.

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