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

[Umang Jain]

Hello,

On 8/10/22 06:28, Laurent Pinchart via libcamera-devel wrote:
> 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.


+1

>
>> ---
>>   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 ?


Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

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