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

[Stefan Klug]

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.

Cheers,
Stefan

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