[PATCH v3.1] fixup! libcamera: converter_v4l2_m2m: Improve crop bounds support

[Jacopo Mondi]

Hi Stefan

On Fri, Dec 13, 2024 at 10:26:05AM +0100, Stefan Klug wrote:
> Changes semantics of inputCropBounds to return null bounds if an
> unconfigured stream is provided. Return the device crop bounds if a
> nullptr is provided.
>
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>
> Hi Jacopo,
>
> I forgot to handle your comment in the v3. Sorry for that. This is my
> proposal. It is somewhere in the middle of your suggestions, but I think
> it makes the intent clearer.

I like isConfigured() as it allows the pipeline to behave accordingly
to the converter configuration.

I like less the three-way behaviour of
Converter::inputCropBounds(Stream *)

1) If called with a configured Stream it returns the input crop bounds
of that stream
2) If called with a nullptr it returns the default crop bounds
3) When called with a Stream it returns an empty Rectangle

To make it less confusing, instead of

	virtual std::pair<Rectangle, Rectangle> inputCropBounds(
				const Stream *stream = nullptr) = 0;

I would

	virtual std::pair<Rectangle, Rectangle> inputCropBounds() = 0;
	virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;

or maybe even just
	virtual std::pair<Rectangle, Rectangle> cropBounds() = 0;

So that pipeline handlers could


		std::pair<Rectangle, Rectangle> cropLimits;
		if (dewarper_->isConfigured(&data->mainPathStream_))
                        cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_);
                else
                        cropLimits = dewarper_->cropBounds();

Which I find clearer as an API than

		const Stream *stream = nullptr;
		if (dewarper_->isConfigured(&data->mainPathStream_))
			stream = &data->mainPathStream_;

		std::pair<Rectangle, Rectangle> cropLimits =
			dewarper_->inputCropBounds(stream);

Up to you

>
> I would like to keep the getCropBound() a static function as we
> discussed before.
>
> The commit message will also be updated for v4.
>
> Is this change ok for you?
>
> Best regards,
> Stefan
>
>  include/libcamera/internal/converter.h        |  4 +++-
>  .../internal/converter/converter_v4l2_m2m.h   |  1 +
>  src/libcamera/converter.cpp                   | 12 +++++++++++-
>  .../converter/converter_v4l2_m2m.cpp          | 19 ++++++++++++++++---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |  6 +++++-
>  5 files changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 9213ae5b9c33..465b0b0596d9 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -73,6 +73,7 @@ public:
>
>  	virtual int configure(const StreamConfiguration &inputCfg,
>  			      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> +	virtual bool isConfigured(const Stream *stream) const = 0;
>  	virtual int exportBuffers(const Stream *stream, unsigned int count,
>  				  std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
>
> @@ -83,7 +84,8 @@ public:
>  				 const std::map<const Stream *, FrameBuffer *> &outputs) = 0;
>
>  	virtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0;
> -	virtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream) = 0;
> +	virtual std::pair<Rectangle, Rectangle> inputCropBounds(
> +				const Stream *stream = nullptr) = 0;
>
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> index 89bd2878b190..175f0e1109a6 100644
> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h
> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
> @@ -54,6 +54,7 @@ public:
>
>  	int configure(const StreamConfiguration &inputCfg,
>  		      const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);
> +	bool isConfigured(const Stream *stream) const override;
>  	int exportBuffers(const Stream *stream, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index c3da162b7de7..74cea46cc709 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -169,6 +169,13 @@ Converter::~Converter()
>   * \return 0 on success or a negative error code otherwise
>   */
>
> +/**
> + * \fn Converter::isConfigured()
> + * \brief Check if a given stream is configured
> + * \param[in] stream The output stream
> + * \return True if the \a stream is configured or false otherwise
> + */
> +
>  /**
>   * \fn Converter::exportBuffers()
>   * \brief Export buffers from the converter device
> @@ -238,9 +245,12 @@ Converter::~Converter()
>   * this function should be called after the \a stream has been configured using
>   * configure().
>   *
> - * When called with an invalid \a stream, the function returns the default crop
> + * When called with nullptr \a stream this function returns the default crop
>   * bounds of the converter.
>   *
> + * When called with an invalid \a stream, this function returns a pair of null
> + * rectangles
> + *
>   * \return A pair containing the minimum and maximum crop bound in that order
>   */
>
> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
> index 6857472b29f2..4df253749feb 100644
> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp
> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
> @@ -559,6 +559,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
>  	return 0;
>  }
>
> +/**
> + * \copydoc libcamera::Converter::isConfigured
> + */
> +bool V4L2M2MConverter::isConfigured(const Stream *stream) const
> +{
> +	return streams_.find(stream) != streams_.end();
> +}
> +
>  /**
>   * \copydoc libcamera::Converter::exportBuffers
>   */
> @@ -595,11 +603,16 @@ int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)
>  std::pair<Rectangle, Rectangle>
>  V4L2M2MConverter::inputCropBounds(const Stream *stream)
>  {
> +	if (stream == nullptr)
> +		return inputCropBounds_;
> +
>  	auto iter = streams_.find(stream);
> -	if (iter != streams_.end())
> -		return iter->second->inputCropBounds();
> +	if (iter == streams_.end()) {
> +		LOG(Converter, Error) << "Invalid output stream";
> +		return {};
> +	}
>
> -	return inputCropBounds_;
> +	return iter->second->inputCropBounds();
>  }
>
>  /**
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 4dcc5a4fa6a1..ad162164df1a 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1281,8 +1281,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)
>  	ControlInfoMap::Map controls;
>
>  	if (dewarper_) {
> +		const Stream *stream = nullptr;
> +		if (dewarper_->isConfigured(&data->mainPathStream_))
> +			stream = &data->mainPathStream_;
> +
>  		std::pair<Rectangle, Rectangle> cropLimits =
> -			dewarper_->inputCropBounds(&data->mainPathStream_);
> +			dewarper_->inputCropBounds(stream);
>
>  		/*
>  		 * ScalerCrop is specified to be in Sensor coordinates.
> --
> 2.43.0
>