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

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


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