[PATCH 3/9] libcamera: v4l2_subdevice: Expose media bus format info as internal API

[Laurent Pinchart]

Hi Jacopo,

On Wed, Feb 28, 2024 at 09:32:29AM +0100, Jacopo Mondi wrote:
> On Tue, Feb 27, 2024 at 04:09:47PM +0200, Laurent Pinchart wrote:
> > The V4L2SubdeviceFormatInfo structure, internal to the
> > v4l2_subdevice.cpp compilation unit, contains information about media
> > bus formats that will be useful in other parts of libcamera. To prepare
> > for this, expose the structure in the v4l2_subdevice.h header and turn
> > it into a class with a similar design as PixelFormatInfo.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> >  include/libcamera/internal/v4l2_subdevice.h | 13 +++
> >  src/libcamera/v4l2_subdevice.cpp            | 94 ++++++++++++++-------
> >  2 files changed, 76 insertions(+), 31 deletions(-)
> >
> > diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> > index 17db311bcfb3..a4df9ddfd322 100644
> > --- a/include/libcamera/internal/v4l2_subdevice.h
> > +++ b/include/libcamera/internal/v4l2_subdevice.h
> > @@ -29,6 +29,19 @@ namespace libcamera {
> >
> >  class MediaDevice;
> >
> > +class MediaBusFormatInfo
> > +{
> > +public:
> > +	bool isValid() const { return code != 0; }
> > +
> > +	static const MediaBusFormatInfo &info(uint32_t code);
> > +
> > +	const char *name;
> > +	uint32_t code;
> > +	unsigned int bitsPerPixel;
> > +	PixelFormatInfo::ColourEncoding colourEncoding;
> > +};
> > +
> >  struct V4L2SubdeviceCapability final : v4l2_subdev_capability {
> >  	bool isReadOnly() const
> >  	{
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index a74b8362b6d1..fd289ae9ae6f 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -36,28 +36,40 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(V4L2)
> >
> > +/**
> > + * \class MediaBusFormatInfo
> > + * \brief Information about media bus formats
> > + *
> > + * The MediaBusFormatInfo class groups together information describing a media
> > + * bus format. It facilitates handling of media bus formats by providing data
> > + * commonly used in pipeline handlers.
> > + *
> > + * \var MediaBusFormatInfo::name
> > + * \brief The format name as a human-readable string, used as the text
> > + * representation of the format
> > + *
> > + * \var MediaBusFormatInfo::code
> > + * \brief The media bus format code described by this instance
> 
> Is it worth saying it's one of the MEDIA_BUS_FMT_ entries defined by
> V4L2 ?

Good idea, I'll add it.

> > + *
> > + * \var MediaBusFormatInfo::bitsPerPixel
> > + * \brief The average number of bits per pixel
> > + *
> > + * The number of bits per pixel averages the total number of bits for all
> > + * colour components over the whole image, excluding any padding bits or
> > + * padding pixels.
> 
> Not sure I got why it's an average here

See MEDIA_BUS_FMT_UYVY8_1_5X8 in
https://docs.kernel.org/userspace-api/media/v4l/subdev-formats.html#v4l2-mbus-pixelcode.
The chroma samples are shared between multiple pixels.

> > + *
> > + * For formats that transmit multiple or fractional pixels per sample, the
> > + * value will differ from the bus width.
> 
> Is this relevant ? Isn't this related to the HW bus width ?

Yes it's related to the hardware bus width. The point of this comment is
to indicate to the reader that bitsPerPixel is not the bus width.

> > + *
> > + * Formats that don't have a fixed number of bits per pixel, such as compressed
> > + * formats, report 0 in this field.
> > + *
> > + * \var MediaBusFormatInfo::colourEncoding
> > + * \brief The colour encoding type
> > + */
> > +
> >  namespace {
> >
> > -/*
> > - * \struct MediaBusFormatInfo
> > - * \brief Information about media bus formats
> > - * \param name Name of MBUS format
> > - * \param code The media bus format code
> > - * \param bitsPerPixel Bits per pixel
> > - * \param colourEncoding Type of colour encoding
> > - */
> > -struct MediaBusFormatInfo {
> > -	const char *name;
> > -	uint32_t code;
> > -	unsigned int bitsPerPixel;
> > -	PixelFormatInfo::ColourEncoding colourEncoding;
> > -};
> > -
> > -/*
> > - * \var mediaBusFormatInfo
> > - * \brief A map that associates MediaBusFormatInfo struct to V4L2 media
> > - * bus codes
> > - */
> >  const std::map<uint32_t, MediaBusFormatInfo> mediaBusFormatInfo{
> >  	/* This table is sorted to match the order in linux/media-bus-format.h */
> >  	{ MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, {
> > @@ -533,6 +545,33 @@ const std::map<uint32_t, MediaBusFormatInfo> mediaBusFormatInfo{
> >
> >  } /* namespace */
> >
> > +/**
> > + * \fn bool MediaBusFormatInfo::isValid() const
> > + * \brief Check if the media bus format info is valid
> > + * \return True if the media bus format info is valid, false otherwise
> > + */
> > +
> > +/**
> > + * \brief Retrieve information about a media bus format
> > + * \param[in] code The media bus format code
> > + * \return The MediaBusFormatInfo describing the \a code if known, or an invalid
> > + * MediaBusFormatInfo otherwise
> > + */
> > +const MediaBusFormatInfo &MediaBusFormatInfo::info(uint32_t code)
> > +{
> > +	static const MediaBusFormatInfo invalid{};
> > +
> > +	const auto it = mediaBusFormatInfo.find(code);
> > +	if (it == mediaBusFormatInfo.end()) {
> > +		LOG(V4L2, Warning)
> > +			<< "Unsupported media bus format "
> 
> Fitst on the previous line

Not the full statement:

		LOG(V4L2, Warning) << "Unsupported media bus format " << utils::hex(code, 4);

and our usual coding style is to wrap just after LOG() in that case.

> > +			<< utils::hex(code, 4);
> > +		return invalid;
> > +	}
> > +
> > +	return it->second;
> > +}
> > +
> >  /**
> >   * \struct V4L2SubdeviceCapability
> >   * \brief struct v4l2_subdev_capability object wrapper and helpers
> > @@ -629,14 +668,7 @@ const std::string V4L2SubdeviceFormat::toString() const
> >   */
> >  uint8_t V4L2SubdeviceFormat::bitsPerPixel() const
> >  {
> > -	const auto it = mediaBusFormatInfo.find(mbus_code);
> > -	if (it == mediaBusFormatInfo.end()) {
> > -		LOG(V4L2, Error) << "No information available for format '"
> > -				 << *this << "'";
> > -		return 0;
> > -	}
> > -
> > -	return it->second.bitsPerPixel;
> > +	return MediaBusFormatInfo::info(mbus_code).bitsPerPixel;
> >  }
> >
> >  /**
> > @@ -903,9 +935,9 @@ std::optional<ColorSpace> V4L2Subdevice::toColorSpace(const v4l2_mbus_framefmt &
> >  		return std::nullopt;
> >
> >  	PixelFormatInfo::ColourEncoding colourEncoding;
> > -	auto iter = mediaBusFormatInfo.find(format.code);
> > -	if (iter != mediaBusFormatInfo.end()) {
> > -		colourEncoding = iter->second.colourEncoding;
> > +	const MediaBusFormatInfo &info = MediaBusFormatInfo::info(format.code);
> > +	if (info.isValid()) {
> > +		colourEncoding = info.colourEncoding;
> >  	} else {
> >  		LOG(V4L2, Warning)
> >  			<< "Unknown subdev format "

-- 
Regards,

Laurent Pinchart