[PATCH v2 2/2] libcamera: formats: Deduplicate PixelFormatInfo::info() code

[Laurent Pinchart]

Hi Stefan,

On Fri, Feb 14, 2025 at 05:09:48PM +0100, Stefan Klug wrote:
> Use PixelFormatInfo::info(const PixelFormat &format) to implement
> PixelFormatInfo::info(const V4L2PixelFormat &format).
> 
> This has one noteworthy side effect: If info(V4L2PixelFormat &format) is
> called with a valid but unsupported (by libcamera) format, we now get
> the same warning as in the info(PixelFormat &format) case.

Now that you state it that way, can it actually ever happen ? For the
warning to be printed, we would need a V4L2PixelFormat that the vpf2pf
table (in v4l2_pixelformat.cpp) knows about, but that is unknown to
pixelFormatInfo (in formats.cpp). Give that every entry in vpf2pf
requires a PixelFormat, I would expect it to be present in
pixelFormatInfo.

If we hit the warning, it means there would be a clear bug in libcamera.
I initially thought this patch could introduce verbose warnings that we
may not want to see, and may need to be reverted in the future. It now
sounds like the warning will only be printed when something goes really
wrong. That lowers my concern.

You may want to indicate in the commit message that we expect the
warning only in case of a bug in the libcamera core. Apart from that,
the patch looks good, you can keep my Rb tag.

> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/formats.cpp | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index b4518e61d04a..df7413f58ba8 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -1024,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>  const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
>  {
>  	PixelFormat pixelFormat = format.toPixelFormat(false);
> -	if (!pixelFormat.isValid())
> -		return pixelFormatInfoInvalid;
> -
> -	const auto iter = pixelFormatInfo.find(pixelFormat);
> -	if (iter == pixelFormatInfo.end())
> -		return pixelFormatInfoInvalid;
> -
> -	return iter->second;
> +	return info(pixelFormat);
>  }
>  
>  /**

-- 
Regards,

Laurent Pinchart