| Message ID | 20250718144456.58625-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-07-18 15:44:55) > Let's add a new Rectangle method that returns the coordinates of the > central area of the original rectangle cropped by given > numerator/denominator fraction. This is useful to specify a limited > area of the image to operate on, for example computing statistics only > from a reduced area for speedup. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > include/libcamera/geometry.h | 2 ++ > src/libcamera/geometry.cpp | 18 ++++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index f322e3d5b..603c9a117 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -297,6 +297,8 @@ public: > [[nodiscard]] Rectangle scaledBy(const Size &numerator, > const Size &denominator) const; > [[nodiscard]] Rectangle translatedBy(const Point &point) const; > + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator, > + const unsigned int denominator) const; > > Rectangle transformedBetween(const Rectangle &source, > const Rectangle &target) const; > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index 81cc8cd53..8c1b156c5 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -837,6 +837,24 @@ Rectangle Rectangle::translatedBy(const Point &point) const > return { x + point.x, y + point.y, width, height }; > } > > +/** > + * \brief Return a central area crop of the rectangle > + * \param[in] numerator The numerator of the crop factor > + * \param[in] denominator The denominator of the crop factor > + * > + * The rectangle of the central part of the original rectangle is computed, of > + * numerator/denominator times the original width and height. > + * > + * \return The cropped Rectangle coordinates > + */ > +Rectangle Rectangle::croppedBy(const unsigned int numerator, > + const unsigned int denominator) const > +{ > + const unsigned int w = numerator * width / denominator; > + const unsigned int h = numerator * height / denominator; In Rectangle::scaledBy the numbers are cast to 64 bits to ensure that overflows don't happen. I suspect we should do the same here... Or use that call as a helper ? > + return Rectangle((width - w) / 2, (height - h) / 2, w, h); Also scaledBy accounts for the x, y coordinates too. We can't always assume that a rectangle being cropped starts at 0,0! For example the incoming Rectangle you're croppping might already have a crop applied to represent the active pixels of a sensor array... > +} I wonder if implementing this as : Rectangle Rectangle::croppedBy(const unsigned int numerator, const unsigned int denominator) const { Size cropped = this.size() * numerator / denominator); return cropped.centeredTo(this.center()); } would solve the above concerns. Actually I don't think that would protect against overflow ... but maybe it's not a big deal (yet?) So maybe inlining the Size cropped still makes sense. Which makes it more like: Rectangle Rectangle::croppedBy(const unsigned int numerator, const unsigned int denominator) const { unsigned int w = static_cast<uint64_t>(width) * numerator / denominator; unsigned int h = static_cast<uint64_t>(height) * numerator / denominator; return Size(w, h).centeredTo(center()); } ? Perhaps we should also add both: croppedBy() and cropBy() though to have the version that modifies in place? Finally, any additions to geometry.cpp likely need associated tests in test/geometry.cpp -- Kieran > + > /** > * \brief Transform a Rectangle from one reference rectangle to another > * \param[in] source The \a source reference rectangle > -- > 2.50.1 >
Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2025-07-18 15:44:55) >> Let's add a new Rectangle method that returns the coordinates of the >> central area of the original rectangle cropped by given > >> numerator/denominator fraction. This is useful to specify a limited >> area of the image to operate on, for example computing statistics only >> from a reduced area for speedup. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> include/libcamera/geometry.h | 2 ++ >> src/libcamera/geometry.cpp | 18 ++++++++++++++++++ >> 2 files changed, 20 insertions(+) >> >> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h >> index f322e3d5b..603c9a117 100644 >> --- a/include/libcamera/geometry.h >> +++ b/include/libcamera/geometry.h >> @@ -297,6 +297,8 @@ public: >> [[nodiscard]] Rectangle scaledBy(const Size &numerator, >> const Size &denominator) const; >> [[nodiscard]] Rectangle translatedBy(const Point &point) const; >> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator, >> + const unsigned int denominator) const; >> >> Rectangle transformedBetween(const Rectangle &source, >> const Rectangle &target) const; >> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp >> index 81cc8cd53..8c1b156c5 100644 >> --- a/src/libcamera/geometry.cpp >> +++ b/src/libcamera/geometry.cpp >> @@ -837,6 +837,24 @@ Rectangle Rectangle::translatedBy(const Point &point) const >> return { x + point.x, y + point.y, width, height }; >> } >> >> +/** >> + * \brief Return a central area crop of the rectangle >> + * \param[in] numerator The numerator of the crop factor >> + * \param[in] denominator The denominator of the crop factor >> + * >> + * The rectangle of the central part of the original rectangle is computed, of >> + * numerator/denominator times the original width and height. >> + * >> + * \return The cropped Rectangle coordinates >> + */ >> +Rectangle Rectangle::croppedBy(const unsigned int numerator, >> + const unsigned int denominator) const >> +{ >> + const unsigned int w = numerator * width / denominator; >> + const unsigned int h = numerator * height / denominator; > > In Rectangle::scaledBy the numbers are cast to 64 bits to ensure that > overflows don't happen. I suspect we should do the same here... > > Or use that call as a helper ? > > >> + return Rectangle((width - w) / 2, (height - h) / 2, w, h); > > Also scaledBy accounts for the x, y coordinates too. We can't always > assume that a rectangle being cropped starts at 0,0! > > For example the incoming Rectangle you're croppping might already have a > crop applied to represent the active pixels of a sensor array... > >> +} > > I wonder if implementing this as : > > Rectangle Rectangle::croppedBy(const unsigned int numerator, > const unsigned int denominator) const > { > Size cropped = this.size() * numerator / denominator); > return cropped.centeredTo(this.center()); > } > > > would solve the above concerns. Actually I don't think that would > protect against overflow ... but maybe it's not a big deal (yet?) So > maybe inlining the Size cropped still makes sense. > > Which makes it more like: > > Rectangle Rectangle::croppedBy(const unsigned int numerator, > const unsigned int denominator) const > { > unsigned int w = static_cast<uint64_t>(width) * numerator / denominator; > unsigned int h = static_cast<uint64_t>(height) * numerator / denominator; > > return Size(w, h).centeredTo(center()); > > } > > ? This version looks safe and right, I'll use it in v2, thank you for the suggestion. > Perhaps we should also add both: > > croppedBy() and cropBy() > > though to have the version that modifies in place? We don't have currently use for the in-place-modification version, should it be added anyway? > Finally, any additions to geometry.cpp likely need associated tests in > test/geometry.cpp I'll add them. > -- > Kieran > > > >> + >> /** >> * \brief Transform a Rectangle from one reference rectangle to another >> * \param[in] source The \a source reference rectangle >> -- >> 2.50.1 >>
Quoting Milan Zamazal (2025-07-21 19:58:55) > Hi Kieran, > > thank you for review. > > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2025-07-18 15:44:55) > >> Let's add a new Rectangle method that returns the coordinates of the > >> central area of the original rectangle cropped by given > > > >> numerator/denominator fraction. This is useful to specify a limited > >> area of the image to operate on, for example computing statistics only > >> from a reduced area for speedup. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> include/libcamera/geometry.h | 2 ++ > >> src/libcamera/geometry.cpp | 18 ++++++++++++++++++ > >> 2 files changed, 20 insertions(+) > >> > >> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > >> index f322e3d5b..603c9a117 100644 > >> --- a/include/libcamera/geometry.h > >> +++ b/include/libcamera/geometry.h > >> @@ -297,6 +297,8 @@ public: > >> [[nodiscard]] Rectangle scaledBy(const Size &numerator, > >> const Size &denominator) const; > >> [[nodiscard]] Rectangle translatedBy(const Point &point) const; > >> + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator, > >> + const unsigned int denominator) const; > >> > >> Rectangle transformedBetween(const Rectangle &source, > >> const Rectangle &target) const; > >> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > >> index 81cc8cd53..8c1b156c5 100644 > >> --- a/src/libcamera/geometry.cpp > >> +++ b/src/libcamera/geometry.cpp > >> @@ -837,6 +837,24 @@ Rectangle Rectangle::translatedBy(const Point &point) const > >> return { x + point.x, y + point.y, width, height }; > >> } > >> > >> +/** > >> + * \brief Return a central area crop of the rectangle > >> + * \param[in] numerator The numerator of the crop factor > >> + * \param[in] denominator The denominator of the crop factor > >> + * > >> + * The rectangle of the central part of the original rectangle is computed, of > >> + * numerator/denominator times the original width and height. > >> + * > >> + * \return The cropped Rectangle coordinates > >> + */ > >> +Rectangle Rectangle::croppedBy(const unsigned int numerator, > >> + const unsigned int denominator) const > >> +{ > >> + const unsigned int w = numerator * width / denominator; > >> + const unsigned int h = numerator * height / denominator; > > > > In Rectangle::scaledBy the numbers are cast to 64 bits to ensure that > > overflows don't happen. I suspect we should do the same here... > > > > Or use that call as a helper ? > > > > > >> + return Rectangle((width - w) / 2, (height - h) / 2, w, h); > > > > Also scaledBy accounts for the x, y coordinates too. We can't always > > assume that a rectangle being cropped starts at 0,0! > > > > For example the incoming Rectangle you're croppping might already have a > > crop applied to represent the active pixels of a sensor array... > > > >> +} > > > > I wonder if implementing this as : > > > > Rectangle Rectangle::croppedBy(const unsigned int numerator, > > const unsigned int denominator) const > > { > > Size cropped = this.size() * numerator / denominator); > > return cropped.centeredTo(this.center()); > > } > > > > > > would solve the above concerns. Actually I don't think that would > > protect against overflow ... but maybe it's not a big deal (yet?) So > > maybe inlining the Size cropped still makes sense. > > > > Which makes it more like: > > > > Rectangle Rectangle::croppedBy(const unsigned int numerator, > > const unsigned int denominator) const > > { > > unsigned int w = static_cast<uint64_t>(width) * numerator / denominator; > > unsigned int h = static_cast<uint64_t>(height) * numerator / denominator; > > > > return Size(w, h).centeredTo(center()); > > > > } > > > > ? > > This version looks safe and right, I'll use it in v2, thank you for the > suggestion. > > > Perhaps we should also add both: > > > > croppedBy() and cropBy() > > > > though to have the version that modifies in place? > > We don't have currently use for the in-place-modification version, > should it be added anyway? If they're not needed at the moment it's fine not to add them. And probably simpler for now. It's just the symmetry with the scaledBy/scaleBy I was comparing against. > > > Finally, any additions to geometry.cpp likely need associated tests in > > test/geometry.cpp > > I'll add them. Thanks. Kieran > > > -- > > Kieran > > > > > > > >> + > >> /** > >> * \brief Transform a Rectangle from one reference rectangle to another > >> * \param[in] source The \a source reference rectangle > >> -- > >> 2.50.1 > >> >
diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h index f322e3d5b..603c9a117 100644 --- a/include/libcamera/geometry.h +++ b/include/libcamera/geometry.h @@ -297,6 +297,8 @@ public: [[nodiscard]] Rectangle scaledBy(const Size &numerator, const Size &denominator) const; [[nodiscard]] Rectangle translatedBy(const Point &point) const; + [[nodiscard]] Rectangle croppedBy(const unsigned int numerator, + const unsigned int denominator) const; Rectangle transformedBetween(const Rectangle &source, const Rectangle &target) const; diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp index 81cc8cd53..8c1b156c5 100644 --- a/src/libcamera/geometry.cpp +++ b/src/libcamera/geometry.cpp @@ -837,6 +837,24 @@ Rectangle Rectangle::translatedBy(const Point &point) const return { x + point.x, y + point.y, width, height }; } +/** + * \brief Return a central area crop of the rectangle + * \param[in] numerator The numerator of the crop factor + * \param[in] denominator The denominator of the crop factor + * + * The rectangle of the central part of the original rectangle is computed, of + * numerator/denominator times the original width and height. + * + * \return The cropped Rectangle coordinates + */ +Rectangle Rectangle::croppedBy(const unsigned int numerator, + const unsigned int denominator) const +{ + const unsigned int w = numerator * width / denominator; + const unsigned int h = numerator * height / denominator; + return Rectangle((width - w) / 2, (height - h) / 2, w, h); +} + /** * \brief Transform a Rectangle from one reference rectangle to another * \param[in] source The \a source reference rectangle
Let's add a new Rectangle method that returns the coordinates of the central area of the original rectangle cropped by given numerator/denominator fraction. This is useful to specify a limited area of the image to operate on, for example computing statistics only from a reduced area for speedup. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- include/libcamera/geometry.h | 2 ++ src/libcamera/geometry.cpp | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+)