[libcamera-devel] [PATCH v3 3/6] libcamera: camera_sensor: Default selection targets
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 30 21:13:26 CET 2020
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
?
> + << 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.
> 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
More information about the libcamera-devel
mailing list