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