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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Apr 21, 2021 at 02:57:56PM +0200, Jacopo Mondi wrote:
> On Tue, Apr 13, 2021 at 03:18:02AM +0300, Laurent Pinchart wrote:
> > On Thu, Mar 18, 2021 at 11:39:37AM +0100, 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.
> > >
> > > 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 d9296ea3e674..11bb7cb95084 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 };
> >
> > You don't need two values, a single one will do, it can be overwritten
> > in the second loop.
> 
> Correct, I got the same understanding but I would prefer to use the
> same "style" as the python script, not to diverge...
> 
> What do you think ?

I don't think diverging is an issue, as I believe this code should be
completely rewritten based on an actual understanding of the procedures,
instead of blindly mimicking python code that has known issues. I'm OK
with this series as a short term, stop-gap measure, but it's clearly not
what we need longer term. I will likely insist more strongly to have
this rewritten the next time code from python is integrated.

> > With an updated commit message,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > >  		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]) {
> > >  			unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> > >
> > >  			pipeConfigs.push_back({ bdsSF, { iif.width, ifHeight },

-- 
Regards,

Laurent Pinchart