[libcamera-devel] libcamera: pipeline: simple: Reset format on capture side of converter

10423 diff mbox series

When configuring the converter, the format is first set on the output
side based on the format of the camera pipeline output, and then the
format is set on the capture side to match the desired stream
configuration. The format parameter passed to
V4L2VideoDevice::setFormat() uses the same variable for both calls,
which has the unwanted side effect of carrying plane configuration from
the output side to the capture side of the converter. In particular, the
stride or plane size requested on the capture side can become
unnecessarily large when converting to a format with a lower number of
bits per pixel (for instance converting YUYV to NV12).

Fix this by resetting the format variable before using it to configure
the capture side.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/simple/converter.cpp | 1 +
 1 file changed, 1 insertion(+)

Hi Laurent,

On Sun, Nov 15, 2020 at 11:16:07PM +0200, Laurent Pinchart wrote:
> When configuring the converter, the format is first set on the output
> side based on the format of the camera pipeline output, and then the
> format is set on the capture side to match the desired stream
> configuration. The format parameter passed to
> V4L2VideoDevice::setFormat() uses the same variable for both calls,
> which has the unwanted side effect of carrying plane configuration from
> the output side to the capture side of the converter. In particular, the
> stride or plane size requested on the capture side can become
> unnecessarily large when converting to a format with a lower number of
> bits per pixel (for instance converting YUYV to NV12).
> 
> Fix this by resetting the format variable before using it to configure
> the capture side.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/libcamera/pipeline/simple/converter.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index 57538ab08fcd..67e6e864aa0a 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -164,6 +164,7 @@ int SimpleConverter::configure(PixelFormat inputFormat, const Size &inputSize,
>  
>  	/* Set the pixel format and size on the output. */
>  	videoFormat = m2m_->capture()->toV4L2PixelFormat(cfg->pixelFormat);
> +	format = {};
>  	format.fourcc = videoFormat;
>  	format.size = cfg->size;
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Laurent,

On 15/11/2020 21:16, Laurent Pinchart wrote:
> When configuring the converter, the format is first set on the output
> side based on the format of the camera pipeline output, and then the
> format is set on the capture side to match the desired stream
> configuration. The format parameter passed to
> V4L2VideoDevice::setFormat() uses the same variable for both calls,
> which has the unwanted side effect of carrying plane configuration from
> the output side to the capture side of the converter. In particular, the
> stride or plane size requested on the capture side can become
> unnecessarily large when converting to a format with a lower number of
> bits per pixel (for instance converting YUYV to NV12).
> 
> Fix this by resetting the format variable before using it to configure
> the capture side.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hrm, came here to say, perhaps we should use a separate/new variable
then ... but actually - I don't think that is necessary.

Re-initialising it is just the same.

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

> ---
>  src/libcamera/pipeline/simple/converter.cpp | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index 57538ab08fcd..67e6e864aa0a 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -164,6 +164,7 @@ int SimpleConverter::configure(PixelFormat inputFormat, const Size &inputSize,
>  
>  	/* Set the pixel format and size on the output. */
>  	videoFormat = m2m_->capture()->toV4L2PixelFormat(cfg->pixelFormat);
> +	format = {};
>  	format.fourcc = videoFormat;
>  	format.size = cfg->size;
>  
>