[libcamera-devel] [PATCH v2 12/20] libcamera: ipu3: Always use the maximum frame size

[Jacopo Mondi]

Hi Laurent,

On Fri, Jul 10, 2020 at 03:07:40PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Jul 10, 2020 at 09:38:53AM +0200, Jacopo Mondi wrote:
> > On Thu, Jul 09, 2020 at 03:46:54PM +0200, Niklas Söderlund wrote:
> > > On 2020-07-09 10:41:20 +0200, Jacopo Mondi wrote:
> > > > The requirement of having the ImgU output height 32 pixels smaller
> > > > than the input frame produced by the CIO2 makes it complicated to
> > > > re-adjust the sensor produced size after the alignement has been
> > > > applied.
> > > >
> > > > To simplify the procedure, always ask for the full frame size from the
> > > > CIO2 unit.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > ---
> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++--------
> > > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index d07f1a7b5ae8..feabffe641e1 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -156,8 +156,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > >  	unsigned int rawCount = 0;
> > > >  	unsigned int outCount = 0;
> > > >  	Size yuvSize;
> > > > -	Size size;
> > > > -
> > > >  	for (const StreamConfiguration &cfg : config_) {
> > > >  		const PixelFormatInfo &info =
> > > >  			PixelFormatInfo::info(cfg.pixelFormat);
> > > > @@ -174,11 +172,6 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > >  			if (cfg.size > yuvSize)
> > > >  				yuvSize = cfg.size;
> > > >  		}
> > > > -
> > > > -		if (cfg.size.width > size.width)
> > > > -			size.width = cfg.size.width;
> > > > -		if (cfg.size.height > size.height)
> > > > -			size.height = cfg.size.height;
> > > >  	}
> > > >  	if (rawCount > 1 || outCount > 2) {
> > > >  		LOG(IPU3, Error)
> > > > @@ -189,10 +182,13 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > > >  	}
> > > >
> > > >  	/* Generate raw configuration from CIO2. */
> > > > -	cio2Configuration_ = data_->cio2_.generateConfiguration(size);
> > > > +	Size sensorSize = data_->cio2_.sensor()->resolution();
> > > > +	cio2Configuration_ = data_->cio2_.generateConfiguration(sensorSize);
> > >
> > > Could this not impact framerate for the CIO2 if we always request the
> > > largest sensor size? Would it be possible instead to increase the size
> > > passed to CIO2Device::generateConfiguration() by 32?
> >
> > Yes it does, and this is probably just wrong, as it ignores the
> > requested raw capture size.
> >
> > Fun fact which actually lead me to add this change: if I keep the
> > previous version where the sensor is asked for the smaller as possible
> > resolution that supports all stream (+32 as you here suggest) the IPU3
> > pipeline parameter calculations fail on Soraka for some output
> > resolutions.
> >
> > In example: calculating the configuration for a single stream in
> > 1280x720 with a frame resolution of 2112x1188 (which is the smaller
> > ov13858 resolution that is large enough) fails: the tool is not able
> > to find any suitable configuration that satisfied the pipeline
> > constraints:
> >
> > $ python pipe_config.py input=2112x1188 main=1280x720
> > IndexError: list index out of range
> >
> > If I use the full frame size, everything is unicorns&rainbows again
> >
> > $ python pipe_config.py input=4224x3136 main=1280x720
> > -------- The selected pipe configuration: --------------
> > output_res_if:4220x2600
> > output_res_bds:1688x1040
> > output_res_gdc:1280x720
> > FOV Horizontal:0.757576, FOV vertical:0.573980
> >
> > Why ? I have no idea, and I think we should stop waiting for Intel to
> > actually care about this platform anymore, so I don't expect they
> > could help us debugging the tool.
>
> What's the alternative, to you want to debug it yourself ? :-) If there
> are issue, please report them, and you can CC Bingbu and Tomasz.
>

The alternative is to live with the risk of the procedure not working
for all resolutions. Which is anyway better thant what we have today

> > The re-implementation I made performs the same calculations as the
> > script, hence reducing the frame size makes a lot of configurations
> > fail.
> >
> > If you look at
> > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov5670.xml
> > https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/baseboard-poppy/media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml
> >
> > The sensor is always asked for the full frame,
>
> Is it ? For the ov5670 I see three configurations using 1296x972, and
> for the ov13858 I see nine configurations with smaller sensor
> resolutions.

Thanks for checking, I grepped on the xml file, but visual inspection
revealed that I was not doing it right

 <input width="1296" height="972" format="CIO2" />

Now  I see this too

>
> > so I suspect that's how
> > the system is intenteded to work (even if, I wonder, what happens if I
> > connect a sensor that can only produce the above failing 2112x1188
> > resolution ?)
> >
> > I -fear- for IPU3 we should always go full frame size to make the pipe
> > config happy, or there's maybe a requirement to respect some aspect
> > ratio I still haven't been able to figure out. In any case, this would
> > hinder the ability to capture raw+YUV, as the raw frame resolution in
> > that case is selected from the application, and it might prevent the
> > pipeline from being able to calculate a configuration.
> >
> > Not nice, I know.
> >
> > If that's how thing have to work, a software scaler for raw frames
> > would nee to be plugged to the pipeline to down-scale raw frames to
> > the desired resolution. Or we restrict RAW streams to the full size
> > only. What do you think ?
>
> No software scaler, no. If a configuration isn't supported by the
> hardware we can obviously reject it. For instance if the application
> requests raw in 640x480 and wants YUV in a larger resolution, as the
> ImgU can't downscale, we can't accept that, and we can go for a higher
> raw resolution or a small YUV resolution. There's nothing new under the
> sun here, and I'm OK with temporarily restricting the raw frames to the
> full sensor resolution if it helps moving forward, but let's keep in
> mind we may have to fix that at some point in the future, so we
> shouldn't hardcode too much.

I see. I'll try to rework this with this in mind.

Thanks
  j

>
> > > >  	if (!cio2Configuration_.pixelFormat.isValid())
> > > >  		return Invalid;
> > > >
> > > > +	LOG(IPU3, Debug) << "CIO2 configuration: " << cio2Configuration_.toString();
> > > > +
> > > >  	/*
> > > >  	 * Adjust the configurations if needed and assign streams while
> > > >  	 * iterating them.
>
> --
> Regards,
>
> Laurent Pinchart