[PATCH v4 10/20] pipeline: rkisp1: Query dewarper crop bounds if no stream configured

[Jacopo Mondi]

Hi Stefan

On Mon, Dec 16, 2024 at 09:47:16PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On Mon, Dec 16, 2024 at 07:14:05PM +0100, Jacopo Mondi wrote:
> > Hi Stefan
> >
> > On Mon, Dec 16, 2024 at 04:40:50PM +0100, Stefan Klug wrote:
> > > Query the crop bounds on the dewarper instead of the stream in case the
> > > camera was not yet configured when updateControls() gets called. This
> > > provides sane defaults for the controls.
> > >
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > >
> > > ---
> > >
> > > Changes in v4:
> > > - Split patch from libcamera: converter_v4l2_m2m: Improve crop bounds
> > >   support
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 89946b782854..56192451eb3c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -1220,8 +1220,11 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
> > >  	ControlInfoMap::Map controls;
> > >
> > >  	if (dewarper_) {
> > > -		std::pair<Rectangle, Rectangle> cropLimits =
> > > -			dewarper_->inputCropBounds(&data->mainPathStream_);
> > > +		std::pair<Rectangle, Rectangle> cropLimits;
> > > +		if (dewarper_->isConfigured(&data->mainPathStream_))
> > > +			cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_);
> > > +		else
> > > +			cropLimits = dewarper_->inputCropBounds();
> >
> > Now I wonder if the right way shouldn't have been
> >
> > /**
> >  * \fn Converter::inputCropBounds(const Stream *stream)
> >  * \brief Retrieve the crop bounds for \a stream
> >  * \param[in] stream The output stream
> >
> >  * ....
> >
> > - *
> > - * When called with an unconfigured \a stream, this function returns a pair of
> > - * null rectangles.
> > + * When called with an unconfigured \a stream, this function returns
> > + * the coverter default crop rectangle
> > + *
>
> That is exactly the version from v2 - righti? :-)
>
> >
> > So that you could even skip this patch completely (and probably the
> > isConfigured() one too).
> >
> > Generally, I don't like API that have too many implicit behaviours and
> > prefer the ones where the caller has to chose the right overload to
> > call, so that the logic is clear in the calling place and is harder to
> > mis-uses, hence I might be biased. I'll let you decide really, I have already
> > pulled you in too many directions enough.
>
> I liked the version from v2 better at first. Now that the explicit one
> is there I actually like it because a) You get an error if you pass a
> non configured stream and b) it is clearer from the using code that
> different things can happen there. In the end both version would do the
> job. If no one else objects with a good argument I'd leave it like it is
> now.

Fine with me of course.

Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j

>
> Cheers,
> Stefan
>
> >
> > >
> > >  		controls[&controls::ScalerCrop] = ControlInfo(cropLimits.first,
> > >  							      cropLimits.second,
> > > --
> > > 2.43.0
> > >