[libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add format information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 28 13:20:54 CEST 2020
Hi Jacopo,
On Tue, Apr 28, 2020 at 09:18:48AM +0200, Jacopo Mondi wrote:
> On Tue, Apr 28, 2020 at 05:01:23AM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 28, 2020 at 02:06:34AM +0200, Niklas Söderlund wrote:
> > > On 2020-04-27 23:32:34 +0200, Jacopo Mondi wrote:
> > > > Define a map of static information associated with a media bus code.
> > > > Start by reporting the bits-per-pixel for each media bus code defined by
> > > > the V4L2 kernel API, to later expand it when necessary.
> > > >
> > > > Add to the V4L2SubdeviceFormat class a method to return the bits per
> > > > pixel, retrieved by inspecting the static map of format information.
> > > >
> > > > While at there, remove a rouge map inclusion from header file.
> >
> > s/there/it/ (or "while there")
> > s/rouge/rogue/ (unless you really mean red :-))
> >
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > >
> > > > ---
> > > > src/libcamera/include/v4l2_subdevice.h | 2 +-
> > > > src/libcamera/v4l2_subdevice.cpp | 104 +++++++++++++++++++++++++
> > > > 2 files changed, 105 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > > index 27ba5b17f61e..d0e565dbdaab 100644
> > > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > > @@ -7,7 +7,6 @@
> > > > #ifndef __LIBCAMERA_V4L2_SUBDEVICE_H__
> > > > #define __LIBCAMERA_V4L2_SUBDEVICE_H__
> > > >
> > > > -#include <map>
> > > > #include <string>
> > > > #include <vector>
> > > >
> > > > @@ -27,6 +26,7 @@ struct V4L2SubdeviceFormat {
> > > > Size size;
> > > >
> > > > const std::string toString() const;
> > > > + uint8_t bitsPerPixel() const;
> >
> > I think unsigned int would be more efficient (or at least not less). It
> > would also avoid interesting issues when writing
> >
> > LOG(Info) << "bits per pixel: " << format.bitsPerPixel();
> >
> > as with uint8_t it will be considered as a char and not a number (I
> > know...).
>
> I stumbled into this a few days ago, yes, I know :(
>
> I can change this to unsigned int, sure
>
> > > > };
> > > >
> > > > class V4L2Subdevice : public V4L2Device
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 74788ce7cf4f..93fe4b8c3d32 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -14,6 +14,7 @@
> > > > #include <sys/ioctl.h>
> > > > #include <unistd.h>
> > > >
> > > > +#include <linux/media-bus-format.h>
> > > > #include <linux/v4l2-subdev.h>
> > > >
> > > > #include <libcamera/geometry.h>
> > > > @@ -32,6 +33,92 @@ namespace libcamera {
> > > >
> > > > LOG_DECLARE_CATEGORY(V4L2)
> > > >
> > > > +namespace {
> > > > +
> > > > +/*
> > > > + * \var formatInfoMap
> > > > + * \brief A map that associates bits per pixel to V4L2 media bus codes
> > > > + */
> > > > +const std::map<uint32_t, uint8_t> formatInfoMap = {
> > > > + { V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE, 16 },
> > > > + { V4L2_MBUS_FMT_BGR565_2X8_BE, 16 },
> > > > + { V4L2_MBUS_FMT_BGR565_2X8_LE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB565_2X8_BE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB565_2X8_LE, 16 },
> > > > + { V4L2_MBUS_FMT_RGB666_1X18, 18 },
> > > > + { V4L2_MBUS_FMT_RGB888_1X24, 24 },
> > > > + { V4L2_MBUS_FMT_RGB888_2X12_BE, 24 },
> > > > + { V4L2_MBUS_FMT_RGB888_2X12_LE, 24 },
> > > > + { V4L2_MBUS_FMT_ARGB8888_1X32, 32 },
> > > > + { V4L2_MBUS_FMT_Y8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_UV8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_UYVY8_1_5X8, 40 },
> >
> > 1_5X8 means 1.5x8 = 12 :-)
>
> Ah!
>
> > > > + { V4L2_MBUS_FMT_VYUY8_1_5X8, 40 },
> > > > + { V4L2_MBUS_FMT_YUYV8_1_5X8, 40 },
> > > > + { V4L2_MBUS_FMT_YVYU8_1_5X8, 40 },
> > > > + { V4L2_MBUS_FMT_UYVY8_2X8, 16 },
> > > > + { V4L2_MBUS_FMT_VYUY8_2X8, 16 },
> > > > + { V4L2_MBUS_FMT_YUYV8_2X8, 16 },
> > > > + { V4L2_MBUS_FMT_YVYU8_2X8, 16 },
> > > > + { V4L2_MBUS_FMT_Y10_1X10, 10 },
> > > > + { V4L2_MBUS_FMT_UYVY10_2X10, 20 },
> > > > + { V4L2_MBUS_FMT_VYUY10_2X10, 20 },
> > > > + { V4L2_MBUS_FMT_YUYV10_2X10, 20 },
> > > > + { V4L2_MBUS_FMT_YVYU10_2X10, 20 },
> > > > + { V4L2_MBUS_FMT_Y12_1X12, 12 },
> > > > + { V4L2_MBUS_FMT_UYVY8_1X16, 16 },
> > > > + { V4L2_MBUS_FMT_VYUY8_1X16, 16 },
> > > > + { V4L2_MBUS_FMT_YUYV8_1X16, 16 },
> > > > + { V4L2_MBUS_FMT_YVYU8_1X16, 16 },
> > > > + { V4L2_MBUS_FMT_YDYUYDYV8_1X16, 16 },
> > > > + { V4L2_MBUS_FMT_UYVY10_1X20, 20 },
> > > > + { V4L2_MBUS_FMT_VYUY10_1X20, 20 },
> > > > + { V4L2_MBUS_FMT_YUYV10_1X20, 20 },
> > > > + { V4L2_MBUS_FMT_YVYU10_1X20, 20 },
> > > > + { V4L2_MBUS_FMT_YUV10_1X30, 30 },
> > > > + { V4L2_MBUS_FMT_AYUV8_1X32, 32 },
> > > > + { V4L2_MBUS_FMT_UYVY12_2X12, 24 },
> > > > + { V4L2_MBUS_FMT_VYUY12_2X12, 24 },
> > > > + { V4L2_MBUS_FMT_YUYV12_2X12, 24 },
> > > > + { V4L2_MBUS_FMT_YVYU12_2X12, 24 },
> > > > + { V4L2_MBUS_FMT_UYVY12_1X24, 24 },
> > > > + { V4L2_MBUS_FMT_VYUY12_1X24, 24 },
> > > > + { V4L2_MBUS_FMT_YUYV12_1X24, 24 },
> > > > + { V4L2_MBUS_FMT_YVYU12_1X24, 24 },
> > > > + { V4L2_MBUS_FMT_SBGGR8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGBRG8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGRBG8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SRGGB8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_ALAW8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGBRG10_ALAW8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGRBG10_ALAW8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SRGGB10_ALAW8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_DPCM8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGBRG10_DPCM8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SGRBG10_DPCM8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SRGGB10_DPCM8_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, 16 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE, 16 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_BE, 16 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_2X8_PADLO_LE, 16 },
> > > > + { V4L2_MBUS_FMT_SBGGR10_1X10, 10 },
> > > > + { V4L2_MBUS_FMT_SGBRG10_1X10, 10 },
> > > > + { V4L2_MBUS_FMT_SGRBG10_1X10, 10 },
> > > > + { V4L2_MBUS_FMT_SRGGB10_1X10, 10 },
> > > > + { V4L2_MBUS_FMT_SBGGR12_1X12, 24 },
> > > > + { V4L2_MBUS_FMT_SGBRG12_1X12, 24 },
> > > > + { V4L2_MBUS_FMT_SGRBG12_1X12, 24 },
> > > > + { V4L2_MBUS_FMT_SRGGB12_1X12, 24 },
> >
> > These last four tshould be 12 bits per pixel.
>
> Ouch, sorry, I re-checked this twice but I've missed this it seems
>
> > > > + { V4L2_MBUS_FMT_JPEG_1X8, 8 },
> > > > + { V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },
> >
> > I'd drop these two as bits per pixel is really ill-defined for JPEG.
>
> Ack
>
> > I think the bits per pixel concept is ill-defined in general. Our
> > current use case for this is limited to Bayer formats, to know the
> > dynamic range of the data. When transmitting, let's say,
> > V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE, I can imagine the pipeline handler
> > possibly being interested in knowing that the bus width is 16 bits in
> > order to configure the pipeline, and the IPA being interested in knowing
> > the dynamic range is 10 bits. We'll have to revisiti all this later,
> > there's no reason to block this patch for this, but please remember this
> > issue in the future as I'll want to refactor this before getting more
> > users of this API.
>
> I agree... should we start thinking of a 'sample width' vs 'bus width'
> ?
That's the idea, yes. I'm sure we'll add more parameters later. Your
patch is a good first step :-)
> > > > + { V4L2_MBUS_FMT_AHSV8888_1X32, 32 },
> > > > +};
> > > > +
> > > > +}; /* namespace */
> >
> > s/};/}/
> >
> > > > +
> > > > /**
> > > > * \struct V4L2SubdeviceFormat
> > > > * \brief The V4L2 sub-device image format and sizes
> > > > @@ -81,6 +168,23 @@ const std::string V4L2SubdeviceFormat::toString() const
> > > > return ss.str();
> > > > }
> > > >
> > > > +/**
> > > > + * \brief Retrieve the number of bits per pixel for the V4L2 subdevice format
> > > > + * \return The number of bits per pixel for the format, or default it to 8 if
> > > > + * the format's media bus code is not supported
> > > > + */
> > > > +uint8_t V4L2SubdeviceFormat::bitsPerPixel() const
> > > > +{
> > > > + const auto it = formatInfoMap.find(mbus_code);
> > > > + if (it == formatInfoMap.end()) {
> > > > + LOG(V4L2, Error) << "No information available for format '"
> > > > + << toString() << "'";
> > > > + return 0;
> > >
> > > This does not match the documentation of defaulting to 8 if the format
> > > is not found. I'm wondering however if we shall not make the error fatal
> > > as a default value that is wrong could lead to really odd behaviors.
> >
> > The code was updated but the documentation stayed as-is. s/or default it
> > to 8/or 0/.
>
> Yeah, leftover
>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > > + }
> > > > +
> > > > + return it->second;
> > > > +}
> > > > +
> > > > /**
> > > > * \class V4L2Subdevice
> > > > * \brief A V4L2 subdevice as exposed by the Linux kernel
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list