[libcamera-devel] [PATCH 3/5] libcamera: geometry: Give constructors to Rectangle
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 15 11:31:46 CEST 2020
Hi Niklas,
On Wed, Jul 15, 2020 at 08:33:45AM +0200, Niklas Söderlund wrote:
> On 2020-07-15 02:40:07 +0300, Laurent Pinchart wrote:
> > Rectangle, unlike Size, has no constructor, requiring the users to
> > explicitly initialize the instances. This is error-prone, add
> > constructors.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/geometry.h | 15 +++++++++++++
> > src/libcamera/camera_sensor.cpp | 2 +-
> > src/libcamera/geometry.cpp | 22 +++++++++++++++++++
> > src/libcamera/pipeline/ipu3/imgu.cpp | 7 +-----
> > .../pipeline/raspberrypi/raspberrypi.cpp | 7 +-----
> > src/libcamera/pipeline/vimc/vimc.cpp | 7 +-----
> > 6 files changed, 41 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index f3088d33fa2c..380248ac9a50 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -127,6 +127,21 @@ static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
> > }
> >
> > struct Rectangle {
>
> I wonder if we should not turn Size and Rectangle into classes instead
> of structs when we start to add methods to them. Or is there an
> advantage to keep them as structs?
See the rest of the series :-)
> > + Rectangle()
> > + : Rectangle(0, 0, 0, 0)
> > + {
> > + }
> > +
> > + Rectangle(int xpos, int ypos, const Size &size)
> > + : x(xpos), y(ypos), width(size.width), height(size.height)
> > + {
> > + }
> > +
> > + Rectangle(int xpos, int ypos, unsigned int w, unsigned int h)
> > + : x(xpos), y(ypos), width(w), height(h)
> > + {
> > + }
> > +
> > int x;
> > int y;
> > unsigned int width;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index b14b4051dca6..6e93cc51155b 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -487,7 +487,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > info->model = model();
> >
> > /* Get the active area size. */
> > - Rectangle rect = {};
> > + Rectangle rect;
> > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> > if (ret) {
> > LOG(CameraSensor, Error)
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index 5a0bcf7c1e12..3a3784df39e6 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -297,6 +297,28 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> > * refers to, are defined by the context were rectangle is used.
> > */
> >
> > +/**
> > + * \fn Rectangle::Rectangle()
> > + * \brief Construct a Rectangle with all coordinates set to 0
> > + */
> > +
> > +/**
> > + * \fn Rectangle::Rectangle(int x, int y, const Size &size)
> > + * \brief Construct a Rectangle with the given position and size
> > + * \param[in] x The horizontal coordinate of the top-left corner
> > + * \param[in] y The vertical coordinate of the top-left corner
> > + * \param[in] size The size
> > + */
> > +
> > +/**
> > + * \fn Rectangle::Rectangle(int x, int y, unsigned int width, unsigned int height)
> > + * \brief Construct a Rectangle with the given position and size
> > + * \param[in] x The horizontal coordinate of the top-left corner
> > + * \param[in] y The vertical coordinate of the top-left corner
> > + * \param[in] width The width
> > + * \param[in] height The height
> > + */
> > +
> > /**
> > * \var Rectangle::x
> > * \brief The horizontal coordinate of the rectangle's top-left corner
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index d7f4173d3607..4bec4b2f1a76 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -100,12 +100,7 @@ int ImgUDevice::configureInput(const Size &size,
> > * to configure the crop/compose rectangles, contradicting the
> > * V4L2 specification.
> > */
> > - Rectangle rect = {
> > - .x = 0,
> > - .y = 0,
> > - .width = inputFormat->size.width,
> > - .height = inputFormat->size.height,
> > - };
> > + Rectangle rect{ 0, 0, inputFormat->size };
>
> Out out curiosity why do you use list initialization ?
>
> Questions for my education apart,
Mostly because the code was already using curly braces :-)
There are a few important differences between
Rectangle rect{ 0, 0, inputFormat->size };
and
Rectangle rect(0, 0, inputFormat->size);
One is that the former will not narrow arguments through implicit cast.
See https://en.cppreference.com/w/cpp/language/list_initialization:
"[...] all constructors of T participate in overload resolution against
the set of arguments that consists of the elements of the
braced-init-list, with the restriction that only non-narrowing
conversions are allowed."
It's commonly considered as a good practice as the default
initialization mechanism. There are however drawbacks,
https://stackoverflow.com/questions/18222926/why-is-list-initialization-using-curly-braces-better-than-the-alternatives
explains some of them.
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> > ret = imgu_->setSelection(PAD_INPUT, V4L2_SEL_TGT_CROP, &rect);
> > if (ret)
> > return ret;
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 009e502b288c..2d848ac3c735 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -752,12 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> > }
> >
> > /* Adjust aspect ratio by providing crops on the input image. */
> > - Rectangle crop = {
> > - .x = 0,
> > - .y = 0,
> > - .width = sensorFormat.size.width,
> > - .height = sensorFormat.size.height
> > - };
> > + Rectangle crop{ 0, 0, sensorFormat.size };
> >
> > int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;
> > if (ar > 0)
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index a6f457c71fb1..4f461b928514 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -256,12 +256,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> > return ret;
> >
> > if (data->media_->version() >= KERNEL_VERSION(5, 6, 0)) {
> > - Rectangle crop = {
> > - .x = 0,
> > - .y = 0,
> > - .width = subformat.size.width,
> > - .height = subformat.size.height,
> > - };
> > + Rectangle crop{ 0, 0, subformat.size };
> > ret = data->scaler_->setSelection(0, V4L2_SEL_TGT_CROP, &crop);
> > if (ret)
> > return ret;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list