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

[Jacopo Mondi]

Hi Chris

On Sat, Mar 26, 2022 at 02:15:17AM +0000, Christian Rauch via libcamera-devel wrote:
> 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.

To be honest I'm not opposed to this, even if it provides a tempting
shortcut for applications to bypass usage of proper types.

A few comments on the patch below

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

The first question I have is if we want to allow implicit conversion

        Point p;
        vector<int> a = p;

I would prefer not and require users to go through explicit
conversion.

> >>>> +
> >>>> +	operator std::vector<int>() const
> >>>> +	{
> >>>> +		return { x, y };
> >>>> +	}

There is also one question about the operator semantic: the returned
vector will contain copies of x and y, hence:


        Point p(5, 6);
        std::vector<int> v = tmp;
        v[0] = 7;
        cout << p.x();

        -> 5

>From a conversion operator I would expect to be able to get a
reference to the object content, instead of copies. Maybe a dedicated
operator is better suited ?


> >>>>  };
> >>>>
> >>>>  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) };
> >>>> +	}

Why cast to int ?

> >>>>  };
> >>>>
> >>>>  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) };
> >>>> +	}

Same question about cast.

Also, you're missing documentation. Please have a look to geometry.cpp
and provide documentations in complete form. As an example

        /**
         * \fn Size::alignDownTo(unsigned int hAlignment, unsigned int vAlignment)
         * \brief Align the size down horizontally and vertically in place
         * \param[in] hAlignment Horizontal alignment
         * \param[in] vAlignment Vertical alignment
         *
         * This functions rounds the width and height down to the nearest multiple of
         * \a hAlignment and \a vAlignment respectively.
         *
         * \return A reference to this object
         */

We can help refine the text if necessary

Thanks
  j

> >>>>  };
> >>>>
> >>>>  bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> >>>> --
> >>>> 2.25.1
> >>>>