[libcamera-devel] Issues with ControlSerializer

[Jacopo Mondi]

Hi Kieran

On Mon, Jul 26, 2021 at 10:07:17AM +0100, Kieran Bingham wrote:
> 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.

Yes, it's std::unordered_map::find

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

Correct, good memory. We indeed open the door to id collision between
v4l2 controls and libcamera controls. Currently we cannot distinguish
between a v4l2 defined control and a libcamera defined one by design.
Unless we add a flag isV4L2_ or something, but I'm not sure it's
ideal.

>
> > 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