[PATCH v3] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline

[Jacopo Mondi]

Hello Umang

On Thu, Sep 26, 2024 at 09:12:49PM GMT, Umang Jain wrote:
> Hi Jacopo,
>
> On 26/09/24 7:41 pm, Jacopo Mondi wrote:
> > Hi Umang
> >
> > On Thu, Sep 26, 2024 at 07:02:36PM GMT, Umang Jain wrote:
> > > It is possible that the maximum sensor size (returned by
> > > CameraSensor::resolution()) is not supported by the pipeline. In such
> > > cases, a filter function is required to filter out resolutions of the
> > > camera sensor, which cannot be supported by the pipeline.
> > >
> > > Introduce the filter function filterSensorResolution() in RkISP1Path
> > > class and use it for validate() and generateConfiguration(). The
> > > maximum sensor resolution is picked from that filtered set of
> > > resolutions instead of CameraSensor::resolution().
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > ---
> > > Changes v3:
> > > - None, just a resent over latest master
> > > - Split out from [v2,0/2] pipeline: rkisp1: Filter out sensor sizes not supported by the pipeline
> > >    so that, this can make independent progress.
> > > ---
> > >   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 51 ++++++++++++++++++-
> > >   src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  8 +++
> > >   2 files changed, 57 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > index c49017d1..2421d06c 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > @@ -126,12 +126,59 @@ void RkISP1Path::populateFormats()
> > >   	}
> > >   }
> > >
> > > +/**
> > > + * \brief Filter the sensor resolutions that can be supported
> > > + * \param[in] sensor The camera sensor
> > > + *
> > > + * This function retrieves all the sizes supported by the sensor and
> > > + * filters all the resolutions that can be supported on the pipeline.
> > > + * It is possible that the sensor's maximum output resolution is higher
> > > + * than the ISP maximum input. In that case, this function filters out all
> > > + * the resolution incapable of being supported and returns the maximum
> > > + * sensor resolution that can be supported by the pipeline.
> > > + *
> > > + * \return Maximum sensor size supported on the pipeline
> > > + */
> > > +Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)
> > > +{
> > > +	auto iter = sensorSizesMap_.find(sensor);
> > > +	if (iter != sensorSizesMap_.end() && !iter->second.empty())

Can iter->second.empty() happen ?

> > > +		return iter->second.back();
> > > +
> > > +	sensorSizesMap_.emplace(sensor, std::vector<Size>());
> > > +
> > > +	std::vector<Size> sensorSizes;
> > > +	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > > +	for (const auto it : mbusCodes) {
> > > +		std::vector<Size> sizes = sensor->sizes(it);
> > > +		for (Size sz : sizes)
> > Can't you filter out sizes that are larger than max sizes ?
> >
> > So you can populate sensorSizesMap_.second directly avoiding copies ?
>
> If I try to directly populate sensorSizesMap_ with the max size filter, it
> might have duplicated sizes entries (same size for different codes).
>
> That's why, I opted a temporary vector sensorSizes here, is used to collect
> all sizes for the formats that the sensor supports.
>
> Then later sorted, de-duplicated and filtered, before populating the
> sensorSizeMap_.second
>

I think you can do the same filtering/sorting directly on the vector
inside the map already.

What I mean is something like

Size RkISP1Path::filterSensorResolution(const CameraSensor *sensor)
{
	auto iter = sensorSizesMap_.find(sensor);
	if (iter != sensorSizesMap_.end())
		return iter->second.back();

	std::vector<Size> &sizes = sensorSizesMap_[sensor];
	for (unsigned int code : sensor->mbusCodes()) {
		for (const Size &size : sensor->sizes(code)) {
			if (size.width > maxResolution_.width ||
			    size.height > maxResolution_.height)
				continue;

			sizes.push_back(size);
		}
	}

	/* Sort in increasing order and remove duplicates. */
	std::sort(sizes.begin(), sizes.end());
	auto last = std::unique(sizes.begin(), sizes.end());
	sizes.erase(last, sizes.end());

	return sizes.back();
}

warning: compile tested only

Do you think we need the list of supported, unique and sorted formats
anywhere else ? Otherwise you could just store the max res and not the
whole vector.

I won't bikeshed too much on the name, but even if this function
indeed does filter the sensor's resolutions, what it does is to return
the maxium sensor resolution supported by the pipeline, so I would
have gone for something like RkISP1Path::maxResolution(sensor *). Up
to you.

Thanks
  j

> >
> > > +			sensorSizes.push_back(sz);
> > > +	}
> > Can't we build this map once at populateFormats() time and solely
>
> The map is only built once. Checkout the early return at the start of the
> function
> > return the max supported size when needed ?
>
> I don't think you can do at populateFormats() as it is called in init()
>
> And RkISP1Path::init() is called in match() for RkISP1 pipeline handler,
> before the camera is even created.
> >

Right, at init() time you don't have a sensor to operate with, fine
then!

> > > +
> > > +	std::sort(sensorSizes.begin(), sensorSizes.end());
> > > +
> > > +	/* Remove duplicates. */
> > > +	auto last = std::unique(sensorSizes.begin(), sensorSizes.end());
> > > +	sensorSizes.erase(last, sensorSizes.end());
> > > +
> > > +	/* Discard any sizes that the pipeline is unable to support. */
> > > +	for (auto sz : sensorSizes) {
> > > +		if (sz.width > maxResolution_.width ||
> > > +		    sz.height > maxResolution_.height)
> > > +			continue;
> > > +
> > > +		sensorSizesMap_[sensor].push_back(sz);
> > > +	}
> > > +
> > > +	return sensorSizesMap_[sensor].back();
> > > +}
> > > +
> > >   StreamConfiguration
> > >   RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> > >   				  StreamRole role)
> > >   {
> > >   	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > > -	const Size &resolution = sensor->resolution();
> > > +	Size resolution = filterSensorResolution(sensor);
> > >
> > >   	/* Min and max resolutions to populate the available stream formats. */
> > >   	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > > @@ -220,7 +267,7 @@ CameraConfiguration::Status RkISP1Path::validate(const CameraSensor *sensor,
> > >   						 StreamConfiguration *cfg)
> > >   {
> > >   	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> > > -	const Size &resolution = sensor->resolution();
> > > +	Size resolution = filterSensorResolution(sensor);
> > >
> > >   	const StreamConfiguration reqCfg = *cfg;
> > >   	CameraConfiguration::Status status = CameraConfiguration::Valid;
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > index 08edefec..9f75fe1f 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > > @@ -7,6 +7,7 @@
> > >
> > >   #pragma once
> > >
> > > +#include <map>
> > >   #include <memory>
> > >   #include <set>
> > >   #include <vector>
> > > @@ -63,6 +64,7 @@ public:
> > >
> > >   private:
> > >   	void populateFormats();
> > > +	Size filterSensorResolution(const CameraSensor *sensor);
> > >
> > >   	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
> > >
> > > @@ -77,6 +79,12 @@ private:
> > >   	std::unique_ptr<V4L2Subdevice> resizer_;
> > >   	std::unique_ptr<V4L2VideoDevice> video_;
> > >   	MediaLink *link_;
> > > +
> > > +	/*
> > > +	 * Map from camera sensors to the sizes (in increasing order),
> > > +	 * which are guaranteed to be supported by the pipeline.
> > > +	 */
> > > +	std::map<const CameraSensor *, std::vector<Size>> sensorSizesMap_;
> > >   };
> > >
> > >   class RkISP1MainPath : public RkISP1Path
> > > --
> > > 2.45.0
> > >
>