| Message ID | 20220322205708.734021-1-Rauch.Christian@gmx.de |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 >
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@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 >>
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@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 > >>
Hi Paul, Am 25.03.22 um 10:22 schrieb paul.elder@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@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 >>>>
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@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@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 > >>>>
Hi Christian, On Sat, Mar 26, 2022 at 02:15:17AM +0000, Christian Rauch via libcamera-devel wrote: > Am 25.03.22 um 10:22 schrieb paul.elder@ideasonboard.com: > > 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. I'm a bit reluctant to go this way, for two reasons. First, such an implicit cast will reduce type safety. It will be easy, for instance, to mistakenly use a Point instead of a Size, without the compiler warning you. The second reason is that it encodes in the libcamera API assumptions made by other frameworks. This is particularly true for the Rectangle class. In libcamera we present it as { x, y, w, h }, but the alternative { x1, y1, x2, y2 } representation is also commonly used. If the vector cast operator is used for compatibility with other APIs, I don't think we should pick one representation over the other. For those reasons, I'm tempted to say that conversion to other representations for these data types don't belong to the libcamera public API. > >> Am 24.03.22 um 11:36 schrieb paul.elder@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);
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);