| Message ID | 20220307124633.115452-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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
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();
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();
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(-)