[libcamera-devel,v3,2/4] libcamera: rkisp1: Assign sizes to roles
diff mbox series

Message ID 20230307114804.42291-3-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Fix generateConfiguration
Related show

Commit Message

Jacopo Mondi March 7, 2023, 11:48 a.m. UTC 
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

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

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@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(-)

Comments

Laurent Pinchart March 19, 2023, 8 p.m. UTC | #1 
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.

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@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);
Jacopo Mondi March 20, 2023, 7:21 a.m. UTC | #2 
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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@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
Laurent Pinchart March 20, 2023, 11:28 p.m. UTC | #3 
Hi Jacopo,

On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:
> > 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

That's not great indeed.

> 2) different results depending on which sensor is connected

That's to be expected, as sensors have different capabilities :-)

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

Unless the application explicitly requests a non-square pixel ratio,
defaulting to 1920x1080 with non-square pixels *really* bad. There are
multiple options in this case, starting from a 4208x3118 sensor
resolution:

- Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect
  ratio
- Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect
  ratio

Both will produce square pixels.

> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@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);
Jacopo Mondi March 21, 2023, 4:35 p.m. UTC | #4 
Hi Laurent

On Tue, Mar 21, 2023 at 01:28:02AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:
> > On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:
> > > 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
>
> That's not great indeed.
>
> > 2) different results depending on which sensor is connected
>
> That's to be expected, as sensors have different capabilities :-)
>
> > I think a known 1080p size is a much more predictable behavior
> > for applications, regardless of the aspect ratio.
>
> Unless the application explicitly requests a non-square pixel ratio,
> defaulting to 1920x1080 with non-square pixels *really* bad. There are
> multiple options in this case, starting from a 4208x3118 sensor
> resolution:
>
> - Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect
>   ratio

Thanks for clarifying this.

I'll add a small patch to the series to crop at the resizer sink pad.
In this way the aspect ratio should be maintained before downscaling

By default, the pipeline handler when asked for 1080p selects a
smaller sensor size than the full resolution, but to validate your
question I hacked it to always use full res and the result I have is:

rkisp1_path.cpp:331 Configured main resizer input pad with 4208x3120-YUYV8_2X8 crop (0, 0)/4208x2368
rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8

Without the hack, a size of 2104x1560 is selected from the sensor,
which gets cropped to 2104x1184 before getting downscaled to 1920x1080

rkisp1_path.cpp:331 Configured main resizer input pad with 2104x1560-YUYV8_2X8 crop (0, 0)/2104x1184
rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8

Would it work if I add that patch to the series before this one ?

> - Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect
>   ratio
>
> Both will produce square pixels.
>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@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
Laurent Pinchart June 5, 2023, 2:11 p.m. UTC | #5 
Hi Jacopo,

On Tue, Mar 21, 2023 at 05:35:53PM +0100, Jacopo Mondi wrote:
> On Tue, Mar 21, 2023 at 01:28:02AM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:
> > > On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:
> > > > 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
> >
> > That's not great indeed.
> >
> > > 2) different results depending on which sensor is connected
> >
> > That's to be expected, as sensors have different capabilities :-)
> >
> > > I think a known 1080p size is a much more predictable behavior
> > > for applications, regardless of the aspect ratio.
> >
> > Unless the application explicitly requests a non-square pixel ratio,
> > defaulting to 1920x1080 with non-square pixels *really* bad. There are
> > multiple options in this case, starting from a 4208x3118 sensor
> > resolution:
> >
> > - Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect
> >   ratio
> 
> Thanks for clarifying this.
> 
> I'll add a small patch to the series to crop at the resizer sink pad.
> In this way the aspect ratio should be maintained before downscaling

Sounds good :-)

> By default, the pipeline handler when asked for 1080p selects a
> smaller sensor size than the full resolution, but to validate your
> question I hacked it to always use full res and the result I have is:
> 
> rkisp1_path.cpp:331 Configured main resizer input pad with 4208x3120-YUYV8_2X8 crop (0, 0)/4208x2368
> rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8

1920x1080 is exactly 16:9, while 4208x2368 is very close but not quite.
The best option in this case would be to crop to 4192x2358. This is 16
pixels less than the full active resolution on the horizontal direction,
and I think this is by design.

ISP processing blocks often process pixels based on the value of their
neighbours. This means that pixels too close to the edges are special
cases, as the neighbours needed by the processing may be outside of the
image. ISPs need to work around that, to still output a somewhat
meaningful value for pixels close to the edges. Depending on the ISP
architecture, the processing blocks will for instance duplicate the
value of the edge pixels, or mirror the image around the edge. That's a
best effort approach, and it means that the pixels close to the edges
on the ISP output will likely be of lower quality than the other pixels.
They are thus often cropped out from the output.

Camera sensors take this into account, and include additional pixels on
all edges to account for the ISP processing. 8 lines and columns on each
side is a common value. It is thus not surprising that the best image
width before downscaling to 1920x1080 is exactly 16 pixels less than the
sensor output.

The best ISP input to generate 1920x1080 is thus (4192+16)x(2358+16) =
4208x2374. On the scaler input, the ISP should be configured to crop 8
pixels on each side, producing 4192x2358. This can then be downscaled to
1920x1080 for a 16:9 image with perfectly square pixels.

There's no need to take all this into account in this patch series, but
we should at some point. We will possibly need helpers to perform those
calculations, and I expect they will differ between different ISPs.

Note that producing 4:3 images should go through the same type of
calculation. I'm however slightly annoyed there, as 4208x3120 is close
to 4:3, so I would have expected (4208-16)x(3120-16) to be exactly 4:3.
It isn't. The best option seems to be (4208-72)x(3120-18) = 4136x3102,
which has lots of horizontal cropping.

> Without the hack, a size of 2104x1560 is selected from the sensor,
> which gets cropped to 2104x1184 before getting downscaled to 1920x1080
> 
> rkisp1_path.cpp:331 Configured main resizer input pad with 2104x1560-YUYV8_2X8 crop (0, 0)/2104x1184
> rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8
> 
> Would it work if I add that patch to the series before this one ?

Yes, that's fine.

> > - Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect
> >   ratio
> >
> > Both will produce square pixels.
> >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@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);

Patch
diff mbox series 

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)
 {
 	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);