[libcamera-devel] [PATCH] implement vector casts for Point, Size and Rectangle

[Laurent Pinchart]

Hi Christian,

On Sat, Mar 26, 2022 at 02:15:17AM +0000, Christian Rauch via libcamera-devel wrote:
> Am 25.03.22 um 10:22 schrieb paul.elder at ideasonboard.com:
> > On Thu, Mar 24, 2022 at 10:41:50PM +0000, Christian Rauch wrote:
> >> Hello Paul,
> >>
> >> To clarify, with "serialisation" I did not mean to convert those types
> >> into a byte stream, but rather "flatten" those structures, so they can
> >> be represented as a list of values.
> >>
> >> I did not intend to use the IPADataSerializer and I actually was not
> >> aware of this. The intention with the vector<int> cast was to represent
> >
> > Ah, I see. I got confused since that's that only place that I know of
> > where serialization actually takes place.
> >
> >> those libcamera specific types in the form of standard types for the
> >> public API and not for internal use.
> >
> > I'm still not sure I see the use case of wanting to use a vector of ints
> > over dedicated geometry types (which come with nice helpers btw),
> > though. Especially when unsigned ints are cast to signed ints.
> 
> The use-case is that those data types can then be represented by
> primitive data structures (like arrays). I want to use those geometry
> types in another framework as configurable parameter, but this only
> supports plain old data types incl. strings, and arrays thereof.

I'm a bit reluctant to go this way, for two reasons.

First, such an implicit cast will reduce type safety. It will be easy,
for instance, to mistakenly use a Point instead of a Size, without the
compiler warning you.

The second reason is that it encodes in the libcamera API assumptions
made by other frameworks. This is particularly true for the Rectangle
class. In libcamera we present it as { x, y, w, h }, but the alternative
{ x1, y1, x2, y2 } representation is also commonly used. If the vector
cast operator is used for compatibility with other APIs, I don't think
we should pick one representation over the other.

For those reasons, I'm tempted to say that conversion to other
representations for these data types don't belong to the libcamera
public API.

> >> Am 24.03.22 um 11:36 schrieb paul.elder at ideasonboard.com:
> >>> Hello Christian,
> >>>
> >>> Thanks for the patch.
> >>>
> >>> Easy things first: missing Signed-off-by tag.
> >>>
> >>> On Tue, Mar 22, 2022 at 08:57:08PM +0000, Christian Rauch via libcamera-devel wrote:
> >>>> Cast operators from Point, Size and Rectangle to std::vector<int> allows to
> >>>> serialise those types without dedicated conversion functions.
> >>>
> >>> IPADataSerializer converts to and from vectors of uint8_t, so this alone
> >>> isn't sufficient for replacing the specialized serializers, because:
> >>> - it's not plumbed into the IPADataSerializer
> >>> - it only casts to a vector of int
> >>>
> >>>
> >>> Let's look at the first issue first: it's not plumbed into the
> >>> IPADataSerializer.
> >>>
> >>> How would you use this std::vector<int> cast operator? If you really
> >>> want to avoid the specialized IPADataSerializers for these geometry
> >>> structs, then you'd have to use one of the existing IPADataSerializers.
> >>>
> >>> I presume you're imagining using IPADataSerializer<std::vector<V>>.
> >>> In this case, that means that when the pipeline handler and IPA send
> >>> data to each other, they'll be the ones that have to cast the geometry
> >>> structs (ie. *do the serialization*). This is not very nice. Imagine if
> >>> a pipeline handler had to do:
> >>>
> >>> ipa_->configure(static_cast<std::vector<int>>(size));
> >>>
> >>> instead of the simpler:
> >>>
> >>> ipa_->configure(size);
> >>>
> >>> Not to mention the receiver then has to deserialize it.
> >>>
> >>> That defeats the whole purpose of the IPA interface definitions and the
> >>> IPADataSerializer. We might as well go back to the days where pipeline
> >>> handlers and IPAs had to manually serialize and deserialize everything.
> >>>
> >>> Another problem with using IPADataSerializer<std::vector<V>> is that we
> >>> would be serializing/deserializing the vector twice, since it would
> >>> first be Point -> std::vector<int> (via the cast operator), and then
> >>> std::vector<int> -> std::vector<uint8_t> (via
> >>> IPADataSerializer<std::vector<V>>), which breaks every int into a vector
> >>> of uint8_t.
> >>>
> >>> So by avoiding a specialized IPADataSerializer you'd also be increasing
> >>> the serialization cost of the geometry types. The specialized
> >>> IPADataSerializers are auto-generated anyway, not sure why you'd want to
> >>> replace them in this way.
> >>>
> >>>
> >>> And now the second issue: it only casts to a vector of int.
> >>>
> >>> If this patch defined a cast operator to std::vector<uint8_t>, then I
> >>> would say that that doesn't belong here. If Point/Size/Rectangle expose
> >>> a cast operator to std::vector<uint8_t>, that means it's a feature that
> >>> we support for applications to use, which is not something that we want
> >>> (note that this is in the public API). A cast to std::vector<uint8_t>
> >>> for serialization purposes is clearly a job for the serializer.
> >>>
> >>> On the other hand if the cast operator to std::vector<uint8_t> was just
> >>> { x, y }, then it's simply incorrect, as you're obviously losing 24 bits
> >>> of precision.
> >>>
> >>> Also, Size and Rectangle have unsigned ints, but you're casting them to
> >>> ints. If the purpose is for serialization, then as mentioned earlier, it
> >>> doesn't belong here. If the purpose is not for serialization, then...
> >>> what is the purpose?
> >>>
> >>>
> >>> Thanks,
> >>>
> >>> Paul
> >>>
> >>>> ---
> >>>>  include/libcamera/geometry.h | 15 +++++++++++++++
> >>>>  1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> >>>> index 7838b679..7d1e403a 100644
> >>>> --- a/include/libcamera/geometry.h
> >>>> +++ b/include/libcamera/geometry.h
> >>>> @@ -38,6 +38,11 @@ public:
> >>>>  	{
> >>>>  		return { -x, -y };
> >>>>  	}
> >>>> +
> >>>> +	operator std::vector<int>() const
> >>>> +	{
> >>>> +		return { x, y };
> >>>> +	}
> >>>>  };
> >>>>
> >>>>  bool operator==(const Point &lhs, const Point &rhs);
> >>>> @@ -167,6 +172,11 @@ public:
> >>>>
> >>>>  	Size &operator*=(float factor);
> >>>>  	Size &operator/=(float factor);
> >>>> +
> >>>> +	operator std::vector<int>() const
> >>>> +	{
> >>>> +		return { static_cast<int>(width), static_cast<int>(height) };
> >>>> +	}
> >>>>  };
> >>>>
> >>>>  bool operator==(const Size &lhs, const Size &rhs);
> >>>> @@ -283,6 +293,11 @@ public:
> >>>>  	__nodiscard Rectangle scaledBy(const Size &numerator,
> >>>>  				       const Size &denominator) const;
> >>>>  	__nodiscard Rectangle translatedBy(const Point &point) const;
> >>>> +
> >>>> +	operator std::vector<int>() const
> >>>> +	{
> >>>> +		return { x, y, static_cast<int>(width), static_cast<int>(height) };
> >>>> +	}
> >>>>  };
> >>>>
> >>>>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);

-- 
Regards,

Laurent Pinchart