| Message ID | 20210302194948.20396-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2021-03-02 21:49:48 +0200, Laurent Pinchart wrote: > When a sensor can upscale the image, the native sensor resolution isn't > equal to the largest size reported by the sensor. Use the active area > size instead, and default it to the largest enumerated size if the crop > rectangle targets are not supported. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > Changes since v1: > > - Don't set default pixelArraySize_ to override it later > > Note how this causes the pixelArraySize_ = sizes_.back() assignment to > be duplicated in CameraSensor::validateSensorDriver() and > CameraSensor::initVimcDefaultProperties(). Jacopo, do you prefer v1 or > v2 ? > > --- > include/libcamera/internal/camera_sensor.h | 3 +-- > src/libcamera/camera_sensor.cpp | 23 +++++++++++----------- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index f22ffbfe9f97..71d012f795fe 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -53,7 +53,7 @@ public: > const MediaEntity *entity() const { return entity_; } > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > const std::vector<Size> &sizes() const { return sizes_; } > - const Size &resolution() const { return resolution_; } > + Size resolution() const { return activeArea_.size(); } > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > const Size &size) const; > @@ -87,7 +87,6 @@ private: > std::string id_; > > V4L2Subdevice::Formats formats_; > - Size resolution_; > std::vector<unsigned int> mbusCodes_; > std::vector<Size> sizes_; > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 8a1b9bd277df..8db6e8974a8d 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -233,12 +233,6 @@ int CameraSensor::init() > auto last = std::unique(sizes_.begin(), sizes_.end()); > sizes_.erase(last, sizes_.end()); > > - /* > - * The sizes_ vector is sorted in ascending order, the resolution is > - * thus the last element of the vector. > - */ > - resolution_ = sizes_.back(); > - > /* > * VIMC is a bit special, as it does not yet support all the mandatory > * requirements regular sensors have to respect. > @@ -324,14 +318,20 @@ int CameraSensor::validateSensorDriver() > Rectangle rect; > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > if (ret) { > - rect = Rectangle(resolution()); > + /* > + * Default the pixel array size to the largest size supported > + * by the sensor. The sizes_ vector is sorted in ascending > + * order, the largest size is thus the last element. > + */ > + pixelArraySize_ = sizes_.back(); > + > LOG(CameraSensor, Warning) > << "The PixelArraySize property has been defaulted to " > - << rect.toString(); > + << pixelArraySize_.toString(); > err = -EINVAL; > + } else { > + pixelArraySize_ = rect.size(); > } > - pixelArraySize_.width = rect.width; > - pixelArraySize_.height = rect.height; > > ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_); > if (ret) { > @@ -397,7 +397,8 @@ int CameraSensor::validateSensorDriver() > */ > void CameraSensor::initVimcDefaultProperties() > { > - pixelArraySize_ = resolution(); > + /* Use the largest supported size. */ > + pixelArraySize_ = sizes_.back(); > activeArea_ = Rectangle(pixelArraySize_); > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On Tue, Mar 02, 2021 at 09:49:48PM +0200, Laurent Pinchart wrote: > When a sensor can upscale the image, the native sensor resolution isn't > equal to the largest size reported by the sensor. Use the active area > size instead, and default it to the largest enumerated size if the crop > rectangle targets are not supported. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Changes since v1: > > - Don't set default pixelArraySize_ to override it later > > Note how this causes the pixelArraySize_ = sizes_.back() assignment to > be duplicated in CameraSensor::validateSensorDriver() and > CameraSensor::initVimcDefaultProperties(). Jacopo, do you prefer v1 or > v2 ? I think I slightly prefer this one as the two functions are in mutually exclusive call paths and it's easier to follow compared to an initialization followed by an over-write. Anyway, we're talking details here. > > --- > include/libcamera/internal/camera_sensor.h | 3 +-- > src/libcamera/camera_sensor.cpp | 23 +++++++++++----------- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index f22ffbfe9f97..71d012f795fe 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -53,7 +53,7 @@ public: > const MediaEntity *entity() const { return entity_; } > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > const std::vector<Size> &sizes() const { return sizes_; } > - const Size &resolution() const { return resolution_; } > + Size resolution() const { return activeArea_.size(); } > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > const Size &size) const; > @@ -87,7 +87,6 @@ private: > std::string id_; > > V4L2Subdevice::Formats formats_; > - Size resolution_; > std::vector<unsigned int> mbusCodes_; > std::vector<Size> sizes_; > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 8a1b9bd277df..8db6e8974a8d 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -233,12 +233,6 @@ int CameraSensor::init() > auto last = std::unique(sizes_.begin(), sizes_.end()); > sizes_.erase(last, sizes_.end()); > > - /* > - * The sizes_ vector is sorted in ascending order, the resolution is > - * thus the last element of the vector. > - */ > - resolution_ = sizes_.back(); > - > /* > * VIMC is a bit special, as it does not yet support all the mandatory > * requirements regular sensors have to respect. > @@ -324,14 +318,20 @@ int CameraSensor::validateSensorDriver() > Rectangle rect; > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > if (ret) { > - rect = Rectangle(resolution()); > + /* > + * Default the pixel array size to the largest size supported > + * by the sensor. The sizes_ vector is sorted in ascending > + * order, the largest size is thus the last element. > + */ > + pixelArraySize_ = sizes_.back(); > + > LOG(CameraSensor, Warning) > << "The PixelArraySize property has been defaulted to " > - << rect.toString(); > + << pixelArraySize_.toString(); > err = -EINVAL; > + } else { > + pixelArraySize_ = rect.size(); Can't you if (ret) { rect = Rectangle(sizes_.back()) .... } pixelArraySize = rect.size(); Anyway, that's minor too. The patch looks good. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> I recall you had a patch that inspected the list of the V4L2Subdevice control info map instead of gong to the device to check if a control was supported (iirc that help avoiding an error message being printed out by subdev_->getControls() in CameraSensor::sensorInfo()). Wasn't it paired with this one ? > } > - pixelArraySize_.width = rect.width; > - pixelArraySize_.height = rect.height; > > ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_); > if (ret) { > @@ -397,7 +397,8 @@ int CameraSensor::validateSensorDriver() > */ > void CameraSensor::initVimcDefaultProperties() > { > - pixelArraySize_ = resolution(); > + /* Use the largest supported size. */ > + pixelArraySize_ = sizes_.back(); > activeArea_ = Rectangle(pixelArraySize_); > } > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Wed, Mar 03, 2021 at 02:09:54PM +0100, Jacopo Mondi wrote: > On Tue, Mar 02, 2021 at 09:49:48PM +0200, Laurent Pinchart wrote: > > When a sensor can upscale the image, the native sensor resolution isn't > > equal to the largest size reported by the sensor. Use the active area > > size instead, and default it to the largest enumerated size if the crop > > rectangle targets are not supported. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v1: > > > > - Don't set default pixelArraySize_ to override it later > > > > Note how this causes the pixelArraySize_ = sizes_.back() assignment to > > be duplicated in CameraSensor::validateSensorDriver() and > > CameraSensor::initVimcDefaultProperties(). Jacopo, do you prefer v1 or > > v2 ? > > I think I slightly prefer this one as the two functions are in > mutually exclusive call paths and it's easier to follow compared to an > initialization followed by an over-write. Works for me. > Anyway, we're talking details here. > > > --- > > include/libcamera/internal/camera_sensor.h | 3 +-- > > src/libcamera/camera_sensor.cpp | 23 +++++++++++----------- > > 2 files changed, 13 insertions(+), 13 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index f22ffbfe9f97..71d012f795fe 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -53,7 +53,7 @@ public: > > const MediaEntity *entity() const { return entity_; } > > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } > > const std::vector<Size> &sizes() const { return sizes_; } > > - const Size &resolution() const { return resolution_; } > > + Size resolution() const { return activeArea_.size(); } > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > const Size &size) const; > > @@ -87,7 +87,6 @@ private: > > std::string id_; > > > > V4L2Subdevice::Formats formats_; > > - Size resolution_; > > std::vector<unsigned int> mbusCodes_; > > std::vector<Size> sizes_; > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 8a1b9bd277df..8db6e8974a8d 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -233,12 +233,6 @@ int CameraSensor::init() > > auto last = std::unique(sizes_.begin(), sizes_.end()); > > sizes_.erase(last, sizes_.end()); > > > > - /* > > - * The sizes_ vector is sorted in ascending order, the resolution is > > - * thus the last element of the vector. > > - */ > > - resolution_ = sizes_.back(); > > - > > /* > > * VIMC is a bit special, as it does not yet support all the mandatory > > * requirements regular sensors have to respect. > > @@ -324,14 +318,20 @@ int CameraSensor::validateSensorDriver() > > Rectangle rect; > > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > if (ret) { > > - rect = Rectangle(resolution()); > > + /* > > + * Default the pixel array size to the largest size supported > > + * by the sensor. The sizes_ vector is sorted in ascending > > + * order, the largest size is thus the last element. > > + */ > > + pixelArraySize_ = sizes_.back(); > > + > > LOG(CameraSensor, Warning) > > << "The PixelArraySize property has been defaulted to " > > - << rect.toString(); > > + << pixelArraySize_.toString(); > > err = -EINVAL; > > + } else { > > + pixelArraySize_ = rect.size(); > > Can't you > if (ret) { > rect = Rectangle(sizes_.back()) > .... > } > > pixelArraySize = rect.size(); > > Anyway, that's minor too. That would convert from size to rectangle and then to size again :-) It's minor indeed. > The patch looks good. > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > I recall you had a patch that inspected the list of the V4L2Subdevice > control info map instead of gong to the device to check if a control > was supported (iirc that help avoiding an error message being printed > out by subdev_->getControls() in CameraSensor::sensorInfo()). Wasn't > it paired with this one ? That was part of commit 79266225d2af ("libcamera: camera_sensor: Check control availability from idmap"), merged in master. > > } > > - pixelArraySize_.width = rect.width; > > - pixelArraySize_.height = rect.height; > > > > ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_); > > if (ret) { > > @@ -397,7 +397,8 @@ int CameraSensor::validateSensorDriver() > > */ > > void CameraSensor::initVimcDefaultProperties() > > { > > - pixelArraySize_ = resolution(); > > + /* Use the largest supported size. */ > > + pixelArraySize_ = sizes_.back(); > > activeArea_ = Rectangle(pixelArraySize_); > > } > >
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index f22ffbfe9f97..71d012f795fe 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -53,7 +53,7 @@ public: const MediaEntity *entity() const { return entity_; } const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } const std::vector<Size> &sizes() const { return sizes_; } - const Size &resolution() const { return resolution_; } + Size resolution() const { return activeArea_.size(); } V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, const Size &size) const; @@ -87,7 +87,6 @@ private: std::string id_; V4L2Subdevice::Formats formats_; - Size resolution_; std::vector<unsigned int> mbusCodes_; std::vector<Size> sizes_; diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 8a1b9bd277df..8db6e8974a8d 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -233,12 +233,6 @@ int CameraSensor::init() auto last = std::unique(sizes_.begin(), sizes_.end()); sizes_.erase(last, sizes_.end()); - /* - * The sizes_ vector is sorted in ascending order, the resolution is - * thus the last element of the vector. - */ - resolution_ = sizes_.back(); - /* * VIMC is a bit special, as it does not yet support all the mandatory * requirements regular sensors have to respect. @@ -324,14 +318,20 @@ int CameraSensor::validateSensorDriver() Rectangle rect; int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); if (ret) { - rect = Rectangle(resolution()); + /* + * Default the pixel array size to the largest size supported + * by the sensor. The sizes_ vector is sorted in ascending + * order, the largest size is thus the last element. + */ + pixelArraySize_ = sizes_.back(); + LOG(CameraSensor, Warning) << "The PixelArraySize property has been defaulted to " - << rect.toString(); + << pixelArraySize_.toString(); err = -EINVAL; + } else { + pixelArraySize_ = rect.size(); } - pixelArraySize_.width = rect.width; - pixelArraySize_.height = rect.height; ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_DEFAULT, &activeArea_); if (ret) { @@ -397,7 +397,8 @@ int CameraSensor::validateSensorDriver() */ void CameraSensor::initVimcDefaultProperties() { - pixelArraySize_ = resolution(); + /* Use the largest supported size. */ + pixelArraySize_ = sizes_.back(); activeArea_ = Rectangle(pixelArraySize_); }
When a sensor can upscale the image, the native sensor resolution isn't equal to the largest size reported by the sensor. Use the active area size instead, and default it to the largest enumerated size if the crop rectangle targets are not supported. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Changes since v1: - Don't set default pixelArraySize_ to override it later Note how this causes the pixelArraySize_ = sizes_.back() assignment to be duplicated in CameraSensor::validateSensorDriver() and CameraSensor::initVimcDefaultProperties(). Jacopo, do you prefer v1 or v2 ? --- include/libcamera/internal/camera_sensor.h | 3 +-- src/libcamera/camera_sensor.cpp | 23 +++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-)