[libcamera-devel] [PATCH v3 4/4] ipu3: cio2: Tweak sensor size selection policy

[Jacopo Mondi]

Hello Umang,

On Mon, Aug 30, 2021 at 03:38:44PM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 8/30/21 2:22 PM, Jacopo Mondi wrote:
> > On Mon, Aug 30, 2021 at 02:02:27PM +0530, Umang Jain wrote:
> > > Hi Me,
> > >
> > > On 8/30/21 1:51 PM, Umang Jain wrote:
> > > > Do not compare higher precision of the ratios, as it might lead to
> > > > absurd selection of sensor size for a relatively low requested
> > > > resolution size.
> > > >
> > > > For example:
> > > > The imx258 driver supports the following sensor resolutions:
> > > >
> > > >    - 4208x3118 = 1.349583066
> > > >    - 2104x1560 = 1.348717949
> > > >    - 1048x780  = 1.343589744
> > > >
> > > > It can be inferred that, that the aspect ratio only differs by a small
> > > > factor with each other. It does not makes sense to select a 4208x3118
> > > > for a requested size of say 640x480 or 1280x720, which is what is
> > > > happening currently.
> > > > ($) cam -c1 -swidth=640,height=480,role=raw
> > > >       - CIO2 configuration: 4208x3118-SGRBG10_IPU3
> > >
> > > This cam command is invalid as of today. It was fine when I wrote the
> > > series.
> > >
> > > The invalidation comes from 4fec25a4b ("ipu3: Disallow raw only camera
> > > configuration"). I am tempted to remove this cam command (since it's not
> > > valid today), but I don't think I would have a better explanation than this.
> > > All I can do is to replace it with a block of text (essentially making the
> > > commit message still longer)
> > >
> > > Any ideas anyone?
> > >
> > What about adding a YUV stream of the same size to the command ?
>
>
> As of master, 7208e70211a6 ("libcamera: ipu3: Always use sensor full frame
> size") is in effect  right? So it's always selecting the highest sensor
> format resolution.

Ah ups, I'm running with my branch :)

>
> I cherry-picked your "DNI: libcamera: ipu3: Use the optimal sensor side"
> which generates the same results as the commit message, to double-check.
>
> So I don't adding YUV stream helps. Probably I'll just need to drop these
> commands and explain everything in a text.

You're right!
Or maybe, for the sake of the example, just revert the commit that
blocks the single RAW stream ?

Anyway, up to you :)

>
> >
> > > > In order to address this constraint, only compare the ratio with single
> > > > precision to make a better decision on the sensor resolution policy
> > > > selection.
> > > >
> > > > ($) cam -c1 -srole=raw,width=640,height=480
> > > >       - CIO2 configuration: 1048x780-SGRBG10_IPU3
> > > >
> > > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > > Tested-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > ---
> > > >    src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++
> > > >    1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > index ceaf665e..9cedcb5b 100644
> > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > > @@ -295,6 +295,14 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
> > > >    				continue;
> > > >    			float ratio = static_cast<float>(sz.width) / sz.height;
> > > > +			/*
> > > > +			 * Ratios can differ by small mantissa difference which
> > > > +			 * can affect the selection of the sensor output size
> > > > +			 * wildly. We are interested in selection of the closest
> > > > +			 * size with respect to the desired output size, hence
> > > > +			 * comparing it with a single precision digit is enough.
> > > > +			 */
> > > > +			ratio = static_cast<unsigned int>(ratio * 10) / 10.0;
> > > >    			float ratioDiff = fabsf(ratio - desiredRatio);
> > > >    			unsigned int area = sz.width * sz.height;
> > > >    			unsigned int areaDiff = area - desiredArea;