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

15077 diff mbox series

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@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Hi Naush

Thanks for this patch!

On Wed, 8 Dec 2021 at 09:42, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> 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@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  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);
>                         }
>                 }
>
> --
> 2.25.1
>

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@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@ideasonboard.com>

>                         }
>                 }
>  
> -- 
> 2.25.1
>