[PATCH] libcamera: formats: Silence warning when creating a PixelFormatInfo from a null format

[Laurent Pinchart]

On Wed, Feb 12, 2025 at 01:00:21PM +0100, Stefan Klug wrote:
> On Wed, Feb 12, 2025 at 11:50:19AM +0000, Kieran Bingham wrote:
> > Quoting Stefan Klug (2025-02-12 11:29:42)
> > > 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.
> > > 
> > > While at it, remove code duplication by using
> > > PixelFormatInfo::info(const PixelFormat &format) to implement
> > > PixelFormatInfo::info(const V4L2PixelFormat &format).
> > 
> > Just to check the obvious - if there is a V4L2 format on a device that
> > we don't have mapped/supported in libcamera, will it still print 
> > 
> > WARN Formats formats.cpp:1006 Unsupported pixel format 0x23456789 ?
> > 
> > As long as that's the case, 
> 
> Well, we got that warning only for formats passed as PixelFormat. If the
> format was passed as V4L2PixelFormat there was no warning at all. Now we
> get the warning in both cases.

This would be worth mentioning in the commit message. Or possible split
in two patches, in case the additional warning for V4L2PixelFormat ends
up being too noisy and we need to revert that part ?

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

> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > ---
> > > 
> > > Hi all,
> > > 
> > > The attached patch 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. I believe that is sensible but maybe the
> > > warning was left off in the V4L2 case on purpose.
> > > 
> > > Best regards,
> > > Stefan
> > > 
> > >  src/libcamera/formats.cpp | 12 ++++--------
> > >  1 file changed, 4 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > > index bfcdfc08960d..df7413f58ba8 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)
> > 
> > Does the !isValid() check mean it's impossible to get here now? Or is
> > this check still necessary?
> 
> isValid() only checks if the fourcc code is 0. So the warning shows up
> if the format is valid but not supported by libcamera. Which was imho
> the intended case.
> 
> > > @@ -1021,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