[libcamera-devel] [PATCH] libcamera: geometry: Construct SizeRange from Size
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Mar 19 12:11:00 CET 2020
Hi Laurent,
On 19/03/2020 00:11, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your patch.
>
> On 2020-03-18 19:23:37 +0200, Laurent Pinchart wrote:
>> The SizeRange constructors take minimum and maximum width and height
>> values as separate arguments. We have a Size class to convey size
>> information, use it in the constructors, and update the callers.
>
> I like it, it's clear in the users which values are one 'group' of width
> and height.
Strongly seconded!
This:
- SizeRange{ 48, 48, 4096, 2160 }
+ SizeRange{ { 48, 48 }, { 4096, 2160 } }
Is *so* much better!
>
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
>> ---
>> include/libcamera/geometry.h | 14 ++++++--------
>> src/libcamera/geometry.cpp | 27 ++++++++++-----------------
>> src/libcamera/pipeline/vimc.cpp | 2 +-
>> src/libcamera/stream.cpp | 2 +-
>> src/libcamera/v4l2_subdevice.cpp | 4 ++--
>> src/libcamera/v4l2_videodevice.cpp | 20 ++++++++++----------
>> test/stream/stream_formats.cpp | 12 ++++++------
>> 7 files changed, 36 insertions(+), 45 deletions(-)
>>
>> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
>> index 52f4d010de76..7f1b29fe8c19 100644
>> --- a/include/libcamera/geometry.h
>> +++ b/include/libcamera/geometry.h
>> @@ -74,21 +74,19 @@ public:
>> {
>> }
>>
>> - SizeRange(unsigned int width, unsigned int height)
>> - : min(width, height), max(width, height), hStep(1), vStep(1)
>> + SizeRange(const Size &size)
>> + : min(size), max(size), hStep(1), vStep(1)
>> {
>> }
>>
>> - SizeRange(unsigned int minW, unsigned int minH,
>> - unsigned int maxW, unsigned int maxH)
>> - : min(minW, minH), max(maxW, maxH), hStep(1), vStep(1)
>> + SizeRange(const Size &minSize, const Size &maxSize)
>> + : min(minSize), max(maxSize), hStep(1), vStep(1)
>> {
>> }
>>
>> - SizeRange(unsigned int minW, unsigned int minH,
>> - unsigned int maxW, unsigned int maxH,
>> + SizeRange(const Size &minSize, const Size &maxSize,
>> unsigned int hstep, unsigned int vstep)
>> - : min(minW, minH), max(maxW, maxH), hStep(hstep), vStep(vstep)
>> + : min(minSize), max(maxSize), hStep(hstep), vStep(vstep)
>> {
>> }
>>
>> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
>> index 92c53f64a58b..75cdcc7c42f1 100644
>> --- a/src/libcamera/geometry.cpp
>> +++ b/src/libcamera/geometry.cpp
>> @@ -217,31 +217,24 @@ bool operator<(const Size &lhs, const Size &rhs)
>> */
>>
>> /**
>> - * \fn SizeRange::SizeRange(unsigned int width, unsigned int height)
>> + * \fn SizeRange::SizeRange(const Size &size)
>> * \brief Construct a size range representing a single size
>> - * \param[in] width The size width
>> - * \param[in] height The size height
>> + * \param[in] size The size
>> */
>>
>> /**
>> - * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH,
>> - * unsigned int maxW, unsigned int maxH)
>> - * \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
>> + * \fn SizeRange::SizeRange(const Size &minSize, const Size &maxSize)
>> + * \brief Construct a size range with specified min and max and steps of 1
That sounds a little odd, probably because of the two 'and's, and no
separator. Is it the specified min, with max and steps of 1? (of course not)
Maybe:
> \brief Construct a size range with specified min and max, and steps of 1
or
> \brief Construct a size range with specified min and max with steps of 1
or
> \brief Construct a size range with specified min and max with a step of 1
Or just leave out the step...?
>> + * \param[in] minSize The minimum size
>> + * \param[in] maxSize The maximum size
>> */
>>
>> /**
>> - * \fn SizeRange::SizeRange(unsigned int minW, unsigned int minH,
>> - * unsigned int maxW, unsigned int maxH,
>> + * \fn SizeRange::SizeRange(const Size &minSize, const Size &maxSize,
>> * 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
>> + * \brief Construct a size range with specified min, max and step
>> + * \param[in] minSize The minimum size
>> + * \param[in] maxSize The maximum size
>> * \param[in] hstep The horizontal step
>> * \param[in] vstep The vertical step
>> */
>> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
>> index fa84f0c1b20d..cbf330614bd6 100644
>> --- a/src/libcamera/pipeline/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc.cpp
>> @@ -177,7 +177,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>> for (PixelFormat pixelformat : pixelformats) {
>> /* The scaler hardcodes a x3 scale-up ratio. */
>> std::vector<SizeRange> sizes{
>> - SizeRange{ 48, 48, 4096, 2160 }
>> + SizeRange{ { 48, 48 }, { 4096, 2160 } }
I really love this.
4 consecutive numbers have suddenly got meaning.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>> };
>> formats[pixelformat] = sizes;
>> }
>> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
>> index e61484caf715..ea3d214271ad 100644
>> --- a/src/libcamera/stream.cpp
>> +++ b/src/libcamera/stream.cpp
>> @@ -251,7 +251,7 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
>> return ranges[0];
>>
>> LOG(Stream, Debug) << "Building range from discrete sizes";
>> - SizeRange range(UINT_MAX, UINT_MAX, 0, 0);
>> + SizeRange range({ UINT_MAX, UINT_MAX }, { 0, 0 });
>> for (const SizeRange &limit : ranges) {
>> if (limit.min < range.min)
>> range.min = limit.min;
>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
>> index f2bcd7f73c5c..8b9da81e8ab3 100644
>> --- a/src/libcamera/v4l2_subdevice.cpp
>> +++ b/src/libcamera/v4l2_subdevice.cpp
>> @@ -313,8 +313,8 @@ std::vector<SizeRange> V4L2Subdevice::enumPadSizes(unsigned int pad,
>> if (ret)
>> break;
>>
>> - sizes.emplace_back(sizeEnum.min_width, sizeEnum.min_height,
>> - sizeEnum.max_width, sizeEnum.max_height);
>> + sizes.emplace_back(Size{ sizeEnum.min_width, sizeEnum.min_height },
>> + Size{ sizeEnum.max_width, sizeEnum.max_height });
>> }
>>
>> if (ret < 0 && ret != -EINVAL && ret != -ENOTTY) {
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index fde8bd88e254..56251a465807 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -971,20 +971,20 @@ std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat)
>>
>> switch (frameSize.type) {
>> case V4L2_FRMSIZE_TYPE_DISCRETE:
>> - sizes.emplace_back(frameSize.discrete.width,
>> - frameSize.discrete.height);
>> + sizes.emplace_back(Size{ frameSize.discrete.width,
>> + frameSize.discrete.height });
>> break;
>> case V4L2_FRMSIZE_TYPE_CONTINUOUS:
>> - sizes.emplace_back(frameSize.stepwise.min_width,
>> - frameSize.stepwise.min_height,
>> - frameSize.stepwise.max_width,
>> - frameSize.stepwise.max_height);
>> + sizes.emplace_back(Size{ frameSize.stepwise.min_width,
>> + frameSize.stepwise.min_height },
>> + Size{ frameSize.stepwise.max_width,
>> + frameSize.stepwise.max_height });
>> break;
>> case V4L2_FRMSIZE_TYPE_STEPWISE:
>> - sizes.emplace_back(frameSize.stepwise.min_width,
>> - frameSize.stepwise.min_height,
>> - frameSize.stepwise.max_width,
>> - frameSize.stepwise.max_height,
>> + sizes.emplace_back(Size{ frameSize.stepwise.min_width,
>> + frameSize.stepwise.min_height },
>> + Size{ frameSize.stepwise.max_width,
>> + frameSize.stepwise.max_height },
>> frameSize.stepwise.step_width,
>> frameSize.stepwise.step_height);
>> break;
>> diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp
>> index 92f1574b8a0b..9353d0085e19 100644
>> --- a/test/stream/stream_formats.cpp
>> +++ b/test/stream/stream_formats.cpp
>> @@ -55,8 +55,8 @@ protected:
>> {
>> /* Test discrete sizes */
>> StreamFormats discrete({
>> - { PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },
>> - { PixelFormat(2), { SizeRange(300, 300), SizeRange(400, 400) } },
>> + { PixelFormat(1), { SizeRange({ 100, 100 }), SizeRange({ 200, 200 }) } },
>> + { PixelFormat(2), { SizeRange({ 300, 300 }), SizeRange({ 400, 400 }) } },
>> });
>>
>> if (testSizes("discrete 1", discrete.sizes(PixelFormat(1)),
>> @@ -68,10 +68,10 @@ protected:
>>
>> /* Test range sizes */
>> StreamFormats range({
>> - { PixelFormat(1), { SizeRange(640, 480, 640, 480) } },
>> - { PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },
>> - { PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },
>> - { PixelFormat(4), { SizeRange(128, 128, 4096, 4096, 128, 128) } },
>> + { PixelFormat(1), { SizeRange({ 640, 480 }, { 640, 480 }) } },
>> + { PixelFormat(2), { SizeRange({ 640, 480 }, { 800, 600 }, 8, 8) } },
>> + { PixelFormat(3), { SizeRange({ 640, 480 }, { 800, 600 }, 16, 16) } },
>> + { PixelFormat(4), { SizeRange({ 128, 128 }, { 4096, 4096 }, 128, 128) } },
>> });
>>
>> if (testSizes("range 1", range.sizes(PixelFormat(1)), { Size(640, 480) }))
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list