[v2,2/2] libcamera: formats: Deduplicate PixelFormatInfo::info() code
diff mbox series

Message ID 20250214161031.240481-3-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • Silence warning when creating a PixelFormatInfo from a null format
Related show

Commit Message

Stefan Klug Feb. 14, 2025, 4:09 p.m. UTC 
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.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/formats.cpp | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Laurent Pinchart Feb. 14, 2025, 4:26 p.m. UTC | #1 
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@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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);
>  }
>  
>  /**

Patch
diff mbox series 

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);
 }
 
 /**