| Message ID | 20250724152936.99830-2-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2025-07-24 16:29:35) > 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 | 19 +++++++++++++++++++ > test/geometry.cpp | 9 +++++++++ > 3 files changed, 30 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..8ceb75698 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -837,6 +837,25 @@ 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 > +{ > + 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()); I think that's a cleaner implementation. My only worries here are still on the naming. I can't convince myself if I'm sure if 'croppedBy' implicitly means centered or not ... I ... think it seems reasonable to be implicit ... any other thoughts / comments from others ? > +} > + > /** > * \brief Transform a Rectangle from one reference rectangle to another > * \param[in] source The \a source reference rectangle > diff --git a/test/geometry.cpp b/test/geometry.cpp > index 11df043b7..9864774f4 100644 > --- a/test/geometry.cpp > +++ b/test/geometry.cpp > @@ -466,6 +466,15 @@ protected: > return TestFail; > } > > + /* Rectangle::croppedBy() tests */ > + if (Rectangle(Size(300, 100)).croppedBy(2, 3) != > + Rectangle(50, 17, 200, 66) || > + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) != > + Rectangle(-14, 300, 128, 300)) { > + cout << "Rectangle::croppedBy() test failed " << endl; > + return TestFail; > + } thanks for adding the test. Simple - but enforces/documents/reinforces the behaviour expectations. > + > /* Rectangle::translateBy() tests */ > r = Rectangle(10, -20, 300, 400); > r.translateBy(Point(-30, 40)); > -- > 2.50.1 >
Hi 2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta: > Quoting Milan Zamazal (2025-07-24 16:29:35) >> 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 | 19 +++++++++++++++++++ >> test/geometry.cpp | 9 +++++++++ >> 3 files changed, 30 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..8ceb75698 100644 >> --- a/src/libcamera/geometry.cpp >> +++ b/src/libcamera/geometry.cpp >> @@ -837,6 +837,25 @@ 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 >> +{ >> + 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()); > > I think that's a cleaner implementation. My only worries here are still > on the naming. I can't convince myself if I'm sure if 'croppedBy' > implicitly means centered or not ... The name is not clear to me at all. I feel like it should be `scale[d]By()`. But of course that exists. Maybe we should extend `scaleBy()` with an "origin" parameter that determines the origin of the scaling (which is 0,0 for the current scaleBy() but would be the center for this version). Something like this: scaleBy(numer, denom, origin) const { x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x; y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y; w = uint64_t(this->w) * numer.x / denom.x; h = uint64_t(this->h) * numer.y / denom.y; return Rectangle(x, y, w, h); } Regards, Barnabás Pőcze > > I ... think it seems reasonable to be implicit ... any other thoughts / > comments from others ? > > >> +} >> + >> /** >> * \brief Transform a Rectangle from one reference rectangle to another >> * \param[in] source The \a source reference rectangle >> diff --git a/test/geometry.cpp b/test/geometry.cpp >> index 11df043b7..9864774f4 100644 >> --- a/test/geometry.cpp >> +++ b/test/geometry.cpp >> @@ -466,6 +466,15 @@ protected: >> return TestFail; >> } >> >> + /* Rectangle::croppedBy() tests */ >> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) != >> + Rectangle(50, 17, 200, 66) || >> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) != >> + Rectangle(-14, 300, 128, 300)) { >> + cout << "Rectangle::croppedBy() test failed " << endl; >> + return TestFail; >> + } > > thanks for adding the test. Simple - but enforces/documents/reinforces > the behaviour expectations. > > > >> + >> /* Rectangle::translateBy() tests */ >> r = Rectangle(10, -20, 300, 400); >> r.translateBy(Point(-30, 40)); >> -- >> 2.50.1 >>
On Wed, Aug 06, 2025 at 08:35:22PM +0200, Barnabás Pőcze wrote: > 2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta: > > Quoting Milan Zamazal (2025-07-24 16:29:35) > >> 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 | 19 +++++++++++++++++++ > >> test/geometry.cpp | 9 +++++++++ > >> 3 files changed, 30 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..8ceb75698 100644 > >> --- a/src/libcamera/geometry.cpp > >> +++ b/src/libcamera/geometry.cpp > >> @@ -837,6 +837,25 @@ 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 > >> +{ > >> + 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()); > > > > I think that's a cleaner implementation. My only worries here are still > > on the naming. I can't convince myself if I'm sure if 'croppedBy' > > implicitly means centered or not ... > > The name is not clear to me at all. I feel like it should be `scale[d]By()`. > But of course that exists. Maybe we should extend `scaleBy()` with an "origin" > parameter that determines the origin of the scaling (which is 0,0 for the current scaleBy() > but would be the center for this version). Something like this: > > scaleBy(numer, denom, origin) const { > x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x; > y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y; > w = uint64_t(this->w) * numer.x / denom.x; > h = uint64_t(this->h) * numer.y / denom.y; > return Rectangle(x, y, w, h); > } I think this is becoming too specialized. How about composing multiple simpler geometry functions to obtain the same result ? We could add a scaledBy() function to Size, which would allow writing r = rect.size().scaledBy(numerator, denominator).centeredTo(rect.center()); or rather with a float ratio instead of numerator and denominator if the x and y ratios are identical: r = rect.size().scaledBy(ratio).centeredTo(rect.center()); > > I ... think it seems reasonable to be implicit ... any other thoughts / > > comments from others ? > > > >> +} > >> + > >> /** > >> * \brief Transform a Rectangle from one reference rectangle to another > >> * \param[in] source The \a source reference rectangle > >> diff --git a/test/geometry.cpp b/test/geometry.cpp > >> index 11df043b7..9864774f4 100644 > >> --- a/test/geometry.cpp > >> +++ b/test/geometry.cpp > >> @@ -466,6 +466,15 @@ protected: > >> return TestFail; > >> } > >> > >> + /* Rectangle::croppedBy() tests */ > >> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) != > >> + Rectangle(50, 17, 200, 66) || > >> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) != > >> + Rectangle(-14, 300, 128, 300)) { > >> + cout << "Rectangle::croppedBy() test failed " << endl; > >> + return TestFail; > >> + } > > > > thanks for adding the test. Simple - but enforces/documents/reinforces > > the behaviour expectations. > > > >> + > >> /* Rectangle::translateBy() tests */ > >> r = Rectangle(10, -20, 300, 400); > >> r.translateBy(Point(-30, 40));
Hi 2025. 08. 07. 15:06 keltezéssel, Laurent Pinchart írta: > On Wed, Aug 06, 2025 at 08:35:22PM +0200, Barnabás Pőcze wrote: >> 2025. 08. 06. 19:42 keltezéssel, Kieran Bingham írta: >>> Quoting Milan Zamazal (2025-07-24 16:29:35) >>>> 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 | 19 +++++++++++++++++++ >>>> test/geometry.cpp | 9 +++++++++ >>>> 3 files changed, 30 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..8ceb75698 100644 >>>> --- a/src/libcamera/geometry.cpp >>>> +++ b/src/libcamera/geometry.cpp >>>> @@ -837,6 +837,25 @@ 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 >>>> +{ >>>> + 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()); >>> >>> I think that's a cleaner implementation. My only worries here are still >>> on the naming. I can't convince myself if I'm sure if 'croppedBy' >>> implicitly means centered or not ... >> >> The name is not clear to me at all. I feel like it should be `scale[d]By()`. >> But of course that exists. Maybe we should extend `scaleBy()` with an "origin" >> parameter that determines the origin of the scaling (which is 0,0 for the current scaleBy() >> but would be the center for this version). Something like this: >> >> scaleBy(numer, denom, origin) const { >> x = (int64_t(this->x) - int64_t(origin.x)) * numer.x / denom.x + origin.x; >> y = (int64_t(this->y) - int64_t(origin.x)) * numer.y / denom.y + origin.y; >> w = uint64_t(this->w) * numer.x / denom.x; >> h = uint64_t(this->h) * numer.y / denom.y; >> return Rectangle(x, y, w, h); >> } > > I think this is becoming too specialized. How about composing multiple Interesting, I feel this is an entirely natural extension, in a sense "revealing" a parameter of the transformation that has been hard-coded so far. > simpler geometry functions to obtain the same result ? We could add a > scaledBy() function to Size, which would allow writing > > r = rect.size().scaledBy(numerator, denominator).centeredTo(rect.center()); > > or rather with a float ratio instead of numerator and denominator if the > x and y ratios are identical: > > r = rect.size().scaledBy(ratio).centeredTo(rect.center()); > >>> I ... think it seems reasonable to be implicit ... any other thoughts / >>> comments from others ? In my opinion this looks good as well. All that seems to be needed is `Size::scaledBy()`. But there is operator*(), so maybe this is already possible: (rect.size() * ratio).centeredTo(rect.center()) Regards, Barnabás Pőcze >>> >>>> +} >>>> + >>>> /** >>>> * \brief Transform a Rectangle from one reference rectangle to another >>>> * \param[in] source The \a source reference rectangle >>>> diff --git a/test/geometry.cpp b/test/geometry.cpp >>>> index 11df043b7..9864774f4 100644 >>>> --- a/test/geometry.cpp >>>> +++ b/test/geometry.cpp >>>> @@ -466,6 +466,15 @@ protected: >>>> return TestFail; >>>> } >>>> >>>> + /* Rectangle::croppedBy() tests */ >>>> + if (Rectangle(Size(300, 100)).croppedBy(2, 3) != >>>> + Rectangle(50, 17, 200, 66) || >>>> + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) != >>>> + Rectangle(-14, 300, 128, 300)) { >>>> + cout << "Rectangle::croppedBy() test failed " << endl; >>>> + return TestFail; >>>> + } >>> >>> thanks for adding the test. Simple - but enforces/documents/reinforces >>> the behaviour expectations. >>> >>>> + >>>> /* Rectangle::translateBy() tests */ >>>> r = Rectangle(10, -20, 300, 400); >>>> r.translateBy(Point(-30, 40)); >
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..8ceb75698 100644 --- a/src/libcamera/geometry.cpp +++ b/src/libcamera/geometry.cpp @@ -837,6 +837,25 @@ 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 +{ + 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()); +} + /** * \brief Transform a Rectangle from one reference rectangle to another * \param[in] source The \a source reference rectangle diff --git a/test/geometry.cpp b/test/geometry.cpp index 11df043b7..9864774f4 100644 --- a/test/geometry.cpp +++ b/test/geometry.cpp @@ -466,6 +466,15 @@ protected: return TestFail; } + /* Rectangle::croppedBy() tests */ + if (Rectangle(Size(300, 100)).croppedBy(2, 3) != + Rectangle(50, 17, 200, 66) || + Rectangle(-100, 100, 300, 700).croppedBy(3, 7) != + Rectangle(-14, 300, 128, 300)) { + cout << "Rectangle::croppedBy() test failed " << endl; + return TestFail; + } + /* Rectangle::translateBy() tests */ r = Rectangle(10, -20, 300, 400); r.translateBy(Point(-30, 40));
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 | 19 +++++++++++++++++++ test/geometry.cpp | 9 +++++++++ 3 files changed, 30 insertions(+)