[libcamera-devel] New warnings for pixel formats on Raspberry Pi platforms
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 8 17:11:11 CEST 2022
Hi Jacopo,
On Mon, Aug 08, 2022 at 04:38:55PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 08, 2022 at 05:10:42PM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 08, 2022 at 12:12:49PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > I have identified the call path that prints the warning out.
> > >
> > > When a video device is opened, we initialize a PixelFormatInfo here
> > > https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/v4l2_videodevice.cpp#n748
> > >
> > > Commit f25ad4a2b16b ("libcamera: formats: Reimplement V4L2 PixelFormatInfo::info()")
> > > https://git.libcamera.org/libcamera/libcamera.git/commit/src/libcamera?id=f25ad4a2b16bc8a0d44e8ca11c46f185b47c34b5
> > >
> > > re-implements PixelFormatInfo::info(const V4L2PixelFormat &format)
> > > by calling V4L2PixelFormat::toPixelFormat() which fails if the V4L2
> > > pixel format is not part of
> > > std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf.
> > >
> > > Before that commit we silently returned an invalid pixel format and
> > > that was it
> > >
> > > - const auto &info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),
> > > - [format](auto pair) {
> > > - return pair.second.v4l2Formats.single == format ||
> > > - pair.second.v4l2Formats.multi == format;
> > > - });
> > > - if (info == pixelFormatInfo.end())
> > > - return pixelFormatInfoInvalid;
> > >
> > > To avoid printing warnings out we should know when it's legit not have
> > > a format associated to a pixel format (becuase it's for internal use
> > > only, like all metadata formats for example), the only way I can think
> > > of is to enumerate the "special" formats, which is indeed a burden to
> > > maintain...
> > >
> > > Otherwise, we can partially revert f25ad4a2b16b and implement
> > > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> > > as was originally proposed in https://patchwork.libcamera.org/patch/16871/
> > >
> > > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> > > {
> > > const auto &info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),
> > > [&format](auto &pair) {
> > > const auto &formats = pair.second.v4l2Formats;
> > > return std::find(formats.begin(), formats.end(), format)
> > > != formats.end();
> > > });
> > > if (info == pixelFormatInfo.end())
> > > return pixelFormatInfoInvalid;
> > >
> > > return info->second;
> > > }
> > >
> > > Of course this would be my preference.
> >
> > Another option would be to add a parameter to the
> > V4L2PixelFormat::toPixelFormat() function to suppress warnings (with a
> > default value that keeps the warning enabled), and implement
> > PixelFormatInfo::info() as
>
> It feels a bit hackish to me :/
> What would the rule be for other callers to use true or false ? Do we
> want to allow them to suppress warnings even when they shouldn't ?
They shouldn't. If this were a public API, I'd be against it. For an
internal API, I think we have more leeway. I agree it's not perfect
though, but on the other hand, handling all the V4L2PixelFormat to
PixelFormat conversion through V4L2PixelFormat::toPixelFormat() is nice
as it will ensure all lookups go through the same vpf2pf map instead of
having two different sources of information.
> > const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)
> > {
> > PixelFormat pixelFormat = format.toPixelFormat(false);
> > if (!pixelFormat.isValid())
> > return pixelFormatInfoInvalid;
> >
> > const auto iter = pixelFormatInfo.find(format);
> > if (iter == pixelFormatInfo.end())
> > return pixelFormatInfoInvalid;
> >
> > return iter->second;
> > }
> >
> > That would be more efficient than the double std::find() on std::vector.
>
> Considering the size of the vectors this feels negligible ?
It's not too vectors, my bad, the first one (pixelFormatInfo) is a map.
The point is two linear searches, with the first one over a large
container and the second one small.
> > What do you think ?
> >
> > > On Mon, Aug 08, 2022 at 09:11:54AM +0100, Kieran Bingham wrote:
> > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-08 08:42:07)
> > > > > Hi Naush
> > > > >
> > > > > On Fri, Aug 05, 2022 at 03:26:16PM +0100, Naushir Patuck via libcamera-devel wrote:
> > > > > > Hi,
> > > > > >
> > > > > > A recent change (commit f25ad4a2b16b libcamera: formats: Reimplement V4L2
> > > > > > PixelFormatInfo::info()) has started throwing WARN messages on the
> > > > > > Raspberry Pi platform:
> > > > > >
> > > > > > [24:06:09.085738817] [8392] INFO Camera camera_manager.cpp:293 libcamera v0.0.0
> > > > > > [24:06:09.196596920] [8408] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format SENS
> > > > > > [24:06:09.196729049] [8408] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > > > [24:06:09.198112797] [8408] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format BSTA
> > > > > > [24:06:09.198203814] [8408] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > > > [24:06:09.199839579] [8408] INFO RPI raspberrypi.cpp:1374 Registered camera /base/soc/i2c0mux/i2c at 1/imx477 at 1a to Unicam device /dev/media0 and ISP device /dev/media3
> > > > >
> > > > > Could you kindly point me to the caller site in your pipeline handler
> > > > > ?
> > > > >
> > > > > > This is because our meta formats for statistics and embedded data do not
> > > > > > have defined types in libcamera::formats::PixelFormat. The reason for this
> > > > > > is that they are metadata formats used exclusively internally in the
> > > > > > pipeline handler, and can never be selected by the application.
> > > > >
> > > > > I would be interested to see the caller as in my head, if those
> > > > > formats are not application visibile, there's no reason to convert
> > > > > them to libcamera::formats (ie, they could be managed as V4L2
> > > > > formats).
> > > >
> > > > FYI: This is happening on IPU3 too.
> > > >
> > > > kbingham at Jupiter:~$ cam --list
> > > > [0:01:55.607533003] [3760] INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3813-fa20fd23
> > > > [0:01:55.621621933] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3G
> > > > [0:01:55.621728733] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.622204108] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3p
> > > > [0:01:55.622273358] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.622399044] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3s
> > > > [0:01:55.622433003] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.622765279] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3G
> > > > [0:01:55.622798961] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.623113998] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3p
> > > > [0:01:55.623142207] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.623266568] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3s
> > > > [0:01:55.623291940] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > > [0:01:55.628131922] [3764] WARN V4L2 v4l2_pixelformat.cpp:301 Unsupported V4L2 pixel format ip3G
> > > > [0:01:55.628184502] [3764] WARN Formats formats.cpp:838 Unsupported pixel format 0x00000000
> > > >
> > > > > > I'm not sure how to fix this. Do I need to add entries in formats.yaml to
> > > > > > generate types in libcamera::formats::PixelFormat, meaning I also have to
> > > > > > add new 4CCs in the drm headers... :-( Given these are metadata formats,
> > > > > > and not user selectable, is this the right thing to do?
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list