[libcamera-devel] [PATCH v4 5/7] libcamera: v4l2_subdevice: Add format information

Tue Apr 28 04:01:23 CEST 2020

Hi Jacopo and Niklas,

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...).

> >  };
> > 
> >  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 :-)

> > +	{ 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.

> > +	{ 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.

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.

> > +	{ 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/.

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