| Message ID | 20260407-kbingham-awb-split-v1-9-a39af3f4dc20@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Extend the Vector class with a constructor to support casting to the > storage type when the given parameters are compatible. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/internal/vector.h | 8 ++++++++ > src/libcamera/vector.cpp | 7 +++++++ > 2 files changed, 15 insertions(+) > > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h > index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 > --- a/include/libcamera/internal/vector.h > +++ b/include/libcamera/internal/vector.h > @@ -51,6 +51,14 @@ public: > std::copy(data.begin(), data.end(), data_.begin()); > } > > + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && > + !std::is_same_v<T, U>> * = nullptr> > + constexpr Vector(const Vector<U, Rows> &other) > + { > + for (unsigned int i = 0; i < Rows; i++) > + data_[i] = static_cast<T>(other[i]); > + } > + > const T &operator[](size_t i) const > { > ASSERT(i < data_.size()); > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp > index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 > --- a/src/libcamera/vector.cpp > +++ b/src/libcamera/vector.cpp > @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) > * The size of \a data must be equal to the dimension size Rows of the vector. > */ > > +/** > + * \fn Vector::Vector(const Vector<U, Rows> &other) > + * \brief Construct a vector by converting another vector's element type > + * \tparam U The source vector element type > + * \param[in] other The vector to convert from > + */ This smells, it might be error prone (I don't have good experience with C++ implicit numeric conversions). How about changing the type of gains to double everywhere, this diff against the whole series compiles for me: > + > /** > * \fn T Vector::operator[](size_t i) const > * \brief Index to an element in the vector
Quoting Milan Zamazal (2026-04-08 15:36:42) > Hi Kieran, > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Extend the Vector class with a constructor to support casting to the > > storage type when the given parameters are compatible. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/internal/vector.h | 8 ++++++++ > > src/libcamera/vector.cpp | 7 +++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h > > index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 > > --- a/include/libcamera/internal/vector.h > > +++ b/include/libcamera/internal/vector.h > > @@ -51,6 +51,14 @@ public: > > std::copy(data.begin(), data.end(), data_.begin()); > > } > > > > + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && > > + !std::is_same_v<T, U>> * = nullptr> > > + constexpr Vector(const Vector<U, Rows> &other) > > + { > > + for (unsigned int i = 0; i < Rows; i++) > > + data_[i] = static_cast<T>(other[i]); > > + } > > + > > const T &operator[](size_t i) const > > { > > ASSERT(i < data_.size()); > > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp > > index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 > > --- a/src/libcamera/vector.cpp > > +++ b/src/libcamera/vector.cpp > > @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) > > * The size of \a data must be equal to the dimension size Rows of the vector. > > */ > > > > +/** > > + * \fn Vector::Vector(const Vector<U, Rows> &other) > > + * \brief Construct a vector by converting another vector's element type > > + * \tparam U The source vector element type > > + * \param[in] other The vector to convert from > > + */ > > This smells, it might be error prone (I don't have good experience with :-) Maybe indeed, that's why I wanted to get this series out already to ask for opinions. > C++ implicit numeric conversions). How about changing the type of gains > to double everywhere, this diff against the whole series compiles for > me: Well - all our controls are floats. So should we instead change to floats everywhere? What really is the right precision! But yeah - this patch is here to try to unblock what I thought were lots of horrible explicit casts on double vectors to float vectors... -- Kieran
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-04-08 15:36:42) >> Hi Kieran, >> > >> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >> >> > Extend the Vector class with a constructor to support casting to the >> > storage type when the given parameters are compatible. >> > >> > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> > --- >> > include/libcamera/internal/vector.h | 8 ++++++++ >> > src/libcamera/vector.cpp | 7 +++++++ >> > 2 files changed, 15 insertions(+) >> > >> > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h >> > index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 >> > --- a/include/libcamera/internal/vector.h >> > +++ b/include/libcamera/internal/vector.h >> > @@ -51,6 +51,14 @@ public: >> > std::copy(data.begin(), data.end(), data_.begin()); >> > } >> > >> > + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && >> > + !std::is_same_v<T, U>> * = nullptr> >> > + constexpr Vector(const Vector<U, Rows> &other) >> > + { >> > + for (unsigned int i = 0; i < Rows; i++) >> > + data_[i] = static_cast<T>(other[i]); >> > + } >> > + >> > const T &operator[](size_t i) const >> > { >> > ASSERT(i < data_.size()); >> > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp >> > index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 >> > --- a/src/libcamera/vector.cpp >> > +++ b/src/libcamera/vector.cpp >> > @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) >> > * The size of \a data must be equal to the dimension size Rows of the vector. >> > */ >> > >> > +/** >> > + * \fn Vector::Vector(const Vector<U, Rows> &other) >> > + * \brief Construct a vector by converting another vector's element type >> > + * \tparam U The source vector element type >> > + * \param[in] other The vector to convert from >> > + */ >> >> This smells, it might be error prone (I don't have good experience with > > :-) Maybe indeed, that's why I wanted to get this series out already to > ask for opinions. > >> C++ implicit numeric conversions). How about changing the type of gains >> to double everywhere, this diff against the whole series compiles for >> me: > > Well - all our controls are floats. So should we instead change to > floats everywhere? What really is the right precision! This is a question I've been asking myself all the time :-). For the software ISP part, I don't think it matters -- float is enough and double doesn't harm. In the end, GPU ISP uses its own precision and CPU ISP uses precomputed tables. No idea about the other pipelines. But it doesn't make much sense to use different types for AWB in libipa and in the pipelines. I don't know whichever one is "right" but I think it should be the same in all the places. > But yeah - this patch is here to try to unblock what I thought were lots > of horrible explicit casts on double vectors to float vectors... > > -- > Kieran
On Wed, Apr 08, 2026 at 06:33:23PM +0100, Kieran Bingham wrote: > Quoting Milan Zamazal (2026-04-08 15:36:42) > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > > > Extend the Vector class with a constructor to support casting to the > > > storage type when the given parameters are compatible. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > > include/libcamera/internal/vector.h | 8 ++++++++ > > > src/libcamera/vector.cpp | 7 +++++++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h > > > index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 > > > --- a/include/libcamera/internal/vector.h > > > +++ b/include/libcamera/internal/vector.h > > > @@ -51,6 +51,14 @@ public: > > > std::copy(data.begin(), data.end(), data_.begin()); > > > } > > > > > > + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && > > > + !std::is_same_v<T, U>> * = nullptr> > > > + constexpr Vector(const Vector<U, Rows> &other) > > > + { > > > + for (unsigned int i = 0; i < Rows; i++) > > > + data_[i] = static_cast<T>(other[i]); > > > + } > > > + > > > const T &operator[](size_t i) const > > > { > > > ASSERT(i < data_.size()); > > > diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp > > > index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 > > > --- a/src/libcamera/vector.cpp > > > +++ b/src/libcamera/vector.cpp > > > @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) > > > * The size of \a data must be equal to the dimension size Rows of the vector. > > > */ > > > > > > +/** > > > + * \fn Vector::Vector(const Vector<U, Rows> &other) > > > + * \brief Construct a vector by converting another vector's element type > > > + * \tparam U The source vector element type > > > + * \param[in] other The vector to convert from > > > + */ > > > > This smells, it might be error prone (I don't have good experience with > > :-) Maybe indeed, that's why I wanted to get this series out already to > ask for opinions. > > > C++ implicit numeric conversions). How about changing the type of gains > > to double everywhere, this diff against the whole series compiles for > > me: > > Well - all our controls are floats. So should we instead change to > floats everywhere? What really is the right precision! If there's an actual need to mix float and double, then we shouldn't enable implicit conversion as every conversion would need to be carefully considered. If there's no need to mix them, then let's pick one. I'd like Barnabás' opinion here too. > But yeah - this patch is here to try to unblock what I thought were lots > of horrible explicit casts on double vectors to float vectors...
2026. 04. 08. 23:21 keltezéssel, Laurent Pinchart írta: > On Wed, Apr 08, 2026 at 06:33:23PM +0100, Kieran Bingham wrote: >> Quoting Milan Zamazal (2026-04-08 15:36:42) >>> Kieran Bingham <kieran.bingham@ideasonboard.com> writes: >>> >>>> Extend the Vector class with a constructor to support casting to the >>>> storage type when the given parameters are compatible. >>>> >>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>> --- >>>> include/libcamera/internal/vector.h | 8 ++++++++ >>>> src/libcamera/vector.cpp | 7 +++++++ >>>> 2 files changed, 15 insertions(+) >>>> >>>> diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h >>>> index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 >>>> --- a/include/libcamera/internal/vector.h >>>> +++ b/include/libcamera/internal/vector.h >>>> @@ -51,6 +51,14 @@ public: >>>> std::copy(data.begin(), data.end(), data_.begin()); >>>> } >>>> >>>> + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && >>>> + !std::is_same_v<T, U>> * = nullptr> I think `std::is_constructible_v<...>` would be sufficient here. >>>> + constexpr Vector(const Vector<U, Rows> &other) >>>> + { >>>> + for (unsigned int i = 0; i < Rows; i++) >>>> + data_[i] = static_cast<T>(other[i]); >>>> + } >>>> + >>>> const T &operator[](size_t i) const >>>> { >>>> ASSERT(i < data_.size()); >>>> diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp >>>> index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 >>>> --- a/src/libcamera/vector.cpp >>>> +++ b/src/libcamera/vector.cpp >>>> @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) >>>> * The size of \a data must be equal to the dimension size Rows of the vector. >>>> */ >>>> >>>> +/** >>>> + * \fn Vector::Vector(const Vector<U, Rows> &other) >>>> + * \brief Construct a vector by converting another vector's element type >>>> + * \tparam U The source vector element type >>>> + * \param[in] other The vector to convert from >>>> + */ >>> >>> This smells, it might be error prone (I don't have good experience with >> >> :-) Maybe indeed, that's why I wanted to get this series out already to >> ask for opinions. >> >>> C++ implicit numeric conversions). How about changing the type of gains >>> to double everywhere, this diff against the whole series compiles for >>> me: >> >> Well - all our controls are floats. So should we instead change to >> floats everywhere? What really is the right precision! > > If there's an actual need to mix float and double, then we shouldn't > enable implicit conversion as every conversion would need to be > carefully considered. If there's no need to mix them, then let's pick > one. > > I'd like Barnabás' opinion here too. I think it's unfortunately too easy to mix floats and doubles in C and C++. So in my opinion it is probably better to be wary of that and either use the same type where possible, or pay careful attention to the where and how the conversions happen. There is e.g. `-Wfloat-conversion`, but maybe that would be too much. Regarding precision, I feel like `float` is most likely sufficient for everything in libcamera, at least in the [-1,1] range, but probably even in [-8192,8192]. Nonetheless, the ability to convert one vector to another vector of a different type like this could be useful, regarding that I think the main question is whether to make it implicit (like here), or explicit via an explicit constructor or even possibly a separate function. > >> But yeah - this patch is here to try to unblock what I thought were lots >> of horrible explicit casts on double vectors to float vectors... >
diff --git a/include/libcamera/internal/vector.h b/include/libcamera/internal/vector.h index 16b6aef0b38f4b094d83449b5cdb1ba48b17c2bf..d29547f170e37c15f85e46abca82f89c8995115d 100644 --- a/include/libcamera/internal/vector.h +++ b/include/libcamera/internal/vector.h @@ -51,6 +51,14 @@ public: std::copy(data.begin(), data.end(), data_.begin()); } + template<typename U, std::enable_if_t<std::is_arithmetic_v<U> && + !std::is_same_v<T, U>> * = nullptr> + constexpr Vector(const Vector<U, Rows> &other) + { + for (unsigned int i = 0; i < Rows; i++) + data_[i] = static_cast<T>(other[i]); + } + const T &operator[](size_t i) const { ASSERT(i < data_.size()); diff --git a/src/libcamera/vector.cpp b/src/libcamera/vector.cpp index 4dad1b9001c5df97f71031724729563cae0962c3..397e370fd023caef069095dc5abebe2d6d76848a 100644 --- a/src/libcamera/vector.cpp +++ b/src/libcamera/vector.cpp @@ -52,6 +52,13 @@ LOG_DEFINE_CATEGORY(Vector) * The size of \a data must be equal to the dimension size Rows of the vector. */ +/** + * \fn Vector::Vector(const Vector<U, Rows> &other) + * \brief Construct a vector by converting another vector's element type + * \tparam U The source vector element type + * \param[in] other The vector to convert from + */ + /** * \fn T Vector::operator[](size_t i) const * \brief Index to an element in the vector
Extend the Vector class with a constructor to support casting to the storage type when the given parameters are compatible. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/internal/vector.h | 8 ++++++++ src/libcamera/vector.cpp | 7 +++++++ 2 files changed, 15 insertions(+)