| Message ID | 20210208013805.30842-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Feb 08, 2021 at 03:38:05AM +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 Not sure I got this one. If the sensor can upscale, wouldn't the largest produced size be enumerated through the canonical ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ? Thanks j > 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> > --- > include/libcamera/internal/camera_sensor.h | 3 +-- > src/libcamera/camera_sensor.cpp | 16 ++++++++-------- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index c8f81882a958..74c35d1c8922 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -55,7 +55,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 c9e8d49b7935..59834ffcdd94 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -234,10 +234,12 @@ int CameraSensor::init() > sizes_.erase(last, sizes_.end()); > > /* > - * The sizes_ vector is sorted in ascending order, the resolution is > - * thus the last element of the vector. > + * Initialize the pixel array size as the largest size supported by the > + * sensor. It will be overridden later using the crop bounds. The > + * sizes_ vector is sorted in ascending order, the largest size is thus > + * the last element of the vector. > */ > - resolution_ = sizes_.back(); > + pixelArraySize_ = sizes_.back(); > > /* > * VIMC is a bit special, as it does not yet support all the mandatory > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver() > Rectangle rect; > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > if (ret) { > - rect = Rectangle(resolution()); > 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) { > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver() > */ > void CameraSensor::initVimcDefaultProperties() > { > - pixelArraySize_ = resolution(); > activeArea_ = Rectangle(pixelArraySize_); > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote: > On Mon, Feb 08, 2021 at 03:38:05AM +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 > > Not sure I got this one. If the sensor can upscale, wouldn't the > largest produced size be enumerated through the canonical > ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ? It would, that's the issue :-) ENUM_FRAME_SIZE would report the largest size the scaler can produce, which would be larger than the sensor's native resolution. > > 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> > > --- > > include/libcamera/internal/camera_sensor.h | 3 +-- > > src/libcamera/camera_sensor.cpp | 16 ++++++++-------- > > 2 files changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index c8f81882a958..74c35d1c8922 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -55,7 +55,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 c9e8d49b7935..59834ffcdd94 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -234,10 +234,12 @@ int CameraSensor::init() > > sizes_.erase(last, sizes_.end()); > > > > /* > > - * The sizes_ vector is sorted in ascending order, the resolution is > > - * thus the last element of the vector. > > + * Initialize the pixel array size as the largest size supported by the > > + * sensor. It will be overridden later using the crop bounds. The > > + * sizes_ vector is sorted in ascending order, the largest size is thus > > + * the last element of the vector. > > */ > > - resolution_ = sizes_.back(); > > + pixelArraySize_ = sizes_.back(); > > > > /* > > * VIMC is a bit special, as it does not yet support all the mandatory > > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver() > > Rectangle rect; > > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > if (ret) { > > - rect = Rectangle(resolution()); > > 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) { > > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver() > > */ > > void CameraSensor::initVimcDefaultProperties() > > { > > - pixelArraySize_ = resolution(); > > activeArea_ = Rectangle(pixelArraySize_); > > } > >
Hi Laurent, On Mon, Feb 08, 2021 at 05:17:32PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote: > > On Mon, Feb 08, 2021 at 03:38:05AM +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 > > > > Not sure I got this one. If the sensor can upscale, wouldn't the > > largest produced size be enumerated through the canonical > > ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ? > > It would, that's the issue :-) ENUM_FRAME_SIZE would report the largest > size the scaler can produce, which would be larger than the sensor's > native resolution. > Right, first email of the day, I was not fully awake probably :) > > > 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> > > > --- > > > include/libcamera/internal/camera_sensor.h | 3 +-- > > > src/libcamera/camera_sensor.cpp | 16 ++++++++-------- > > > 2 files changed, 9 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > index c8f81882a958..74c35d1c8922 100644 > > > --- a/include/libcamera/internal/camera_sensor.h > > > +++ b/include/libcamera/internal/camera_sensor.h > > > @@ -55,7 +55,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 c9e8d49b7935..59834ffcdd94 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -234,10 +234,12 @@ int CameraSensor::init() > > > sizes_.erase(last, sizes_.end()); > > > > > > /* > > > - * The sizes_ vector is sorted in ascending order, the resolution is > > > - * thus the last element of the vector. > > > + * Initialize the pixel array size as the largest size supported by the > > > + * sensor. It will be overridden later using the crop bounds. The > > > + * sizes_ vector is sorted in ascending order, the largest size is thus > > > + * the last element of the vector. > > > */ > > > - resolution_ = sizes_.back(); > > > + pixelArraySize_ = sizes_.back(); Having the pixelArraySize_ initialization here, to expect it will be updated later is a bit hard to follow. What if you initialize it to size_.back() if retrieving CROP_BOUNDS fails ? > > > > > > /* > > > * VIMC is a bit special, as it does not yet support all the mandatory > > > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver() > > > Rectangle rect; > > > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > > if (ret) { > > > - rect = Rectangle(resolution()); > > > 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) { > > > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver() > > > */ > > > void CameraSensor::initVimcDefaultProperties() > > > { > > > - pixelArraySize_ = resolution(); > > > activeArea_ = Rectangle(pixelArraySize_); > > > } > > > > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Feb 08, 2021 at 04:45:27PM +0100, Jacopo Mondi wrote: > On Mon, Feb 08, 2021 at 05:17:32PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 08, 2021 at 09:51:52AM +0100, Jacopo Mondi wrote: > > > On Mon, Feb 08, 2021 at 03:38:05AM +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 > > > > > > Not sure I got this one. If the sensor can upscale, wouldn't the > > > largest produced size be enumerated through the canonical > > > ENUM_FRAME_SIZE ioctl from which we build the sizes_ vector anyway ? > > > > It would, that's the issue :-) ENUM_FRAME_SIZE would report the largest > > size the scaler can produce, which would be larger than the sensor's > > native resolution. > > Right, first email of the day, I was not fully awake probably :) I'll try to do better in my last e-mail of today ;-) > > > > 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> > > > > --- > > > > include/libcamera/internal/camera_sensor.h | 3 +-- > > > > src/libcamera/camera_sensor.cpp | 16 ++++++++-------- > > > > 2 files changed, 9 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > index c8f81882a958..74c35d1c8922 100644 > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > @@ -55,7 +55,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 c9e8d49b7935..59834ffcdd94 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -234,10 +234,12 @@ int CameraSensor::init() > > > > sizes_.erase(last, sizes_.end()); > > > > > > > > /* > > > > - * The sizes_ vector is sorted in ascending order, the resolution is > > > > - * thus the last element of the vector. > > > > + * Initialize the pixel array size as the largest size supported by the > > > > + * sensor. It will be overridden later using the crop bounds. The > > > > + * sizes_ vector is sorted in ascending order, the largest size is thus > > > > + * the last element of the vector. > > > > */ > > > > - resolution_ = sizes_.back(); > > > > + pixelArraySize_ = sizes_.back(); > > Having the pixelArraySize_ initialization here, to expect it will be > updated later is a bit hard to follow. > > What if you initialize it to size_.back() if retrieving CROP_BOUNDS > fails ? I'll try that. > > > > > > > > /* > > > > * VIMC is a bit special, as it does not yet support all the mandatory > > > > @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver() > > > > Rectangle rect; > > > > int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); > > > > if (ret) { > > > > - rect = Rectangle(resolution()); > > > > 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) { > > > > @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver() > > > > */ > > > > void CameraSensor::initVimcDefaultProperties() > > > > { > > > > - pixelArraySize_ = resolution(); > > > > activeArea_ = Rectangle(pixelArraySize_); > > > > } > > > >
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index c8f81882a958..74c35d1c8922 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -55,7 +55,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 c9e8d49b7935..59834ffcdd94 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -234,10 +234,12 @@ int CameraSensor::init() sizes_.erase(last, sizes_.end()); /* - * The sizes_ vector is sorted in ascending order, the resolution is - * thus the last element of the vector. + * Initialize the pixel array size as the largest size supported by the + * sensor. It will be overridden later using the crop bounds. The + * sizes_ vector is sorted in ascending order, the largest size is thus + * the last element of the vector. */ - resolution_ = sizes_.back(); + pixelArraySize_ = sizes_.back(); /* * VIMC is a bit special, as it does not yet support all the mandatory @@ -307,14 +309,13 @@ int CameraSensor::validateSensorDriver() Rectangle rect; int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP_BOUNDS, &rect); if (ret) { - rect = Rectangle(resolution()); 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) { @@ -380,7 +381,6 @@ int CameraSensor::validateSensorDriver() */ void CameraSensor::initVimcDefaultProperties() { - pixelArraySize_ = resolution(); 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> --- include/libcamera/internal/camera_sensor.h | 3 +-- src/libcamera/camera_sensor.cpp | 16 ++++++++-------- 2 files changed, 9 insertions(+), 10 deletions(-)