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

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

Commit Message

Jacopo Mondi Feb. 22, 2023, 3:19 p.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>
---
 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

Paul Elder March 3, 2023, 8:34 a.m. UTC | #1 
On Wed, Feb 22, 2023 at 04:19:15PM +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
> 
> 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>

> ---
>  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 569fb8ecb629..05a7ba03b2d2 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -162,6 +162,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> +	static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> +
>  	RkISP1CameraData *cameraData(Camera *camera)
>  	{
>  		return static_cast<RkISP1CameraData *>(camera->_d());
> @@ -634,12 +636,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:
> @@ -649,12 +654,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:
> @@ -665,6 +674,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			}
>  
>  			colorSpace = ColorSpace::Raw;
> +			size = data->sensor_->resolution();
>  			break;
>  
>  		default:
> @@ -683,7 +693,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);
> -- 
> 2.39.0
>
Kieran Bingham March 7, 2023, 8:53 a.m. UTC | #2 
Hi Jacopo,

Quoting Jacopo Mondi via libcamera-devel (2023-02-22 15:19:15)
> 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>
> ---
>  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 569fb8ecb629..05a7ba03b2d2 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -162,6 +162,8 @@ public:
>         bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> +       static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> +
>         RkISP1CameraData *cameraData(Camera *camera)
>         {
>                 return static_cast<RkISP1CameraData *>(camera->_d());
> @@ -634,12 +636,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();

I expect applications often want to 'choose' what size still they
handle, but this is certainly a reasonable default for stills.

>                         break;
>  
>                 case StreamRole::Viewfinder:
> @@ -649,12 +654,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:
> @@ -665,6 +674,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>                         }
>  
>                         colorSpace = ColorSpace::Raw;
> +                       size = data->sensor_->resolution();

And certainly correct expected for RAW.


>                         break;
>  
>                 default:
> @@ -683,7 +693,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>                 }
>  

It's a little bit non-obvious that the sizes above can now be further
refined by this next call. Not problematic, but it looks like the size
has been 'chosen' already above - but that's just the starting
point/default.

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

Though it's only about the common bounding sizes so I think it all looks
pretty reasonable.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         /* 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);
> -- 
> 2.39.0
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 569fb8ecb629..05a7ba03b2d2 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -162,6 +162,8 @@  public:
 	bool match(DeviceEnumerator *enumerator) override;
 
 private:
+	static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
+
 	RkISP1CameraData *cameraData(Camera *camera)
 	{
 		return static_cast<RkISP1CameraData *>(camera->_d());
@@ -634,12 +636,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:
@@ -649,12 +654,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:
@@ -665,6 +674,7 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 			}
 
 			colorSpace = ColorSpace::Raw;
+			size = data->sensor_->resolution();
 			break;
 
 		default:
@@ -683,7 +693,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);