| Message ID | 20201230180120.78407-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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{};
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@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@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
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_; + 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: " + << 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); if (ret) { LOG(CameraSensor, Info) - << "Failed to retrieve the active pixel area size"; + << "The PixelArraySize property has been defaulted to: " + << defaultActiveAreaSize.toString(); + rect = defaultActiveAreaSize; err = -EINVAL; } + activeAreaSize_ = rect; ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect); if (ret) { LOG(CameraSensor, Info) - << "Failed to retreive the sensor crop rectangle"; + << "The analog crop rectangle has been defaulted to: " + << defaultActiveAreaSize.toString(); 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_ }); /* 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. + */ + 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; /* * 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. */ - 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{};
Support for the V4L2 selection API is optional in the CameraSensor class. Currently the properties registred by using values read 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 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@jmondi.org> --- include/libcamera/internal/camera_sensor.h | 3 + src/libcamera/camera_sensor.cpp | 71 +++++++++------------- 2 files changed, 31 insertions(+), 43 deletions(-)