| Message ID | 20240227140953.26093-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent 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@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 ? > + * > + * \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 > + * > + * 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 ? > + * > + * 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 > + << 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 >
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@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 "
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 + * + * \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. + * + * For formats that transmit multiple or fractional pixels per sample, the + * value will differ from 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 " + << 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 "
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@ideasonboard.com> --- include/libcamera/internal/v4l2_subdevice.h | 13 +++ src/libcamera/v4l2_subdevice.cpp | 94 ++++++++++++++------- 2 files changed, 76 insertions(+), 31 deletions(-)