[libcamera-devel] [PATCH v3 08/13] libcamera: v4l2_subdevice: Add format information
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Apr 27 14:56:07 CEST 2020
Hi Jacopo,
On Mon, Apr 27, 2020 at 02:54:06PM +0200, Jacopo Mondi wrote:
> On Sat, Apr 25, 2020 at 11:29:03PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 24, 2020 at 11:52:59PM +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.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > src/libcamera/include/v4l2_subdevice.h | 3 +
> > > src/libcamera/v4l2_subdevice.cpp | 113 +++++++++++++++++++++++++
> > > 2 files changed, 116 insertions(+)
> > >
> > > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > > index 763ffadb61fb..43953a0a5a6c 100644
> > > --- a/src/libcamera/include/v4l2_subdevice.h
> > > +++ b/src/libcamera/include/v4l2_subdevice.h
> > > @@ -23,10 +23,13 @@ namespace libcamera {
> > > class MediaDevice;
> > >
> > > struct V4L2SubdeviceFormat {
> > > + using infoMap = std::map<uint32_t, uint8_t>;
> >
> > There's no need to define a type here as it's not used in the API, you
> > can use it directly below.
>
> I like to shorten the type though, and it does not actually brings
> much obfuscation
That's fine, but then please move it to v4l2_subdev.cpp. Otherwise (as
you've noticed) you have to document it, and documenting a type that
isn't part of the API is confusing for documentation readers.
> > > +
> > > uint32_t mbus_code;
> > > Size size;
> > >
> > > const std::string toString() const;
> > > + uint8_t bitsPerPixel() const;
> > > };
> > >
> > > class V4L2Subdevice : public V4L2Device
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 4dcf8ce48754..6f72c363fc7f 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 information to V4L2 media bus codes
> > > + */
> >
> > No need for doxygen doc (that would require /** instead of /*) as it's
> > purely internal.
>
> This is intended. In other places for documentation not parsed by
> doxygen we decided to maintain the doxygen style for consistency if
> I'm not mistaken
OK, I thought it wasn't intentional. You can drop the \var though, as
this is documenting the next line, and in that case I wonder if \brief
is better than
/* A map that associates information to V4L2 media bus codes. */
:-)
> > > +const V4L2SubdeviceFormat::infoMap formatInfoMap = {
> >
> > So here we would have
> >
> > const std::map<uint32_t, uint8_t> formatInfoMap = {
>
> meh. I can drop it, but we used type shortcuts in other places.
We do when they're part of the API, or used in multiple places, or very
long I think. I'm fine keeping it, but I wonder if
using infoMap = std::map<uint32_t, uint8_t>;
const V4L2SubdeviceFormat::infoMap formatInfoMap = {
is really more readable.
> > > + { 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 },
> > > + { 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 },
> > > + { V4L2_MBUS_FMT_JPEG_1X8, 8 },
> > > + { V4L2_MBUS_FMT_S5C_UYVY_JPEG_1X8, 8 },
> > > + { V4L2_MBUS_FMT_AHSV8888_1X32, 32 },
> > > +};
> > > +
> > > +}; /* namespace */
> > > +
> > > /**
> > > * \struct V4L2SubdeviceFormat
> > > * \brief The V4L2 sub-device image format and sizes
> > > @@ -60,6 +147,14 @@ LOG_DECLARE_CATEGORY(V4L2)
> > > * Section 4.15.3.1 of the above mentioned Linux kernel documentation section.
> > > */
> > >
> > > +/**
> > > + * \typedef V4L2SubdeviceFormat::infoMap
> > > + * \brief Map of static information associated with a V4L2 subdevice format
> > > + *
> > > + * \todo Expand with additional information. At the moment only the
> > > + * bits-per-pixel information is reported.
> > > + */
> > > +
> >
> > And you can drop this too as the type doesn't need to be documented.
> > What we may document later is a format info structure that would contain
> > more than just the bpp, but there's no need to introduce that yet.
>
> Does it hurt ?
As it's purely internal, I think it can be confusing.
> > > /**
> > > * \var V4L2SubdeviceFormat::mbus_code
> > > * \brief The image format bus code
> > > @@ -81,6 +176,24 @@ 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 mbus code is not supported
> >
> > s/mbus/media bus/
> >
> > > + */
> > > +uint8_t V4L2SubdeviceFormat::bitsPerPixel() const
> > > +{
> > > + const auto it = formatInfoMap.find(mbus_code);
> > > + if (it == formatInfoMap.end()) {
> > > + LOG(V4L2, Error) << "No information available for format: '"
> >
> > s/://
> >
> > > + << toString() << "'";
> > > + /* Return 8 to avoid divisions by 0. */
> > > + return 8;
> >
> > I think we should return 0 and check for this condition in the caller.
> > When we'll switch to a structure that will contain information about
> > formats, the formatInfo() function will return a pointer, which will be
> > null when the format is not known. I would prefer preparing the callers
> > for this already.
>
> Ack
>
> > With these small changes,
> >
> > 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