[libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor: Retrieve sensor sizes
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Aug 29 10:49:45 CEST 2019
Hi Jacopo,
On 2019-08-28 18:34:06 +0200, Jacopo Mondi wrote:
> Hi Niklas,
>
> On Wed, Aug 28, 2019 at 01:48:10PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2019-08-27 11:50:07 +0200, Jacopo Mondi wrote:
> > > Retrieve the camera sensor pixel array sizes and the active pixel area
> > > using the V4L2Subdevice selection API.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > include/libcamera/geometry.h | 1 +
> > > src/libcamera/camera_sensor.cpp | 51 +++++++++++++++++++++++++++
> > > src/libcamera/geometry.cpp | 24 +++++++++++++
> > > src/libcamera/include/camera_sensor.h | 4 +++
> > > 4 files changed, 80 insertions(+)
> >
> > I think you should split the changes to geometry.{cpp,h} into its own
> > patch.
> >
> Mmmm, I know we stand on two opposite sides here... the change is
> rather trivial and it makes sense to me to show its usage in the same
> patch to avoid people jumping from one patch to another during review.
> I could split if this bothers you, but it's not worth it in my opinion
I agree this is a fight you and I will enjoy for many years I hope,
until I win in the end of course ;-P I don't feel strongly about it so
don't spend energy on it if you don't want to for trivial patches. For
mid to large size patches I feel very strongly on this topic tho.
I know you feel it's easier to review larger patches and not "jump"
between multiple ones. For me it's the opposite, I spend 4 times longer
reviewing a patch like this because my warning light go off asking why
the patch do two or more things. I try to find the reason for this but
can't, and once I realise the patch do more then one thing without it
strictly needing to I comment on it to release my frustration ;-)
I think this is some sort of meta-bikesheeding discussion, not sure if
that is a good thing or not :-)
>
> > >
> > > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > > index 52f4d010de76..b91fcfa1dd17 100644
> > > --- a/include/libcamera/geometry.h
> > > +++ b/include/libcamera/geometry.h
> > > @@ -42,6 +42,7 @@ struct Size {
> > > unsigned int height;
> > >
> > > const std::string toString() const;
> > > + bool contains(const Rectangle &rectangle) const;
> > > };
> > >
> > > bool operator==(const Size &lhs, const Size &rhs);
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index fc7fdcdcaf5b..b6ccaae0930b 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -128,6 +128,28 @@ int CameraSensor::init()
> > > }
> > > rotation_ = value;
> > >
> > > + /*
> > > + * Retrieve and store the sensor pixel array size and active area
> > > + * rectangle.
> > > + */
> > > + Rectangle rect;
> > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_NATIVE_SIZE, &rect);
> > > + if (ret)
> > > + return ret;
> > > + pixelArray_.width = rect.w;
> > > + pixelArray_.height = rect.h;
> > > +
> > > + ret = subdev_->getSelection(0, V4L2_SEL_TGT_CROP_BOUNDS, &rect);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!pixelArray_.contains(rect)) {
> > > + LOG(CameraSensor, Error)
> > > + << "Invalid camera sensor pixel array dimension";
> > > + return -EINVAL;
> > > + }
> > > + activeArea_ = rect;
> > > +
> > > /* Enumerate and cache media bus codes and sizes. */
> > > const ImageFormats formats = subdev_->formats(0);
> > > if (formats.isEmpty()) {
> > > @@ -192,6 +214,35 @@ const Size &CameraSensor::resolution() const
> > > return sizes_.back();
> > > }
> > >
> > > +/**
> > > + * \fn CameraSensor::pixelArray()
> > > + * \brief Retrieve the camera sensor pixel array dimension
> > > + * \return The number of horizontal and vertical pixel units installed on the
> > > + * camera sensor
> > > + *
> > > + * The returned pixel array size is the number of pixels units physically
> > > + * installed on the sensor, including non-active ones, such as pixels used
> > > + * for calibration or testing.
> > > + */
> > > +
> > > +/**
> > > + * \fn CameraSensor::activeArea()
> > > + * \brief Retrieve the active pixels area
> > > + * \return A rectangle describing the active pixel area
> > > + *
> > > + * The returned rectangle is contained in the larger pixel array area,
> > > + * returned by the CameraSensor::pixelArray() operation.
> > > + *
> > > + * \todo This operation collides with CameraSensor::resolution() as they
> > > + * both reports the same information (bugs in the driver code apart).
> > > + * Also, for some sensors, the maximum available resolution might depend on
> > > + * the image size ratio (ie the maximumx resolution for images in 4:3 format
> > > + * is different from the maximum available resolution for 16:9 format). This
> > > + * has to be sorted out as well.
> >
> > Should we remove CameraSensor::resolution() in favor if new more
> > "correct" way of finding out the sensor resolution?
> >
>
> Good question.. another option would be to get the maximum resolution
> in regard to a specific aspect ratio... I would say I should better
> look into the current usages of this operation...
>
> > > + *
> > > + * \sa Size::contains()
> > > + */
> > > +
> > > /**
> > > * \brief Retrieve the best sensor format for a desired output
> > > * \param[in] mbusCodes The list of acceptable media bus codes
> > > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > > index 32b0faeadc63..9b60768a57a7 100644
> > > --- a/src/libcamera/geometry.cpp
> > > +++ b/src/libcamera/geometry.cpp
> > > @@ -116,6 +116,30 @@ const std::string Size::toString() const
> > > return std::to_string(width) + "x" + std::to_string(height);
> > > }
> > >
> > > +/**
> > > + * \fn Size::contains()
> > > + * \param rectangle The rectangle to verify
> > > + * \return True if \a rectangle is contained in the area represented by this
> > > + * Size, false otherwise
> >
> > Return comes at the end of the documentation block.
> >
>
> True, thanks for spotting
>
> > > + *
> > > + * Return true if \a rectangle is completely contained in the area represented
> > > + * by the width and height of this Size, with its top left corner assumed to be
> > > + * at coordinates (0, 0). The rectangle 'x' and 'y' fields represent the
> > > + * horizontal and vertical positive displacement from the Size top left corner,
> > > + * from where the rectangle's horizontal and vertical sizes are calculated.
> > > + */
> > > +bool Size::contains(const Rectangle &rectangle) const
> > > +{
> > > + if (rectangle.x < 0 || rectangle.y < 0)
> > > + return false;
> > > +
> > > + if (rectangle.x + rectangle.w > width ||
> > > + rectangle.y + rectangle.h > height)
> > > + return false;
> > > +
> > > + return true;
> > > +}
> > > +
> > > /**
> > > * \brief Compare sizes for equality
> > > * \return True if the two sizes are equal, false otherwise
> > > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h
> > > index 32d39127b275..abf5344cf9d8 100644
> > > --- a/src/libcamera/include/camera_sensor.h
> > > +++ b/src/libcamera/include/camera_sensor.h
> > > @@ -38,6 +38,8 @@ public:
> > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > > const std::vector<Size> &sizes() const { return sizes_; }
> > > const Size &resolution() const;
> > > + const Size pixelArray() const { return pixelArray_; }
> > > + const Rectangle activeArea() const { return activeArea_; }
> > >
> > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > const Size &size) const;
> > > @@ -59,6 +61,8 @@ private:
> > >
> > > CameraLocation location_;
> > > unsigned int rotation_;
> > > + Size pixelArray_;
> > > + Rectangle activeArea_;
> > > };
> > >
> > > } /* namespace libcamera */
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list