[libcamera-devel] [PATCH v2 1/7] libcamera: ipu3: imgu: Update BDS calculation process

[Jacopo Mondi]

Hi Hiro,
   Yes sorry

On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:
> >> > > > +     unsigned int ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> >> > > > +     unsigned int ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> >> > > > +     unsigned int minIfWidth = in.width - IF_CROP_MAX_W;
> >> > > > +     unsigned int minIfHeight = in.height - IF_CROP_MAX_H;
> >> > >
> >> >
> >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?
> >>
> >> I would be careful and not use 0, but probably 2 and 4 as it seems to be
> >> the
> >> vertical and horizontal alignments the ImgU requires
> >>
> >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth becomes
> >> > very large.
> >> > Ditto to height.
> >>
> >> Laurent had the same question, and we received patch in the past that
> >> was probably related to an issue like this one, if the input size is
> >> smaller than 540 (as IF_CROP_MAX_H=540).
> >> https://patchwork.libcamera.org/patch/11620/
> >>
> >> I've opened (yet) a new issue on the python script this code is based
> >> on to have Intel's opinion on which is the min IF crop size
> >> https://github.com/intel/intel-ipu3-pipecfg/issues/3
> >>
> >>
> > The variable in the python script is (signed) integer.
> > So overflow doesn't happen.
> > The code may work even if width < IF_CROP_MAX_W (I don't check the code
> > any more than it)?
> > On the other hand, our code doesn't work because the following while-loop
> > doesn't loop..?
> > I see your point. It is the best to set the minimum allowed value after we
> > get their feedback.
> > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround to the
> > overflow with todo comment?
> > Well, since the code of causing the overflow is in top-of-tree, I don't
> > mind merging this.
> >
> >
>
> Ping. Can you respond to this before you merge this patch?
>

I've taken your suggestion in

I now have got in my wip v3 version:

+       unsigned int minIfWidth = std::min(IF_ALIGN_W,
+                                          in.width - IF_CROP_MAX_W);
+       unsigned int minIfHeight = std::min(IF_ALIGN_H,
+                                           in.height - IF_CROP_MAX_H);

I will send it out anyway before merging.

>
> > -Hiro
> >
> > I'm thinking of fixing this issue, and the other ones reported during
> >> review of this series (and reflected as issues on the script's github)
> >> on top.
> >>
> >> I'm overly-concerned with the idea of deviating from the script. I've
> >> received several comments on how things could have been done better,
> >> but I'm pushing back as I want this code to be as similar as possible
> >> to the intel's script, otherwise every 4 months when (I have to)
> >> compare the two implementation my brain will melt even more than
> >> usual.
> >>
> >> Result: the code is of awful quality as the script is.
> >>
> >> I re-state the requirement from Intel to provide documentation on
> >> how the ImgU sizes have to be computed instead of a buggy script
> >> (which I apreciate a lot don't get me wrong, I'm sure the author has
> >> put some of his free time to implement that as I'm quite sure nobody
> >> in there apreciate spending more company time on IPU3, but the result
> >> is a best effort implementation nobody is proud of).
> >>
> >> Even better if they could do directly in libcamera... a man can
> >> dream... So far we received close to 0 attention from them and surely
> >> we have no leverage from here to convince them of the contrary :)
> >> Maybe you could push a little to have them consider us a little
> >> bit more ?
> >>
> >> Thanks
> >>    j
> >>
> >> >
> >> >
> >> > > >       while (ifWidth >= minIfWidth) {
> >> > > > -             Size iif{ ifWidth, ifHeight };
> >> > > > -             calculateBDS(pipe, iif, gdc, sf);
> >> > > > +             while (ifHeight >= minIfHeight) {
> >> > > > +                     Size iif{ ifWidth, ifHeight };
> >> > > > +                     calculateBDS(pipe, iif, gdc, sf);
> >> > > > +                     ifHeight -= IF_ALIGN_H;
> >> > > > +             }
> >> > > >
> >> > > >               ifWidth -= IF_ALIGN_W;
> >> > > >       }
> >> > > >
> >> > > > +     /* Repeat search by scaling width first. */
> >> > > > +     ifWidth = utils::alignUp(in.width, IF_ALIGN_W);
> >> > > > +     ifHeight = utils::alignUp(in.height, IF_ALIGN_H);
> >> > > > +     minIfWidth = in.width - IF_CROP_MAX_W;
> >> > > > +     minIfHeight = in.height - IF_CROP_MAX_H;
> >> > > > +     while (ifHeight >= minIfHeight) {
> >> > > > +             /*
> >> > > > +              * \todo This procedure is probably broken:
> >> > > > +              *
> >> https://github.com/intel/intel-ipu3-pipecfg/issues/2
> >> > > > +              */
> >> > > > +             while (ifWidth >= minIfWidth) {
> >> > > > +                     Size iif{ ifWidth, ifHeight };
> >> > > > +                     calculateBDS(pipe, iif, gdc, sf);
> >> > > > +                     ifWidth -= IF_ALIGN_W;
> >> > > > +             }
> >> > > > +
> >> > > > +             ifHeight -= IF_ALIGN_H;
> >> > > > +     }
> >> > > > +
> >> > > >       if (pipeConfigs.size() == 0) {
> >> > > >               LOG(IPU3, Error) << "Failed to calculate pipe
> >> > > configuration";
> >> > > >               return {};
> >> > > > --
> >> > > > 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
> >> > >
> >>
> >