[libcamera-devel] Issues with ControlSerializer

[Laurent Pinchart]

Hello everybody,

On Mon, Jul 26, 2021 at 12:10:30PM +0200, Jacopo Mondi wrote:
> On Mon, Jul 26, 2021 at 10:07:17AM +0100, Kieran Bingham wrote:
> > On 26/07/2021 09:46, Jacopo Mondi wrote:
> > > On Fri, Jul 23, 2021 at 06:34:55PM +0900, paul.elder at ideasonboard.com wrote:
> > >> 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.

As has been noted already, the name isn't needed to index the
ControlInfoMap. It's mostly used for debugging purpose. There's a
comment in control_serializer.cpp about this:

		/**
		 * \todo Find a way to preserve the control name for debugging
		 * purpose.
		 */
		controlIds_.emplace_back(std::make_unique<ControlId>(entry->id, "", type));

I think we can ignore the name for now.

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

Time to sit in front of the fireplace to listen to the story of the
libcamera controls, says the old man. He's wearing a worn out dark grey
robe, and has a long white beard that gives him the authority of the
wise, even before he speaks.

(Note for later: should we have a D&D-themed code camp ?)

The libcamera controls have evolved over time, and I think we're missing
a bit of the wider context here. ControlInfoMap was designed to be
indexed by ControlId *, not by numerical id. If std::map could have used
a reference instead of a pointer as a key, that's what we would have
used, but the container doesn't make that possible (another note for
later: we should consider overriding the at(), find() and count()
functions to take a const ControlId &).

In the early days, libcamera had separate classes for V4L2 controls.
There was lots of overlap, so both have been merged. Looking up controls
by numerical ID became a requirement, so the ControlInfoMap now
generates an internal ControlIdMap (from unsigned int to const ControlId
*) when constructed. This improves performance when looking up by
numerical ID, but two lookup operations are still needed, one to
translate from numerical ID to ControId *, and one to lookup the
ControlInfo (another note: maybe we could replace the ControlIdMap with
a std::map<unsigned int, ControlInfo *> ?).

All this to say that looking up by ControlId is the recommended
operation. Exposing the find(unsigned int) function to applications is
only a side effect of V4L2 support, and I'd like to drop that. Lookups
by numerical id with

	ipaControls_.find(controls::ExposureTime.id());

is thus something I'd like not to encourage.

When writing the control serializer, serialization of the ControlInfoMap
was only considered for V4L2 controls, and only in the pipeline handler
to IPA direction. The rationale was that ControList instances would
refer to either libcamera::controls::controls or V4L2 controls, and in
the first case, there was no need to serialize and deserialize the
ControlInfoMap as libcamera::controls::controls is available on the IPA
side (not how libcamera::controls::controls is a ControlIdMap, not a
ControlInfoMap, and the serializer and deserializer handle ControlList
with no infoMap in a special way). Transferring a ControlInfoMap from
the IPA to the pipeline handler hasn't been considered. Quite obviously,
this design is now collapsing.

I believe the control API would benefit from a redesign (which may not
be as painful as the previous one, as I think the changes will be
smaller in scope, but all design decisions should be reconsidered in the
light of the current usage of controls). We don't need to do so now
though, so the question is what would be the best path forward, to
minimize the immediate effort but still go in the right direction. I'm
afraid I don't have a ready-made answer to that question, but I can
offer a few thoughts.

Without considering what the long-term redesign of control support would
lead to, in the short term it would be nice if the deserializer could
use the correct ControlId pointer when reconstructing the
ControlInfoMap. We obviously can't serialize the pointer, but if we
conveyed which ControlIdMap a ControlInfoMap is referring to, we could
look it up. For libcamera controls, the ControlIdMap is either
libcamera::controls::controls or libcamera::properties::properties,
there's no use case for using anything else (vendor-specific controls
will force us to reconsider this, but that's for longer term). For V4L2
controls, we need to create ControlId instances dynamically at
deserialization time on the IPA side as the IPA doesn't have access to
any ControlIdMap in that case (it's create dynamically by V4L2Device).
It's fine on the IPA side, as V4L2 controls are looked up by numerical
ID in the ControlList. It's also fine on the pipeline handler side for
the same reason, but there we can do better as we don't actually need to
transfer any V4L2 ControlInfoMap from the IPA to the pipeline handler,
given that all ControlList created by the IPA and sent to the pipeline
handler that refer to V4L2 controls will use a ControlInfoMap previously
sent by the pipeline handler to the IPA, and we have a ControlInfoMap
cache in the control serializer framework (I however recall discussing
issues with the cache, which may need to be fixed).

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

The default value is meant to be communicated to applications. This is
why it's not serialized and deserialized, as we only considered
ControlInfoMap serialization in the pipeline handler to IPA directly.
Now that we have a use case to communicate it, I think it should be
added, and it should hopefully be fairly trivial to do so (except maybe
for string controls, where the min and max report the minimum and
maximum string length, while I think the default is a string, but string
controls aren't really used yet).

-- 
Regards,

Laurent Pinchart