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

[Kieran Bingham]

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 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 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 at 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
>