[libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor: Default selection targets

[Jacopo Mondi]

Hi Laurent,

On Wed, Dec 30, 2020 at 10:13:26PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Dec 30, 2020 at 07:01:17PM +0100, Jacopo Mondi wrote:
> > Support for the V4L2 selection API is optional in the CameraSensor
> > class. Currently the properties registred by using values read
>
> s/registred/registered/
>
> > through that API are defaulted in several places (the Android camera HAL
> > or the CameraSensor class).
> >
> > In future support for the selection API will be made mandatory, but to
>
> s/In future/In the future/
>
> > give time to sensor drivers in all test platforms to be updated, provide
> > a default for each of those properties, simplifying the creation of
> > the camera sensor properties list and of CameraSensorInfo.
> >
> > Provide a default size of [2592x2944] for the pixel array size and a
> > default rectangle [16, 12, 2560, 1920] for the active area and analog
> > crop rectangles.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  3 +
> >  src/libcamera/camera_sensor.cpp            | 71 +++++++++-------------
> >  2 files changed, 31 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index aee10aa6e3c7..c2d620f05b65 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -84,6 +84,9 @@ private:
> >  	std::vector<unsigned int> mbusCodes_;
> >  	std::vector<Size> sizes_;
> >
> > +	Size pixelArraySize_;
> > +	Rectangle activeAreaSize_;
>
> It's not a size but a rectangle, I'd call this activeArea_.
>
> > +
> >  	ControlList properties_;
> >  };
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index a1f1256bd6f4..2ce19a40b448 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -31,6 +31,9 @@ namespace libcamera {
> >
> >  LOG_DEFINE_CATEGORY(CameraSensor)
> >
> > +static constexpr Size defaultPixelArraySize = { 2592, 1944 };
> > +static constexpr Rectangle defaultActiveAreaSize = { 16, 12, 2560, 1920 };
> > +
> >  /**
> >   * \struct CameraSensorInfo
> >   * \brief Report the image sensor characteristics
> > @@ -266,25 +269,35 @@ int CameraSensor::validateSensorDriver()
> >  	 * but some properties and features, like constructing a
> >  	 * CameraSensorInfo for the IPA module, won't be supported.
> >  	 */
> > +
> >  	Rectangle rect;
> >  	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect);
> >  	if (ret) {
> >  		LOG(CameraSensor, Info)
> > -			<< "Failed to retrieve the readable pixel area size";
> > +			<< "The PixelArraySize property has been defaulted to: "
>
> s/to:/to/ (and same below). But do we actually need this change ? We
> plan to revert it soon, isn't it better to keep talking about a failure
> ?

I would rather tell what we default to than repeating the "Failed.."
as we get the V4L2Subdevice error message already

>
> > +			<< defaultPixelArraySize.toString();
> > +		rect.width = defaultPixelArraySize.width;
> > +		rect.height = defaultPixelArraySize.height;
> >  		err = -EINVAL;
> >  	}
> > +	pixelArraySize_.width = rect.width;
> > +	pixelArraySize_.height = rect.height;
> >
> >  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
>
> s/rect/activeAreaSize_/
>
> >  	if (ret) {
> >  		LOG(CameraSensor, Info)
> > -			<< "Failed to retrieve the active pixel area size";
> > +			<< "The PixelArraySize property has been defaulted to: "
> > +			<< defaultActiveAreaSize.toString();
> > +		rect = defaultActiveAreaSize;
>
> 		activeAreaSize_ = defaultActiveAreaSize;
>
> >  		err = -EINVAL;
> >  	}
> > +	activeAreaSize_ = rect;
>
> And drop this line.
>
> >
> >  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> >  	if (ret) {
> >  		LOG(CameraSensor, Info)
> > -			<< "Failed to retreive the sensor crop rectangle";
>
> If this has been introduced in a previous patch in the series,
> s/retreive/retrieve/.
>
> > +			<< "The analog crop rectangle has been defaulted to: "
> > +			<< defaultActiveAreaSize.toString();
>
> This looks messy as there's no handling of the default here, only down
> below.
>

I'll drop the default, but I will have to add it to the
CameraSensorInfo creation, as I would like to tell the crop
rectangle has been defaulted.

> >  		err = -EINVAL;
> >  	}
> >
> > @@ -378,30 +391,8 @@ int CameraSensor::initProperties()
> >  	}
> >  	properties_.set(properties::Rotation, propertyValue);
> >
> > -	Rectangle bounds;
> > -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &bounds);
> > -	if (!ret)
> > -		properties_.set(properties::PixelArraySize, bounds.size());
> > -	else
> > -		LOG(CameraSensor, Debug)
> > -			<< "PixelArraySize property not registered";
> > -
> > -	Rectangle crop;
> > -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &crop);
> > -	if (!ret) {
> > -		/*
> > -		 * V4L2_SEL_TGT_CROP_DEFAULT and V4L2_SEL_TGT_CROP_BOUNDS are
> > -		 * defined relatively to the sensor full pixel array size,
> > -		 * while properties::PixelArrayActiveAreas is defined relatively
> > -		 * to properties::PixelArraySize. Adjust it.
> > -		 */
> > -		crop.x -= bounds.x;
> > -		crop.y -= bounds.y;
> > -		properties_.set(properties::PixelArrayActiveAreas, { crop });
> > -	} else {
> > -		LOG(CameraSensor, Debug)
> > -			<< "PixelArrayActiveAreas property not registered";
> > -	}
> > +	properties_.set(properties::PixelArraySize, pixelArraySize_);
> > +	properties_.set(properties::PixelArrayActiveAreas, { activeAreaSize_ });
>
> This is nice. Caching those two values is in my opinion the main feature
> of this patch, it's worth mentioning it in the commit message.
>
> >
> >  	/* Color filter array pattern, register only for RAW sensors. */
> >  	for (const auto &format : formats_) {
> > @@ -657,21 +648,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >  {
> >  	info->model = model();
> >
> > -	/* Get the active area size. */
> > -	Rectangle rect;
> > -	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &rect);
> > -	if (ret) {
> > -		LOG(CameraSensor, Error) << "Failed to get the active area";
> > -		return ret;
> > -	}
> > -	info->activeAreaSize = { rect.width, rect.height };
> > +	/*
> > +	 * The active area size is a static property, while the crop
> > +	 * rectangle needs to be re-read as it changes per-mode.
>
> s/changes per-mode/depends on the sensor configuration/
>
> I'd like to avoid talking about sensor modes everywhere, a sensor mode
> is an artificial limitation that we have to live with but that we should
> still push against as much as possible. It shouldn't become a core
> concept in the libcamera vocabulary.
>
> > +	 */
> > +	info->activeAreaSize = { activeAreaSize_.width, activeAreaSize_.height };
> >
> > -	/* It's mandatory for the subdevice to report its crop rectangle. */
> > -	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > -	if (ret) {
> > -		LOG(CameraSensor, Error) << "Failed to get the crop rectangle";
> > -		return ret;
> > -	}
> > +	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > +	if (ret)
> > +		info->analogCrop = defaultActiveAreaSize;
>
> As this already causes an error, and we want to turn it into a fatal
> error soon, how about dropping this change and the corresponding one in
> CameraSensor::validateSensorDriver() ?
>
> >
> >  	/*
> >  	 * CameraSensorInfo::analogCrop::x and CameraSensorInfo::analogCrop::y
> > @@ -680,8 +665,8 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >  	 *
> >  	 * Compensate it by subtracting the active areas offset.
>
> While at it you could write s/areas/area/ (s/areas/area's/ ?).
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> >  	 */
> > -	info->analogCrop.x -= rect.x;
> > -	info->analogCrop.y -= rect.y;
> > +	info->analogCrop.x -= activeAreaSize_.x;
> > +	info->analogCrop.y -= activeAreaSize_.y;
> >
> >  	/* The bit depth and image size depend on the currently applied format. */
> >  	V4L2SubdeviceFormat format{};
>
> --
> Regards,
>
> Laurent Pinchart