[libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor: Retrieve sensor sizes
Jacopo Mondi
jacopo at jmondi.org
Thu Aug 29 11:07:36 CEST 2019
Hi Niklas,
On Thu, Aug 29, 2019 at 10:49:45AM +0200, Niklas Söderlund wrote:
> 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 :-)
>
So I'm glad we have demonstrated we're both right here (or both wrong)
as by definition bikeshedding discussions are made to disappoint all
the involved parties.
For bigger patches I agree, but please let me keep this one the way it
is...
One less metaphysical question below..
> >
> > > >
> > > > 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;
I hoped for a comment here, as what's here happening is that we're
validating the kernel driver reported parameters. Is this something
we want to do or should we assume the kernel side is, as we all know
very well, perfect ?
Thanks
j
> > > > +
> > > > /* 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190829/20d1f810/attachment.sig>
More information about the libcamera-devel
mailing list