[libcamera-devel] [PATCH] libcamera: camera_sensor: Use active area size as resolution

[Jacopo Mondi]

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 at 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