[v2,1/2] libcamera: formats: Silence warning when creating a PixelFormatInfo from a null format
diff mbox series

Message ID 20250214161031.240481-2-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 
Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it
also produces the following, possibly confusing, warning:

WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000

Silence the warning in PixelFormatInfo::info() in case the format is
invalid.

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 | 3 +++
 1 file changed, 3 insertions(+)

Comments

Laurent Pinchart Feb. 14, 2025, 4:27 p.m. UTC | #1 
Hi Stefan,

Thank you for the patch.

On Fri, Feb 14, 2025 at 05:09:47PM +0100, Stefan Klug wrote:
> Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it
> also produces the following, possibly confusing, warning:
> 
> WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000
> 
> Silence the warning in PixelFormatInfo::info() in case the format is
> invalid.

Sleeping over this made me wonder where/when you hit this warning. Is
there a valid use case to call this function with a default-constructed
pixel format ?

> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index bfcdfc08960d..b4518e61d04a 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
>   */
>  const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
>  {
> +	if (!format.isValid())
> +		return pixelFormatInfoInvalid;
> +
>  	const auto iter = pixelFormatInfo.find(format);
>  	if (iter == pixelFormatInfo.end()) {
>  		LOG(Formats, Warning)
Stefan Klug Feb. 14, 2025, 4:42 p.m. UTC | #2 
Hi Laurent,

On Fri, Feb 14, 2025 at 06:27:33PM +0200, Laurent Pinchart wrote:
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Fri, Feb 14, 2025 at 05:09:47PM +0100, Stefan Klug wrote:
> > Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it
> > also produces the following, possibly confusing, warning:
> > 
> > WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000
> > 
> > Silence the warning in PixelFormatInfo::info() in case the format is
> > invalid.
> 
> Sleeping over this made me wonder where/when you hit this warning. Is
> there a valid use case to call this function with a default-constructed
> pixel format ?

At least I can explain where it happens. Let's see if that is valid...
The V4L2M2MStream has a logPrefix() function which calls
stream_->configuration().toString(). So when you log something early in
the lifetime of the converter (even if it is a debug message that
doesn't show up) that warning shows up. I was puzzled by this until I
realized it is just normal flow. And I see no better way to solve it.

Best regards,
Stefan

> 
> > 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 | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index bfcdfc08960d..b4518e61d04a 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> >   */
> >  const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> >  {
> > +	if (!format.isValid())
> > +		return pixelFormatInfoInvalid;
> > +
> >  	const auto iter = pixelFormatInfo.find(format);
> >  	if (iter == pixelFormatInfo.end()) {
> >  		LOG(Formats, Warning)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Feb. 14, 2025, 4:51 p.m. UTC | #3 
On Fri, Feb 14, 2025 at 05:42:16PM +0100, Stefan Klug wrote:
> On Fri, Feb 14, 2025 at 06:27:33PM +0200, Laurent Pinchart wrote:
> > On Fri, Feb 14, 2025 at 05:09:47PM +0100, Stefan Klug wrote:
> > > Calling PixelFormat().toString() correctly returns "0x0-<INVALID>" but it
> > > also produces the following, possibly confusing, warning:
> > > 
> > > WARN Formats formats.cpp:1006 Unsupported pixel format 0x00000000
> > > 
> > > Silence the warning in PixelFormatInfo::info() in case the format is
> > > invalid.
> > 
> > Sleeping over this made me wonder where/when you hit this warning. Is
> > there a valid use case to call this function with a default-constructed
> > pixel format ?
> 
> At least I can explain where it happens. Let's see if that is valid...
> The V4L2M2MStream has a logPrefix() function which calls
> stream_->configuration().toString(). So when you log something early in
> the lifetime of the converter (even if it is a debug message that
> doesn't show up) that warning shows up. I was puzzled by this until I
> realized it is just normal flow. And I see no better way to solve it.

This was introduced in

commit cc3a3c46a5ae4353b7bc9fe740521cef1008c998
Author: Umang Jain <umang.jain@ideasonboard.com>
Date:   Mon Jun 24 19:18:59 2024 +0530

    libcamera: converter: Replace usage of stream index by Stream pointer

with

 std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const
 {
-        return "stream" + std::to_string(index_);
+        return stream_->configuration().toString();
 }

Usage of the stream configuration there is not nice :-/ We could have
two streams with the same configuration, and wouldn't be able to
differentiate them in the logs. Also, the log identification of a stream
can change over the lifetime of the object. The log prefix is meant to
differentiate between class instances. Can we do something better here ?

> > > 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 | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index bfcdfc08960d..b4518e61d04a 100644
> > > --- a/src/libcamera/formats.cpp
> > > +++ b/src/libcamera/formats.cpp
> > > @@ -1001,6 +1001,9 @@ const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
> > >   */
> > >  const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
> > >  {
> > > +	if (!format.isValid())
> > > +		return pixelFormatInfoInvalid;
> > > +
> > >  	const auto iter = pixelFormatInfo.find(format);
> > >  	if (iter == pixelFormatInfo.end()) {
> > >  		LOG(Formats, Warning)

Patch
diff mbox series 

diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index bfcdfc08960d..b4518e61d04a 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -1001,6 +1001,9 @@  const std::map<PixelFormat, PixelFormatInfo> pixelFormatInfo{
  */
 const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)
 {
+	if (!format.isValid())
+		return pixelFormatInfoInvalid;
+
 	const auto iter = pixelFormatInfo.find(format);
 	if (iter == pixelFormatInfo.end()) {
 		LOG(Formats, Warning)