[libcamera-devel] [PATCH v2 7/7] libcamera: camera_sensor: Retrieve sensor sizes

[Jacopo Mondi]

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

> >
> > 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
-------------- 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/20190828/2a5d6a5c/attachment.sig>