<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, May 12, 2021 at 5:45 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro,<br>
<br>
On Wed, May 12, 2021 at 04:56:19PM +0900, Hirokazu Honda wrote:<br>
> Hi Jacopo,<br>
><br>
> On Wed, May 12, 2021 at 4:45 PM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Hi Hiro, everyone<br>
> ><br>
> > On Wed, May 12, 2021 at 09:16:17AM +0200, Jacopo Mondi wrote:<br>
> > > Hi Hiro,<br>
> > > Yes sorry<br>
> > ><br>
> > > On Wed, May 12, 2021 at 01:23:14PM +0900, Hirokazu Honda wrote:<br>
> > > > >> > > > + unsigned int ifWidth = utils::alignUp(in.width,<br>
> > IF_ALIGN_W);<br>
> > > > >> > > > + unsigned int ifHeight = utils::alignUp(in.height,<br>
> > IF_ALIGN_H);<br>
> > > > >> > > > + unsigned int minIfWidth = in.width - IF_CROP_MAX_W;<br>
> > > > >> > > > + unsigned int minIfHeight = in.height - IF_CROP_MAX_H;<br>
> > > > >> > ><br>
> > > > >> ><br>
> > > > >> > Should we std::max(0u, in.width - IF_CROP_MAX_W)?<br>
> > > > >><br>
> > > > >> I would be careful and not use 0, but probably 2 and 4 as it seems<br>
> > to be<br>
> > > > >> the<br>
> > > > >> vertical and horizontal alignments the ImgU requires<br>
> > > > >><br>
> > > > >> > If in.width < IF_CROP_MAX_W, an overflow happens and minIfWidth<br>
> > becomes<br>
> > > > >> > very large.<br>
> > > > >> > Ditto to height.<br>
> > > > >><br>
> > > > >> Laurent had the same question, and we received patch in the past<br>
> > that<br>
> > > > >> was probably related to an issue like this one, if the input size is<br>
> > > > >> smaller than 540 (as IF_CROP_MAX_H=540).<br>
> > > > >> <a href="https://patchwork.libcamera.org/patch/11620/" rel="noreferrer" target="_blank">https://patchwork.libcamera.org/patch/11620/</a><br>
> > > > >><br>
> > > > >> I've opened (yet) a new issue on the python script this code is<br>
> > based<br>
> > > > >> on to have Intel's opinion on which is the min IF crop size<br>
> > > > >> <a href="https://github.com/intel/intel-ipu3-pipecfg/issues/3" rel="noreferrer" target="_blank">https://github.com/intel/intel-ipu3-pipecfg/issues/3</a><br>
> > > > >><br>
> > > > >><br>
> > > > > The variable in the python script is (signed) integer.<br>
> > > > > So overflow doesn't happen.<br>
> > > > > The code may work even if width < IF_CROP_MAX_W (I don't check the<br>
> > code<br>
> > > > > any more than it)?<br>
> > > > > On the other hand, our code doesn't work because the following<br>
> > while-loop<br>
> > > > > doesn't loop..?<br>
> > > > > I see your point. It is the best to set the minimum allowed value<br>
> > after we<br>
> > > > > get their feedback.<br>
> > > > > But, hmm, shall we set it to IF_ALIGN_W as the temporary workaround<br>
> > to the<br>
> > > > > overflow with todo comment?<br>
> > > > > Well, since the code of causing the overflow is in top-of-tree, I<br>
> > don't<br>
> > > > > mind merging this.<br>
> > > > ><br>
> > > > ><br>
> > > ><br>
> > > > Ping. Can you respond to this before you merge this patch?<br>
> > > ><br>
> > ><br>
> > > I've taken your suggestion in<br>
> > ><br>
> > > I now have got in my wip v3 version:<br>
> > ><br>
> > > + unsigned int minIfWidth = std::min(IF_ALIGN_W,<br>
> > > + in.width - IF_CROP_MAX_W);<br>
> > > + unsigned int minIfHeight = std::min(IF_ALIGN_H,<br>
> > > + in.height - IF_CROP_MAX_H);<br>
> > ><br>
> > > I will send it out anyway before merging.<br>
> > ><br>
> ><br>
> > Run some more testing. With the above change, the number of<br>
> > resolutions that are actually tested quickly becomes not manageable at<br>
> > run time.<br>
> ><br>
> > Just starting the HAL takes 5 seconds per each camera and the number<br>
> > of tested configurations goes up to HALF A MILLION (for each<br>
> > resolutions combination we test at HAL startup time).<br>
> ><br>
> > The overflow helps skipping a lot of configurations, with a maximum of<br>
> > 30.000 or so. Which is anyway awful but acceptable as the HAL startup<br>
> > delay is barely noticeable.<br>
> ><br>
> > I'm not sure how to move forward, if we just skip heights <<br>
> > IF_CROP_MAX_H (which is 540) to avoid the overflow, we would rule out<br>
> > sensors that produces images smaller than 540. This happens today<br>
> > already so it won't technically be a regression, it's just bad.<br>
> ><br>
> ><br>
> Wow... it is pretty bad.<br>
<br>
yes indeed.<br>
<br>
> IMHO, we can go ahead temporarily with the overflow leaving a todo comment<br>
> and let's file a bug about it as this is not a regression introduced by<br>
> this.<br>
> We may want to discuss in the issue tracker.<br>
<br>
I've not recorded it in a bug, and will refer to that in the code<br>
<a href="https://bugs.libcamera.org/show_bug.cgi?id=32" rel="noreferrer" target="_blank">https://bugs.libcamera.org/show_bug.cgi?id=32</a><br>
<br>
For the moment I feel it would be safer to skip all input resolutions <<br>
IF_CROP_MAX<br>
<br></blockquote><div><br></div><div>Ack. Thanks!</div><div><br></div><div>Reviewed-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org">hiroh@chromium.org</a>></div><div><br></div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
><br>
> -Hiro<br>
><br>
> > ><br>
> > > > > -Hiro<br>
> > > > ><br>
> > > > > I'm thinking of fixing this issue, and the other ones reported during<br>
> > > > >> review of this series (and reflected as issues on the script's<br>
> > github)<br>
> > > > >> on top.<br>
> > > > >><br>
> > > > >> I'm overly-concerned with the idea of deviating from the script.<br>
> > I've<br>
> > > > >> received several comments on how things could have been done better,<br>
> > > > >> but I'm pushing back as I want this code to be as similar as<br>
> > possible<br>
> > > > >> to the intel's script, otherwise every 4 months when (I have to)<br>
> > > > >> compare the two implementation my brain will melt even more than<br>
> > > > >> usual.<br>
> > > > >><br>
> > > > >> Result: the code is of awful quality as the script is.<br>
> > > > >><br>
> > > > >> I re-state the requirement from Intel to provide documentation on<br>
> > > > >> how the ImgU sizes have to be computed instead of a buggy script<br>
> > > > >> (which I apreciate a lot don't get me wrong, I'm sure the author has<br>
> > > > >> put some of his free time to implement that as I'm quite sure nobody<br>
> > > > >> in there apreciate spending more company time on IPU3, but the<br>
> > result<br>
> > > > >> is a best effort implementation nobody is proud of).<br>
> > > > >><br>
> > > > >> Even better if they could do directly in libcamera... a man can<br>
> > > > >> dream... So far we received close to 0 attention from them and<br>
> > surely<br>
> > > > >> we have no leverage from here to convince them of the contrary :)<br>
> > > > >> Maybe you could push a little to have them consider us a little<br>
> > > > >> bit more ?<br>
> > > > >><br>
> > > > >> Thanks<br>
> > > > >> j<br>
> > > > >><br>
> > > > >> ><br>
> > > > >> ><br>
> > > > >> > > > while (ifWidth >= minIfWidth) {<br>
> > > > >> > > > - Size iif{ ifWidth, ifHeight };<br>
> > > > >> > > > - calculateBDS(pipe, iif, gdc, sf);<br>
> > > > >> > > > + while (ifHeight >= minIfHeight) {<br>
> > > > >> > > > + Size iif{ ifWidth, ifHeight };<br>
> > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);<br>
> > > > >> > > > + ifHeight -= IF_ALIGN_H;<br>
> > > > >> > > > + }<br>
> > > > >> > > ><br>
> > > > >> > > > ifWidth -= IF_ALIGN_W;<br>
> > > > >> > > > }<br>
> > > > >> > > ><br>
> > > > >> > > > + /* Repeat search by scaling width first. */<br>
> > > > >> > > > + ifWidth = utils::alignUp(in.width, IF_ALIGN_W);<br>
> > > > >> > > > + ifHeight = utils::alignUp(in.height, IF_ALIGN_H);<br>
> > > > >> > > > + minIfWidth = in.width - IF_CROP_MAX_W;<br>
> > > > >> > > > + minIfHeight = in.height - IF_CROP_MAX_H;<br>
> > > > >> > > > + while (ifHeight >= minIfHeight) {<br>
> > > > >> > > > + /*<br>
> > > > >> > > > + * \todo This procedure is probably broken:<br>
> > > > >> > > > + *<br>
> > > > >> <a href="https://github.com/intel/intel-ipu3-pipecfg/issues/2" rel="noreferrer" target="_blank">https://github.com/intel/intel-ipu3-pipecfg/issues/2</a><br>
> > > > >> > > > + */<br>
> > > > >> > > > + while (ifWidth >= minIfWidth) {<br>
> > > > >> > > > + Size iif{ ifWidth, ifHeight };<br>
> > > > >> > > > + calculateBDS(pipe, iif, gdc, sf);<br>
> > > > >> > > > + ifWidth -= IF_ALIGN_W;<br>
> > > > >> > > > + }<br>
> > > > >> > > > +<br>
> > > > >> > > > + ifHeight -= IF_ALIGN_H;<br>
> > > > >> > > > + }<br>
> > > > >> > > > +<br>
> > > > >> > > > if (pipeConfigs.size() == 0) {<br>
> > > > >> > > > LOG(IPU3, Error) << "Failed to calculate pipe<br>
> > > > >> > > configuration";<br>
> > > > >> > > > return {};<br>
> > > > >> > > > --<br>
> > > > >> > > > 2.31.1<br>
> > > > >> > > ><br>
> > > > >> > > > _______________________________________________<br>
> > > > >> > > > libcamera-devel mailing list<br>
> > > > >> > > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > > >> > > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> > > > >> > ><br>
> > > > >> > > --<br>
> > > > >> > > Regards,<br>
> > > > >> > > Niklas Söderlund<br>
> > > > >> > > _______________________________________________<br>
> > > > >> > > libcamera-devel mailing list<br>
> > > > >> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > > >> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> > > > >> > ><br>
> > > > >><br>
> > > > ><br>
> > > _______________________________________________<br>
> > > libcamera-devel mailing list<br>
> > > <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> > > <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
> ><br>
</blockquote></div></div>