[PATCH v4 15/20] pipeline: rkisp1: Enable the dewarper unconditionally

[Jacopo Mondi]

Hi Stefan

On Mon, Dec 16, 2024 at 09:30:15PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Mon, Dec 16, 2024 at 07:32:24PM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> >
> > On Mon, Dec 16, 2024 at 04:40:55PM +0100, Stefan Klug wrote:
> > > In configure() and in the future in generateConfiguration() the
> > > calculated stream sizes and crop rectangles depend on the dewarper being
> > > used or not. It is therefore not possible to postpone that decision
> > > until the dewarper gets configured. Enable the dewarper unconditionally
> > > if it is found and the stream type is not RAW.
> >
> > Am I wrong or the commit message doesn't match the patch ?
>
> Hm, no that was really the message for that change. To me it explains
> the reasoning of the change. This patch was slimmed down by the internal
> fixup dd7f3d5a2436 ("fixup! libcamera: rkisp1: Enable the dewarper
> unconditionally") that you pushed to our common branch (which was
> absolutely valid).

This was a partial revert, and what's left in the commit is

>
> Did I miss something?
>
> Best regards,
> Stefan
>
> >
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Collected tags
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 18038226912a..14d4bb9a929b 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -865,7 +865,10 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> > >  			if (dewarper_ && !isRaw_) {
> > >  				outputCfgs.push_back(const_cast<StreamConfiguration &>(cfg));
> > >  				ret = dewarper_->configure(cfg, outputCfgs);
> > > -				useDewarper_ = ret ? false : true;
> > > +				if (ret)
> > > +					return ret;
> > > +
> > > +				useDewarper_ = true;

Do not enable the dewarper if its configuration fails.
You'll enable the dewarper earlier in a later patch as noted.

Just change the commit message to highlight what this patch does
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

> > >
> > >  				/*
> > >  				 * Calculate the crop rectangle of the data
> > > --
> > > 2.43.0
> > >