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

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