[libcamera-devel] [PATCH v2 3/7] libcamera: ipu3: imgu: Fix BDS height calculation

[Jacopo Mondi]

Hello,
   time to time I dig this series out of the dead patches cemetery.
I'm kind of the Lara Croft of libcamera

On Thu, May 06, 2021 at 02:51:03PM +0900, Hirokazu Honda wrote:
> Hi Jacopo, thank you for the patch,
>
> On Mon, May 3, 2021 at 7:52 PM Niklas Söderlund <
> niklas.soderlund at ragnatech.se> wrote:
>
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2021-05-03 11:27:01 +0200, Jacopo Mondi wrote:
> > > The IF rectangle height is iteratively computed by first subtracting
> > > the scaling factor to the estimated height, then in a successive loop
> > > by adding the same scaling factor until the maximum IF size is not
> > > reached.
> > >
> > > As the computed IF height is not cached in any variable, the second
> > > loop over-writes the result of the first one, even if the BDS alignment
> > > condition is not satisfied.
> > >
> > > Fix this by caching the result of the two iterations, and use the one
> > > that produced any result, with a preference for the one produced by the
> > > second loop, as implemented in the reference python script.
> > >
> > > Tested-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/imgu.cpp | 13 +++++++++----
> > >  1 file changed, 9 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp
> > b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > index 36fee31c1962..ea654e57b0e7 100644
> > > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > > @@ -129,10 +129,10 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> > const Size &iif, const Size &gdc
> > >       float bdsHeight;
> > >
> > >       if (!isSameRatio(pipe->input, gdc)) {
> > > +             unsigned int foundIfHeights[2] = { 0, 0 };
> > >               float estIFHeight = (iif.width * gdc.height) /
> > >                                   static_cast<float>(gdc.width);
> > >               estIFHeight = std::clamp<float>(estIFHeight, minIFHeight,
> > iif.height);
> > > -             bool found = false;
> > >
> > >               ifHeight = utils::alignUp(estIFHeight, IF_ALIGN_H);
> > >               while (ifHeight >= minIFHeight && ifHeight / bdsSF >=
> > minBDSHeight) {
> > > @@ -142,7 +142,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> > const Size &iif, const Size &gdc
> > >                               unsigned int bdsIntHeight =
> > static_cast<unsigned int>(bdsHeight);
> > >
> > >                               if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > > -                                     found = true;
> > > +                                     foundIfHeights[0] = ifHeight;
> > >                                       break;
> > >                               }
> > >                       }
> > > @@ -158,7 +158,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> > const Size &iif, const Size &gdc
> > >                               unsigned int bdsIntHeight =
> > static_cast<unsigned int>(bdsHeight);
> > >
> > >                               if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > > -                                     found = true;
> > > +                                     foundIfHeights[1] = ifHeight;
> > >                                       break;
> > >                               }
> > >                       }
> > > @@ -166,7 +166,12 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe,
> > const Size &iif, const Size &gdc
> > >                       ifHeight += IF_ALIGN_H;
> > >               }
> > >
> > > -             if (found) {
> > > +             if (foundIfHeights[0])
> > > +                     ifHeight = foundIfHeights[0];
> > > +             if (foundIfHeights[1])
> > > +                     ifHeight = foundIfHeights[1];
> > > +
> > > +             if (foundIfHeights[0] || foundIfHeights[1]) {
> >
> > nit: This could be simplified by turning 'found' into an unsigned int
> > initialized to 0 and storing ifHeight inside found instead of true. But
> > I think this maybe a matter of taste so with or without this,
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> >
> >
> Is it necessary to do the second-loop when a height is found in the first
> loop?

Yes, as

             if (foundIfHeights[0])
                     ifHeight = foundIfHeights[0];
             if (foundIfHeights[1])
                     ifHeight = foundIfHeights[1];

overwrites the first found result with the second found one.
Actually as Niklas and Laurent pointed out, this can be handled with a
single variable that gets overwritten by the second loop, if found.

Let's deviate from the script, I'll take this change in.

Thanks
   j

>
> -Hiro
>
> > >                       unsigned int bdsIntHeight = static_cast<unsigned
> > int>(bdsHeight);
> > >
> > >                       pipeConfigs.push_back({ bdsSF, { iif.width,
> > ifHeight },
> > > --
> > > 2.31.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
> >