[libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor: Retrieve sensor sizes
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Aug 29 16:27:22 CEST 2019
Hi Jacopo,
On 2019-08-29 11:07:36 +0200, Jacopo Mondi wrote:
> 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...
Absolutely, no worries. Sorry if that was not clear in my previous mail
that I do not feel strongly about it in regard to this patch.
>
> 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 ?
I'm divided, it's good to have checks but I also thinks the kernel
should report the correct thing. And if not and things so south it
should be fixed in the kernel.
>
> 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
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list