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

[Jacopo Mondi]

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
+ *

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.

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