[libcamera-devel,v1,1/6] pipeline: raspberrypi: Avoid over-allocation for ISP Output 1
diff mbox series

Message ID 20220307124633.115452-2-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Efficient start/stop/start sequences
Related show

Commit Message

Naushir Patuck March 7, 2022, 12:46 p.m. UTC
The V4L2DeviceFormat structure for the ISP Output 1 node was a copy of what is
used ISP Output 1 node, but with the size changed. However, the plane size and
stride values were not updated. So there is a possibility that the buffer might
be over-sized for the requested resolution.

Fix this by only copying the relevent fields from the ISP Output 0
V4L2DeviceFormat structure, and let the device driver size the planes as needed.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Nicolas Dufresne via libcamera-devel March 10, 2022, 10:34 a.m. UTC | #1
Hi Naush

Thanks for this patch!

On Mon, 7 Mar 2022 at 12:46, Naushir Patuck via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> The V4L2DeviceFormat structure for the ISP Output 1 node was a copy of what is
> used ISP Output 1 node, but with the size changed. However, the plane size and
> stride values were not updated. So there is a possibility that the buffer might
> be over-sized for the requested resolution.
>
> Fix this by only copying the relevent fields from the ISP Output 0
> V4L2DeviceFormat structure, and let the device driver size the planes as needed.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 29bff9d6eee4..d604c473c26c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -844,11 +844,13 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>          * colour denoise will not run.
>          */
>         if (!output1Set) {
> -               V4L2DeviceFormat output1Format = format;
> +               V4L2DeviceFormat output1Format;
>                 constexpr Size maxDimensions(1200, 1200);
>                 const Size limit = maxDimensions.boundedToAspectRatio(format.size);
>
>                 output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);
> +               output1Format.colorSpace = format.colorSpace;
> +               output1Format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);
>
>                 LOG(RPI, Debug) << "Setting ISP Output1 (internal) to "
>                                 << output1Format.toString();
> --
> 2.25.1
>

Looks good to me, and we have of course been testing it with picamera2.

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

Thanks
David
Nicolas Dufresne via libcamera-devel March 13, 2022, 3:46 p.m. UTC | #2
Hi Naush,

Thank you for the patch.

On Mon, Mar 07, 2022 at 12:46:28PM +0000, Naushir Patuck via libcamera-devel wrote:
> The V4L2DeviceFormat structure for the ISP Output 1 node was a copy of what is
> used ISP Output 1 node, but with the size changed. However, the plane size and

Did you mean "of what is used by the ISP Output 0 node" ?

> stride values were not updated. So there is a possibility that the buffer might
> be over-sized for the requested resolution.
> 
> Fix this by only copying the relevent fields from the ISP Output 0

s/relevent/relevant/

> V4L2DeviceFormat structure, and let the device driver size the planes as needed.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 29bff9d6eee4..d604c473c26c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -844,11 +844,13 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	 * colour denoise will not run.
>  	 */
>  	if (!output1Set) {
> -		V4L2DeviceFormat output1Format = format;
> +		V4L2DeviceFormat output1Format;
>  		constexpr Size maxDimensions(1200, 1200);
>  		const Size limit = maxDimensions.boundedToAspectRatio(format.size);
>  
>  		output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);
> +		output1Format.colorSpace = format.colorSpace;
> +		output1Format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);
>  
>  		LOG(RPI, Debug) << "Setting ISP Output1 (internal) to "
>  				<< output1Format.toString();

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 29bff9d6eee4..d604c473c26c 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -844,11 +844,13 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	 * colour denoise will not run.
 	 */
 	if (!output1Set) {
-		V4L2DeviceFormat output1Format = format;
+		V4L2DeviceFormat output1Format;
 		constexpr Size maxDimensions(1200, 1200);
 		const Size limit = maxDimensions.boundedToAspectRatio(format.size);
 
 		output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);
+		output1Format.colorSpace = format.colorSpace;
+		output1Format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);
 
 		LOG(RPI, Debug) << "Setting ISP Output1 (internal) to "
 				<< output1Format.toString();