[v4,15/20] pipeline: rkisp1: Enable the dewarper unconditionally
diff mbox series

Message ID 20241216154124.203650-16-stefan.klug@ideasonboard.com
State Accepted
Headers show
Series
  • rkisp1: Fix aspect ratio and ScalerCrop
Related show

Commit Message

Stefan Klug Dec. 16, 2024, 3:40 p.m. UTC 
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.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

---

Changes in v4:
- Collected tags
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Dec. 16, 2024, 6:32 p.m. UTC | #1 
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 ?

>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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;
>
>  				/*
>  				 * Calculate the crop rectangle of the data
> --
> 2.43.0
>
Stefan Klug Dec. 16, 2024, 8:30 p.m. UTC | #2 
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).

Did I miss something?

Best regards,
Stefan

> 
> >
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@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;
> >
> >  				/*
> >  				 * Calculate the crop rectangle of the data
> > --
> > 2.43.0
> >
Jacopo Mondi Dec. 17, 2024, 7:56 a.m. UTC | #3 
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@ideasonboard.com>
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@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@ideasonboard.com>

Thanks
  j

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

Patch
diff mbox series 

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;
 
 				/*
 				 * Calculate the crop rectangle of the data