[libcamera-devel] [PATCH 2/3] pipeline: rkisp1: Fix validation when sensor max is larger than ISP input

[Jacopo Mondi]

Hi Paul

On Tue, Jan 16, 2024 at 06:17:53PM +0900, Paul Elder via libcamera-devel wrote:
> If the maximum sensor output size is larger than the maximum ISP input
> size, the maximum sensor size could be selected and the pipeline would
> fail with an EPIPE. Fix this by clamping the sensor size to the ISP
> input size at validation time.
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 17 ++++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 26 +++++++++----------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  4 ++-
>  3 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 586b46d64..fb67ba8f4 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1202,11 +1202,24 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  	if (param_->open() < 0)
>  		return false;
>
> +	/*
> +	 * Retrieve ISP maximum input size for config validation in the path
> +	 * classes
> +	 */
> +	Size ispMaxInSize = { 0, 0 };
> +	V4L2Subdevice::Formats ispFormats = isp_->formats(0);
> +	for (const auto &[mbus, sizes] : ispFormats) {
> +		for (const auto &size : sizes) {
> +			if (ispMaxInSize < size.max)
> +				ispMaxInSize = size.max;
> +		}
> +	}
> +

I presume this could even be kept as a constant in the pipeline
handler ? Sure, retrieving it a run time is probably better

>  	/* Locate and open the ISP main and self paths. */
> -	if (!mainPath_.init(media_))
> +	if (!mainPath_.init(media_, ispMaxInSize))
>  		return false;
>
> -	if (hasSelfPath_ && !selfPath_.init(media_))
> +	if (hasSelfPath_ && !selfPath_.init(media_, ispMaxInSize))
>  		return false;
>
>  	mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index b62b645ca..eaab7e857 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -62,7 +62,7 @@ RkISP1Path::RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  {
>  }
>
> -bool RkISP1Path::init(MediaDevice *media)
> +bool RkISP1Path::init(MediaDevice *media, const Size &ispMaxInSize)
>  {
>  	std::string resizer = std::string("rkisp1_resizer_") + name_ + "path";
>  	std::string video = std::string("rkisp1_") + name_ + "path";
> @@ -76,6 +76,7 @@ bool RkISP1Path::init(MediaDevice *media)
>  		return false;
>
>  	populateFormats();
> +	ispMaxInSize_ = ispMaxInSize;
>
>  	link_ = media->link("rkisp1_isp", 2, resizer, 0);
>  	if (!link_)
> @@ -172,7 +173,9 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>  		/* Add all the RAW sizes the sensor can produce for this code. */
>  		for (const auto &rawSize : sensor->sizes(mbusCode)) {
>  			if (rawSize.width > maxResolution_.width ||
> -			    rawSize.height > maxResolution_.height)
> +			    rawSize.height > maxResolution_.height ||
> +			    rawSize.width > ispMaxInSize_.width ||
> +			    rawSize.height > ispMaxInSize_.height)

This filters out the streamFormats that are reported to the
application, and I think this should be maybe only limited by the
video capture device output sizes, not the isp input sizes ?

I do however see a few lines above in generateConfiguration()

	const Size &resolution = sensor->resolution();

	/* Min and max resolutions to populate the available stream formats. */
	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
					   .boundedTo(resolution);
	Size minResolution = minResolution_.expandedToAspectRatio(resolution);

Should `resolution` be replaced by the maximum sensor resolution
smaller than the isp max input size (maybe computed in
populateFormats() ? )


>  				continue;
>
>  			streamFormats[format].push_back({ rawSize, rawSize });
> @@ -275,8 +278,9 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  	if (!found)
>  		cfg->pixelFormat = isRaw ? rawFormat : formats::NV12;
>
> -	Size minResolution;
> -	Size maxResolution;
> +	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> +					   .boundedTo(resolution);
> +	Size minResolution = minResolution_.expandedToAspectRatio(resolution);

Should you replace 'resolution' with the above mentioned 'larger
sensor's resolution which is smaller than the ISP input size' ?

>
>  	if (isRaw) {
>  		/*
> @@ -287,16 +291,10 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
>  		V4L2SubdeviceFormat sensorFormat =
>  			sensor->getFormat({ mbusCode }, cfg->size);
>
> -		minResolution = sensorFormat.size;
> -		maxResolution = sensorFormat.size;
> -	} else {
> -		/*
> -		 * Adjust the size based on the sensor resolution and absolute
> -		 * limits of the ISP.
> -		 */
> -		minResolution = minResolution_.expandedToAspectRatio(resolution);
> -		maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> -					      .boundedTo(resolution);
> +		if (!sensorFormat.size.isNull()) {
> +			minResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> +			maxResolution = sensorFormat.size.boundedTo(ispMaxInSize_);
> +		}
>  	}
>
>  	cfg->size.boundTo(maxResolution);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index cd77957ee..c96bd5557 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -35,7 +35,7 @@ public:
>  	RkISP1Path(const char *name, const Span<const PixelFormat> &formats,
>  		   const Size &minResolution, const Size &maxResolution);
>
> -	bool init(MediaDevice *media);
> +	bool init(MediaDevice *media, const Size &ispMaxInSize);

I would creat the path with a pointer to the isp subdev instead of
passing the value here as this doesn't smell like a function parameter
but a system's characterstics . Not a big deal though.

>
>  	int setEnabled(bool enable) { return link_->setEnabled(enable); }
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> @@ -77,6 +77,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> resizer_;
>  	std::unique_ptr<V4L2VideoDevice> video_;
>  	MediaLink *link_;
> +
> +	Size ispMaxInSize_;
>  };
>
>  class RkISP1MainPath : public RkISP1Path
> --
> 2.39.2
>