[libcamera-devel] [PATCH 2/2] pipeline: raspberrypi: Restrict the advertised maximum ISP output resolution

[Kieran Bingham]

Quoting Naushir Patuck (2021-12-08 09:42:11)
> Limit the advertised ISP output sizes available to the sensor resolution in
> PipelineHandlerRPi::generateConfiguration(). The user is free to configure a
> larger resolution than this, and this will work. However, this stops strange
> behavior in applications that use the V4L2 compatability layer to run, and
> request the largest possible advertised resolution, which is much larger than
> the sensor resolution.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 4479e732a645..6ee975e85567 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -531,10 +531,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>  
>         unsigned int rawCount = 0;
>         unsigned int outCount = 0;
> +       Size sensorSize = data->sensor_->resolution();
>         for (const StreamRole role : roles) {
>                 switch (role) {
>                 case StreamRole::Raw:
> -                       size = data->sensor_->resolution();
> +                       size = sensorSize;
>                         sensorFormat = findBestFormat(data->sensorFormats_, size, defaultRawBitDepth);
>                         pixelFormat = mbusCodeToPixelFormat(sensorFormat.mbus_code,
>                                                             BayerFormat::Packing::CSI2);
> @@ -547,7 +548,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>                         fmts = data->isp_[Isp::Output0].dev()->formats();
>                         pixelFormat = formats::NV12;
>                         /* Return the largest sensor resolution. */
> -                       size = data->sensor_->resolution();
> +                       size = sensorSize;
>                         bufferCount = 1;
>                         outCount++;
>                         break;
> @@ -600,11 +601,15 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,
>                                                 std::forward_as_tuple(format.second.begin(), format.second.end()));
>                         }
>                 } else {
> -                       /* Translate the V4L2PixelFormat to PixelFormat. */
> +                       /*
> +                        * Translate the V4L2PixelFormat to PixelFormat. Note that we
> +                        * limit the recommended largest ISP output size to match the
> +                        * sensor resolution.
> +                        */
>                         for (const auto &format : fmts) {
>                                 PixelFormat pf = format.first.toPixelFormat();
>                                 if (pf.isValid())
> -                                       deviceFormats[pf] = format.second;
> +                                       deviceFormats[pf].emplace_back(sensorSize);

This isn't adding to existing entries is it? I think I infer that this
is using emplace_back() to be able to add the sensorSize as entry to the
vector. Is it always guaranteed to be empty at this point otherwise?

<edit> Yes deviceFormats is local and only created in this function ;-)

I think it looks fine though.

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

>                         }
>                 }
>  
> -- 
> 2.25.1
>