[libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign sizes to roles

[Jacopo Mondi]

Hi Laurent

On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > Currently each RkISP1 path (main and self) generate a configuration
> > by bounding the sensor's resolution to their respective maximum output
> > aspect ratio and size.
> >
> > 	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> > 					   .boundedTo(resolution);
> >
> > In the case of self path, whose maximum size is 1920x1920, the generated
> > configuration could get assigned unusual sizes, as the result of the
> > above operation
>
> s/$/./
>
> >
> > As an example, with the imx258 sensor whose resolution is 4208x3118, the
> > resulting size for the Viewfinder use case is an unusual 1920x1423.
> >
> > Fix this by assigning to each role a desired output size:
> > - Use the sensor's resolution for StillCapture and Raw
> > - Use 1080p for Viewfinder and VideoRecording
>
> Will the pipeline handler crop the full sensor resolution to 4096x2304
> before scaling it down to 1080p, or will it just scale ? In the latter
> case, the pixel ratio won't be square, which isn't good.
>

I'm not sure I agree this isn't good. If we struggle to maintain the
sensor's aspect ratio we get:
1) weird and unusual sizes like the above reported 1920x1423
2) different results depending on which sensor is connected

I think a known 1080p size is a much more predictable behavior
for applications, regardless of the aspect ratio.

> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--
> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +
> >  3 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index ebc9bef8688a..60ab7380034c 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -164,6 +164,8 @@ public:
> >  	bool match(DeviceEnumerator *enumerator) override;
> >
> >  private:
> > +	static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> > +
> >  	RkISP1CameraData *cameraData(Camera *camera)
> >  	{
> >  		return static_cast<RkISP1CameraData *>(camera->_d());
> > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >
> >  	for (const StreamRole role : roles) {
> >  		bool useMainPath = mainPathAvailable;
> > +		Size size;
> >
> >  		switch (role) {
> >  		case StreamRole::StillCapture:
> >  			/* JPEG encoders typically expect sYCC. */
> >  			if (!colorSpace)
> >  				colorSpace = ColorSpace::Sycc;
> > +
> > +			size = data->sensor_->resolution();
> >  			break;
> >
> >  		case StreamRole::Viewfinder:
> > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  			 */
> >  			if (!colorSpace)
> >  				colorSpace = ColorSpace::Sycc;
> > +
> > +			size = kRkISP1PreviewSize;
> >  			break;
> >
> >  		case StreamRole::VideoRecording:
> >  			/* Rec. 709 is a good default for HD video recording. */
> >  			if (!colorSpace)
> >  				colorSpace = ColorSpace::Rec709;
> > +
> > +			size = kRkISP1PreviewSize;
> >  			break;
> >
> >  		case StreamRole::Raw:
> > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  			}
> >
> >  			colorSpace = ColorSpace::Raw;
> > +			size = data->sensor_->resolution();
> >  			break;
> >
> >  		default:
> > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> >  		}
> >
> >  		StreamConfiguration cfg =
> > -			path->generateConfiguration(data->sensor_.get(), role);
> > +			path->generateConfiguration(data->sensor_.get(), size, role);
> >  		if (!cfg.pixelFormat.isValid())
> >  			return nullptr;
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > index 5079b268c464..a27ac6fc35cb 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()
> >  }
> >
> >  StreamConfiguration
> > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,
> > +				  const Size &size,
> > +				  StreamRole role)
>
> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
> 				  StreamRole role)
>
> >  {
> >  	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
> >  	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);
> >
> > +	/* The desired stream size, bound to the max resolution. */
> > +	Size streamSize = size.boundedTo(maxResolution);
> > +
> >  	/* Create the list of supported stream formats. */
> >  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> >  	unsigned int rawBitsPerPixel = 0;
> > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> >  	StreamFormats formats(streamFormats);
> >  	StreamConfiguration cfg(formats);
> >  	cfg.pixelFormat = format;
> > -	cfg.size = maxResolution;
> > +	cfg.size = streamSize;
> >  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> >  	return cfg;
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > index bdf3f95b95e1..cd77957ee4a6 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> > @@ -41,6 +41,7 @@ public:
> >  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
> >
> >  	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> > +						  const Size &resolution,
> >  						  StreamRole role);
> >  	CameraConfiguration::Status validate(const CameraSensor *sensor,
> >  					     StreamConfiguration *cfg);
>
> --
> Regards,
>
> Laurent Pinchart