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

[Christian Rauch]

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
those libcamera specific types in the form of standard types for the
public API and not for internal use.

Best,
Christian


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