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

[Hirokazu Honda]

Hi Jacopo,

On Tue, May 11, 2021 at 11:36 PM Jacopo Mondi <jacopo at jmondi.org> wrote:

> 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.
>
>
Ack.

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>


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