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

[David Plowman]

Hi Jacopo, everyone

Thanks very much for fixing this, it's become a somewhat annoying
message since I last updated!!

One related question... I also keep getting an "Unsupported pixel
format" message from formats.cpp:838 as well (one for each occurrence
of this message, it seems). Is that going to need a separate fix?

Thanks!

David

On Wed, 10 Aug 2022 at 07:59, Umang Jain via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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();
> >>      }
> >>