[libcamera-devel] [PATCH v2] libcamera: camera_sensor: Use active area size as resolution
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 3 15:33:47 CET 2021
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 at 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 at 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_);
> > }
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list