[libcamera-devel] [PATCH v2 3/4] ipu3: cio2: Change sensor size selection policy

[Jacopo Mondi]

Hi Kieran,

On Fri, Aug 27, 2021 at 01:47:45PM +0100, Kieran Bingham wrote:
>
> On 10/08/2021 08:58, Umang Jain wrote:
> > The current implementation of getSensorFormat() prioritizes sensor
> > sizes that match the output size FOV ratio.
> >
> > Modify the frame size selection procedure to prioritize resolutions
> > with the same FOV as the sensor's native one, to avoid cropping in the
> > sensor pixel array.
> >
> > For example:
> >
> > - on a Soraka device equipped with ov13858 as back sensor, with a
> >   native resolution of 4224x3136 (4:3), when requested to select the
> >   sensor output size to produce 1080p (16:9) a frame size of 2112x1188
> >   (16:9) is selected causing the ImgU configuration procedure to fail.
> >   If a resolution with the same FOV as the sensor's native size, such
> >   as 2112x1568 (4:3), is selected the pipeline works correctly.
> > > Suggested-by:: Jacopo Mondi <jacopo at jmondi.org>
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain at ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/cio2.cpp | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 88dd364b..3c9331e3 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -248,8 +248,8 @@ StreamConfiguration CIO2Device::generateConfiguration(Size size) const
> >   *
> >   * - The desired \a size shall fit in the sensor output size to avoid the need
> >   *   to up-scale.
> > - * - The sensor output size shall match the desired aspect ratio to avoid the
> > - *   need to crop the field of view.
> > + * - The aspect ratio of sensor output size shall be as close as possible to
> > + *   the sensor's native resolution field of view.
> >   * - The sensor output size shall be as small as possible to lower the required
> >   *   bandwidth.
> >   * - The desired \a size shall be supported by one of the media bus code listed
> > @@ -273,7 +273,9 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
> >  {
> >  	unsigned int desiredArea = size.width * size.height;
> >  	unsigned int bestArea = std::numeric_limits<unsigned int>::max();
> > -	float desiredRatio = static_cast<float>(size.width) / size.height;
> > +	const Size &resolution = sensor_->resolution();
> > +	float desiredRatio = static_cast<float>(resolution.width) /
> > +			     resolution.height;
>
>
> Ok - so we always try to keep the sensor format that is close to native,
> rather than the closest to what is requested.
>
> I'm not yet sure I understand why that is better
>

Neither do I. I actually don't think it's better in a general sense.

Simply, the ImgU configuration procedure has been developed using
resolutions closer to the sensor's native one, and hand tuned when it
was failing, I suppose. For the sensors it has been tested with (the
Soraka and Nautilus ones, and maybe other ChromeOS devices), providing
the same set of input data makes sure the configuration won't fail.

Will it work with other sensors the same way ? We have no guarantees
but with the platforms we can test it makes the configuration
successful.


> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
>
> But does this mean that if I ask for a 16:9 1080p image, I'm going to
> get a larger 4k image, which would then be perceptually squashed?
>
> The end result will still be cropped somewhere right?
>
>
>
> >  	float bestRatio = std::numeric_limits<float>::max();
> >  	Size bestSize;
> >  	uint32_t bestCode = 0;
> >