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

[Christian Rauch]

Hi Paul,

Am 25.03.22 um 10:22 schrieb paul.elder at ideasonboard.com:
> Hi Christian,
>
> 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.

Best,
Christian

>
>
> Paul
>
>>
>>
>> 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);
>>>> --
>>>> 2.25.1
>>>>