| Message ID | 20190527001543.13593-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Mon, May 27, 2019 at 02:15:29AM +0200, Niklas Söderlund wrote: > The size range described might be subject to certain stepping > limitations. Make it possible to record this information. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/geometry.h | 13 +++++++++++-- > src/libcamera/geometry.cpp | 37 ++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > index ec5ed2bee196c82d..1c9267b14274cb5d 100644 > --- a/include/libcamera/geometry.h > +++ b/include/libcamera/geometry.h > @@ -73,18 +73,27 @@ struct SizeRange { > } > > SizeRange(unsigned int width, unsigned int height) > - : min(width, height), max(width, height) > + : min(width, height), max(width, height), hStep(0), vStep(0) > { > } > > SizeRange(unsigned int minW, unsigned int minH, > unsigned int maxW, unsigned int maxH) > - : min(minW, minH), max(maxW, maxH) > + : min(minW, minH), max(maxW, maxH), hStep(1), vStep(1) > + { > + } > + > + SizeRange(unsigned int minW, unsigned int minH, > + unsigned int maxW, unsigned int maxH, > + unsigned int hstep, unsigned int vstep) > + : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep) > { > } > > Size min; > Size max; > + unsigned int hStep; > + unsigned int vStep; > }; > > bool operator==(const SizeRange &lhs, const SizeRange &rhs); > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > index cc25b816e6796ba1..e7f1bafd40e944f5 100644 > --- a/src/libcamera/geometry.cpp > +++ b/src/libcamera/geometry.cpp > @@ -189,6 +189,20 @@ bool operator<(const Size &lhs, const Size &rhs) > * SizeRange describes a range of sizes included in the [min, max] > * interval for both the width and the height. If the minimum and > * maximum sizes are identical it represents a single size. > + * > + * The SizeRange may also contain a vertical and horizontal stepping. > + * The stepping values add additional constrains to the width and height > + * values described by the range. nit: I would write * The SizeRange may also describe a size increment stepping value, * which represents the size increment unit of both the vertical and * horizontal sizes. * * The stepping value is used as size multiplication factor when * calculating a the actual sizes when expressed in pixel units: > + * > + * width = min.width + min.hStep * x > + * height = min.height + min.vStep * y Not sure 'min.h/vStep' is right, it should be just 'hStep' and 'vStep' if I got this right. And I would let 'min' out of the example and just provide: width = w * hStep; height = h * hStep; Up to you.. > + * > + * Where: > + * > + * width <= max.width > + * height < max.height > + * > + * For SizeRanges describing a single size the step values have no effect. "the size increment step value is fixed to 0" > */ > > /** > @@ -212,6 +226,19 @@ bool operator<(const Size &lhs, const Size &rhs) > * \param[in] maxH The maximum height > */ > > +/** > + * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH, > + * unsigned int maxW, unsigned int maxH, > + * unsigned int hstep, unsigned int vstep) > + * \brief Construct an initialized size range > + * \param[in] minW The minimum width > + * \param[in] minH The minimum height > + * \param[in] maxW The maximum width > + * \param[in] maxH The maximum height > + * \param[in] hstep The horizontal step > + * \param[in] vstep The vertical step > + */ > + > /** > * \var SizeRange::min > * \brief The minimum size > @@ -222,6 +249,16 @@ bool operator<(const Size &lhs, const Size &rhs) > * \brief The maximum size > */ > > +/** > + * \var SizeRange::hStep > + * \brief The horizontal step "horizontal size increment step" > + */ > + > +/** > + * \var SizeRange::vStep > + * \brief The vertical step likewise. Documentation apart, the patch is fine Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + */ > + > /** > * \brief Compare size ranges for equality > * \return True if the two size ranges are equal, false otherwise > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Mon, May 27, 2019 at 11:35:03AM +0200, Jacopo Mondi wrote: > On Mon, May 27, 2019 at 02:15:29AM +0200, Niklas Söderlund wrote: > > The size range described might be subject to certain stepping > > limitations. Make it possible to record this information. The word "stepping" (here and in all other patches) sounds weird to me. Is it just me, or should we talk about "step" instead ? > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > include/libcamera/geometry.h | 13 +++++++++++-- > > src/libcamera/geometry.cpp | 37 ++++++++++++++++++++++++++++++++++++ > > 2 files changed, 48 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h > > index ec5ed2bee196c82d..1c9267b14274cb5d 100644 > > --- a/include/libcamera/geometry.h > > +++ b/include/libcamera/geometry.h > > @@ -73,18 +73,27 @@ struct SizeRange { > > } > > > > SizeRange(unsigned int width, unsigned int height) > > - : min(width, height), max(width, height) > > + : min(width, height), max(width, height), hStep(0), vStep(0) > > { > > } > > > > SizeRange(unsigned int minW, unsigned int minH, > > unsigned int maxW, unsigned int maxH) > > - : min(minW, minH), max(maxW, maxH) > > + : min(minW, minH), max(maxW, maxH), hStep(1), vStep(1) > > + { > > + } > > + > > + SizeRange(unsigned int minW, unsigned int minH, > > + unsigned int maxW, unsigned int maxH, > > + unsigned int hstep, unsigned int vstep) > > + : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep) > > { > > } > > > > Size min; > > Size max; > > + unsigned int hStep; > > + unsigned int vStep; > > }; > > > > bool operator==(const SizeRange &lhs, const SizeRange &rhs); > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp > > index cc25b816e6796ba1..e7f1bafd40e944f5 100644 > > --- a/src/libcamera/geometry.cpp > > +++ b/src/libcamera/geometry.cpp > > @@ -189,6 +189,20 @@ bool operator<(const Size &lhs, const Size &rhs) > > * SizeRange describes a range of sizes included in the [min, max] > > * interval for both the width and the height. If the minimum and > > * maximum sizes are identical it represents a single size. > > + * > > + * The SizeRange may also contain a vertical and horizontal stepping. > > + * The stepping values add additional constrains to the width and height > > + * values described by the range. > > nit: I would write > > * The SizeRange may also describe a size increment stepping value, > * which represents the size increment unit of both the vertical and > * horizontal sizes. > * > * The stepping value is used as size multiplication factor when > * calculating a the actual sizes when expressed in pixel units: > > > + * > > + * width = min.width + min.hStep * x > > + * height = min.height + min.vStep * y > > Not sure 'min.h/vStep' is right, it should be just 'hStep' and 'vStep' > if I got this right. And I would let 'min' out of the example and just > provide: > > width = w * hStep; > height = h * hStep; > > Up to you.. > > > + * > > + * Where: > > + * > > + * width <= max.width > > + * height < max.height > > + * > > + * For SizeRanges describing a single size the step values have no effect. > > "the size increment step value is fixed to 0" If we have ranges with steps set to 0 it should indeed be explicitly documented, otherwise applications will crash with a division by 0 at some point. We should also be very careful with this, and it may be best to set the steps to 1 in this case to avoid this issue. > > */ > > > > /** > > @@ -212,6 +226,19 @@ bool operator<(const Size &lhs, const Size &rhs) > > * \param[in] maxH The maximum height > > */ > > > > +/** > > + * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH, > > + * unsigned int maxW, unsigned int maxH, > > + * unsigned int hstep, unsigned int vstep) > > + * \brief Construct an initialized size range > > + * \param[in] minW The minimum width > > + * \param[in] minH The minimum height > > + * \param[in] maxW The maximum width > > + * \param[in] maxH The maximum height > > + * \param[in] hstep The horizontal step > > + * \param[in] vstep The vertical step > > + */ > > + > > /** > > * \var SizeRange::min > > * \brief The minimum size > > @@ -222,6 +249,16 @@ bool operator<(const Size &lhs, const Size &rhs) > > * \brief The maximum size > > */ > > > > +/** > > + * \var SizeRange::hStep > > + * \brief The horizontal step > > "horizontal size increment step" > > > + */ > > + > > +/** > > + * \var SizeRange::vStep > > + * \brief The vertical step > > likewise. > > Documentation apart, the patch is fine > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + */ > > + > > /** > > * \brief Compare size ranges for equality > > * \return True if the two size ranges are equal, false otherwise
diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h index ec5ed2bee196c82d..1c9267b14274cb5d 100644 --- a/include/libcamera/geometry.h +++ b/include/libcamera/geometry.h @@ -73,18 +73,27 @@ struct SizeRange { } SizeRange(unsigned int width, unsigned int height) - : min(width, height), max(width, height) + : min(width, height), max(width, height), hStep(0), vStep(0) { } SizeRange(unsigned int minW, unsigned int minH, unsigned int maxW, unsigned int maxH) - : min(minW, minH), max(maxW, maxH) + : min(minW, minH), max(maxW, maxH), hStep(1), vStep(1) + { + } + + SizeRange(unsigned int minW, unsigned int minH, + unsigned int maxW, unsigned int maxH, + unsigned int hstep, unsigned int vstep) + : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep) { } Size min; Size max; + unsigned int hStep; + unsigned int vStep; }; bool operator==(const SizeRange &lhs, const SizeRange &rhs); diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp index cc25b816e6796ba1..e7f1bafd40e944f5 100644 --- a/src/libcamera/geometry.cpp +++ b/src/libcamera/geometry.cpp @@ -189,6 +189,20 @@ bool operator<(const Size &lhs, const Size &rhs) * SizeRange describes a range of sizes included in the [min, max] * interval for both the width and the height. If the minimum and * maximum sizes are identical it represents a single size. + * + * The SizeRange may also contain a vertical and horizontal stepping. + * The stepping values add additional constrains to the width and height + * values described by the range. + * + * width = min.width + min.hStep * x + * height = min.height + min.vStep * y + * + * Where: + * + * width <= max.width + * height < max.height + * + * For SizeRanges describing a single size the step values have no effect. */ /** @@ -212,6 +226,19 @@ bool operator<(const Size &lhs, const Size &rhs) * \param[in] maxH The maximum height */ +/** + * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH, + * unsigned int maxW, unsigned int maxH, + * unsigned int hstep, unsigned int vstep) + * \brief Construct an initialized size range + * \param[in] minW The minimum width + * \param[in] minH The minimum height + * \param[in] maxW The maximum width + * \param[in] maxH The maximum height + * \param[in] hstep The horizontal step + * \param[in] vstep The vertical step + */ + /** * \var SizeRange::min * \brief The minimum size @@ -222,6 +249,16 @@ bool operator<(const Size &lhs, const Size &rhs) * \brief The maximum size */ +/** + * \var SizeRange::hStep + * \brief The horizontal step + */ + +/** + * \var SizeRange::vStep + * \brief The vertical step + */ + /** * \brief Compare size ranges for equality * \return True if the two size ranges are equal, false otherwise
The size range described might be subject to certain stepping limitations. Make it possible to record this information. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- include/libcamera/geometry.h | 13 +++++++++++-- src/libcamera/geometry.cpp | 37 ++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 2 deletions(-)