[libcamera-devel] [PATCH v2] libcamera: formats: Fix warning for unkown V4L2 pixfmt

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

s/unkown/unknown/ in the subject line.

On Tue, Aug 09, 2022 at 04:49:35PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Commit f25ad4a2b16b ("libcamera: formats: Reimplement V4L2
> PixelFormatInfo::info()") changed the PixelFormatInfo::info(const
> V4L2PixelFormat &format) function overload to:
> 
> 	return info(format.toPixelFormat());
> 
> As part of the series that contains such commit, the PixelFormatInfo for the
> pixel format applied to a video device is now retrieved at
> V4L2VideoDevice::open() time. Some video devices register formats not
> available to applications, for example metadata formats or, in the case
> of ISP devices, formats to describe the ISP statistics and parameters.
> 
> This causes the
> 
> 	format.toPixelFormat()
> 
> call to output a WARN message, which spams the log and unnecessary alert

s/unnecessary alert/unnecessarily alerts/

> the users.
> 
> Augment V4L2PixelFormat::toPixelFormat() with an optional argument to
> suppress warnings in the case a V4L2 pixel format is not known, to
> restore the behaviour preceding commit f25ad4a2b16b and returns an
> invalid PixelFormatInfo without outputting any unnecessary warning
> message.
> 
> Fixes: f25ad4a2b16b ("libcamera: formats: Reimplement V4L2 PixelFormatInfo::info()")
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> 
> This is the version suggested by Laurent of
> [PATCH] libcamera: formats: Search V4L2 format info on map
> 
> I don't like it too much but there's a slight performance improvement.

I wouldn't like it if V4L2PixelFormat was a public class, but for an
internal class I'm OK with this.

> ---
>  include/libcamera/internal/v4l2_pixelformat.h |  2 +-
>  src/libcamera/formats.cpp                     | 10 +++++++++-
>  src/libcamera/v4l2_pixelformat.cpp            | 16 ++++++++++++----
>  3 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h
> index 34d283db44f4..44439fff73eb 100644
> --- a/include/libcamera/internal/v4l2_pixelformat.h
> +++ b/include/libcamera/internal/v4l2_pixelformat.h
> @@ -45,7 +45,7 @@ public:
>  	std::string toString() const;
>  	const char *description() const;
> 
> -	PixelFormat toPixelFormat() const;
> +	PixelFormat toPixelFormat(bool warn = true) const;
>  	static const std::vector<V4L2PixelFormat> &
>  	fromPixelFormat(const PixelFormat &pixelFormat);
> 
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 0bd0e09ae44c..f5769c489e16 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -852,7 +852,15 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>   */
>  const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
>  {
> -	return info(format.toPixelFormat());
> +	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;
>  }
> 
>  /**
> diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp
> index 3590fb735011..26c9f02c759f 100644
> --- a/src/libcamera/v4l2_pixelformat.cpp
> +++ b/src/libcamera/v4l2_pixelformat.cpp
> @@ -294,15 +294,23 @@ const char *V4L2PixelFormat::description() const
> 
>  /**
>   * \brief Convert the V4L2 pixel format to the corresponding PixelFormat
> + * \param[in] warn Flag to control the warning message if the format is not
> + * supported. Default to true, set to false to suppress warning message.

I'd drop the "default to true" as doxygen will show the default value.

 * \param[in] warn When true, log a warning if the V4L2 pixel format isn't known

> + *
> + * Users of this function might try to convert a V4L2PixelFormat to a PixelFormat
> + * just to check if the format is supported or not. In that case, they can suppress
> + * the warning message setting the \a warn argument to false not not pollute the log

s/setting/by setting/
s/not not/to not/

The implementation looks fine, so

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

I've also send a Reviewed-by tag to v1 ("[PATCH] libcamera: formats:
Search V4L2 format info on map") in case it ends up being favoured.

Any second opinion to break the tie ?

> + * with unnecessary warning messages.
> + *
>   * \return The PixelFormat corresponding to the V4L2 pixel format
>   */
> -PixelFormat V4L2PixelFormat::toPixelFormat() const
> +PixelFormat V4L2PixelFormat::toPixelFormat(bool warn) const
>  {
>  	const auto iter = vpf2pf.find(*this);
>  	if (iter == vpf2pf.end()) {
> -		LOG(V4L2, Warning)
> -			<< "Unsupported V4L2 pixel format "
> -			<< toString();
> +		if (warn)
> +			LOG(V4L2, Warning) << "Unsupported V4L2 pixel format "
> +					   << toString();
>  		return PixelFormat();
>  	}
> 

-- 
Regards,

Laurent Pinchart