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

[Hirokazu Honda]

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?

-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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210506/173ae3b8/attachment.htm>