| Message ID | 20200424215304.558317-9-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. 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@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. > + > 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. > +const V4L2SubdeviceFormat::infoMap formatInfoMap = { So here we would have 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 }, > + { 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. > /** > * \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. With these small changes, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + } > + > + return it->second; > +} > + > /** > * \class V4L2Subdevice > * \brief A V4L2 subdevice as exposed by the Linux kernel
Hi Laurent, On Sat, Apr 25, 2020 at 11:29:03PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > 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@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 > > + > > 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 > > > +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. > > + { 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 ? > > /** > > * \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 Thanks j > > With these small changes, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + } > > + > > + return it->second; > > +} > > + > > /** > > * \class V4L2Subdevice > > * \brief A V4L2 subdevice as exposed by the Linux kernel > > -- > Regards, > > Laurent Pinchart
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@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@ideasonboard.com> > > > > > + } > > > + > > > + return it->second; > > > +} > > > + > > > /** > > > * \class V4L2Subdevice > > > * \brief A V4L2 subdevice as exposed by the Linux kernel
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>; + 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 + */ +const V4L2SubdeviceFormat::infoMap 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 }, + { 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. + */ + /** * \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 + */ +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 8 to avoid divisions by 0. */ + return 8; + } + + return it->second; +} + /** * \class V4L2Subdevice * \brief A V4L2 subdevice as exposed by the Linux kernel
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@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 3 + src/libcamera/v4l2_subdevice.cpp | 113 +++++++++++++++++++++++++ 2 files changed, 116 insertions(+)