[libcamera-devel,v4,5/7] libcamera: v4l2_subdevice: Add format information
diff mbox series

Message ID 20200427213236.333777-6-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: Add CameraSensorInfo
Related show

Commit Message

Jacopo Mondi April 27, 2020, 9:32 p.m. UTC 
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.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
 src/libcamera/include/v4l2_subdevice.h |   2 +-
 src/libcamera/v4l2_subdevice.cpp       | 104 +++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletion(-)

--
2.26.1

Comments

Niklas Söderlund April 28, 2020, 12:06 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

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.
> 
> Signed-off-by: Jacopo Mondi <jacopo@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;
>  };
> 
>  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 },
> +	{ 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
> @@ -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.

> +	}
> +
> +	return it->second;
> +}
> +
>  /**
>   * \class V4L2Subdevice
>   * \brief A V4L2 subdevice as exposed by the Linux kernel
> --
> 2.26.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 28, 2020, 2:01 a.m. UTC | #2 
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@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@ideasonboard.com>

> > +	}
> > +
> > +	return it->second;
> > +}
> > +
> >  /**
> >   * \class V4L2Subdevice
> >   * \brief A V4L2 subdevice as exposed by the Linux kernel
Jacopo Mondi April 28, 2020, 7:18 a.m. UTC | #3 
Hi Laurent, Niklas,

On Tue, Apr 28, 2020 at 05:01:23AM +0300, Laurent Pinchart wrote:
> 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@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'
?

>
> > > +	{ 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@ideasonboard.com>
>

Thanks
  j

> > > +	}
> > > +
> > > +	return it->second;
> > > +}
> > > +
> > >  /**
> > >   * \class V4L2Subdevice
> > >   * \brief A V4L2 subdevice as exposed by the Linux kernel
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart April 28, 2020, 11:20 a.m. UTC | #4 
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@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@ideasonboard.com>
> >
> > > > +	}
> > > > +
> > > > +	return it->second;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \class V4L2Subdevice
> > > >   * \brief A V4L2 subdevice as exposed by the Linux kernel

Patch
diff mbox series 

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

 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 },
+	{ 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
@@ -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;
+	}
+
+	return it->second;
+}
+
 /**
  * \class V4L2Subdevice
  * \brief A V4L2 subdevice as exposed by the Linux kernel