[libcamera-devel] [PATCH v3 6/8] libcamera: PixelFormat: Turn into a class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Mar 18 14:37:39 CET 2020
Hi Niklas,
Thank you for the patch.
On Wed, Mar 18, 2020 at 04:31:58AM +0100, Niklas Söderlund wrote:
> Create a class to represent a pixel format. This is done to add support
> for modifiers for the formats. So far no modifiers are added by any
> pipeline handler, all plumbing to deal with them is however in place.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v2
> - Add isValid()
> - Add operator uint32_t()
> - Handle modifiers in operator<()
> - Remove include for log.h
>
> * Changes since v1
> - Remove copy constructor and operator=
> - Removed LOG_DEFINE_CATEGORY
> - Updated documentation
> - Improved toString()
>
> * Changes since RFC
> - Drop table of translation from V4L2 to DRM fourcc and turn PixelFormat
> into a more basic data container class.
> ---
> include/libcamera/pixelformats.h | 25 ++++++-
> src/cam/main.cpp | 6 +-
> src/gstreamer/gstlibcamera-utils.cpp | 8 +--
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 +-
> src/libcamera/pipeline/uvcvideo.cpp | 5 +-
> src/libcamera/pipeline/vimc.cpp | 2 +-
> src/libcamera/pixelformats.cpp | 87 ++++++++++++++++++++++--
> src/libcamera/stream.cpp | 4 +-
> src/libcamera/v4l2_videodevice.cpp | 5 +-
> src/qcam/format_converter.cpp | 2 +-
> src/v4l2/v4l2_camera_proxy.cpp | 4 +-
> 11 files changed, 124 insertions(+), 26 deletions(-)
>
> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> index 544363af7a8c6f05..eb40e55ac159505a 100644
> --- a/include/libcamera/pixelformats.h
> +++ b/include/libcamera/pixelformats.h
> @@ -7,13 +7,36 @@
> #ifndef __LIBCAMERA_PIXEL_FORMATS_H__
> #define __LIBCAMERA_PIXEL_FORMATS_H__
>
> +#include <set>
> #include <stdint.h>
> +#include <string>
>
> #include <linux/drm_fourcc.h>
>
> namespace libcamera {
>
> -using PixelFormat = uint32_t;
> +class PixelFormat
> +{
> +public:
> + PixelFormat();
> + PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> +
> + bool operator==(const PixelFormat &other) const;
> + bool operator!=(const PixelFormat &other) const { return !(*this == other); }
> + bool operator<(const PixelFormat &other) const;
> +
> + bool isValid() const { return fourcc_ != 0; }
> +
> + operator uint32_t() const { return fourcc_; }
> + uint32_t fourcc() const { return fourcc_; }
> + const std::set<uint64_t> &modifiers() const { return modifiers_; }
> +
> + std::string toString() const;
> +
> +private:
> + uint32_t fourcc_;
> + std::set<uint64_t> modifiers_;
> +};
>
> } /* namespace libcamera */
>
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index af516f1cbf23974a..f73e77f381779853 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -253,7 +253,7 @@ int CamApp::prepareConfig()
>
> /* TODO: Translate 4CC string to ID. */
> if (opt.isSet("pixelformat"))
> - cfg.pixelFormat = opt["pixelformat"];
> + cfg.pixelFormat = PixelFormat(opt["pixelformat"]);
> }
> }
>
> @@ -304,8 +304,8 @@ int CamApp::infoConfiguration()
>
> const StreamFormats &formats = cfg.formats();
> for (PixelFormat pixelformat : formats.pixelformats()) {
> - std::cout << " * Pixelformat: 0x" << std::hex
> - << std::setw(8) << pixelformat << " "
> + std::cout << " * Pixelformat: "
> + << pixelformat.toString() << " "
> << formats.range(pixelformat).toString()
> << std::endl;
>
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index 3b3973bcea3dc759..f21e94c3eef92737 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -80,11 +80,11 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)
> GstCaps *caps = gst_caps_new_empty();
>
> for (PixelFormat pixelformat : formats.pixelformats()) {
> - g_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(pixelformat);
> + g_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(pixelformat.fourcc());
With the uint32_t conversion operator, I think you wouldn't have to call
.fourcc() explicitly here and in several locations below. It's of course
up to you.
>
> if (!bare_s) {
> GST_WARNING("Unsupported DRM format %" GST_FOURCC_FORMAT,
> - GST_FOURCC_ARGS(pixelformat));
> + GST_FOURCC_ARGS(pixelformat.fourcc()));
> continue;
> }
>
> @@ -120,7 +120,7 @@ GstCaps *
> gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)
> {
> GstCaps *caps = gst_caps_new_empty();
> - GstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat);
> + GstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat.fourcc());
>
> gst_structure_set(s,
> "width", G_TYPE_INT, stream_cfg.size.width,
> @@ -135,7 +135,7 @@ void
> gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> GstCaps *caps)
> {
> - GstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat);
> + GstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat.fourcc());
>
> /* First fixate the caps using default configuration value. */
> g_assert(gst_caps_is_writable(caps));
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index a95ae48ac8cfbc98..8a11deb814bc0bfb 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -789,7 +789,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> /* Inform IPA of stream configuration and sensor controls. */
> std::map<unsigned int, IPAStream> streamConfig;
> streamConfig[0] = {
> - .pixelFormat = data->stream_.configuration().pixelFormat,
> + .pixelFormat = data->stream_.configuration().pixelFormat.fourcc(),
Note for later, IPAStream needs to be extended with modifiers. Could you
write this down in the tasks list ?
> .size = data->stream_.configuration().size,
> };
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 1de091e0c0e57f7c..731149755728f209 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -113,8 +113,9 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> if (iter == pixelFormats.end()) {
> cfg.pixelFormat = pixelFormats.front();
> LOG(UVC, Debug)
> - << "Adjusting pixel format from " << pixelFormat
> - << " to " << cfg.pixelFormat;
> + << "Adjusting pixel format from "
> + << pixelFormat.toString() << " to "
> + << cfg.pixelFormat.toString();
> status = Adjusted;
> }
>
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 04cad94e739e9ae9..2e2162b2bf4477c5 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -103,7 +103,7 @@ private:
>
> namespace {
>
> -constexpr std::array<PixelFormat, 3> pixelformats{
> +static const std::array<PixelFormat, 3> pixelformats{
> DRM_FORMAT_RGB888,
> DRM_FORMAT_BGR888,
> DRM_FORMAT_BGRA8888,
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index c03335400b709d9b..929578fed3e22c92 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -15,14 +15,91 @@
> namespace libcamera {
>
> /**
> - * \typedef PixelFormat
> + * \class PixelFormat
> * \brief libcamera image pixel format
> *
> * The PixelFormat type describes the format of images in the public libcamera
> - * API. It stores a FourCC value as a 32-bit unsigned integer. The values are
> - * defined in the Linux kernel DRM/KMS API (see linux/drm_fourcc.h).
> - *
> - * \todo Add support for format modifiers
> + * API. It stores a FourCC value as a 32-bit unsigned integer and a set of
> + * modifiers. The FourCC and modifiers values are defined in the Linux kernel
> + * DRM/KMS API (see linux/drm_fourcc.h).
> */
>
> +PixelFormat::PixelFormat()
This should be documented.
/**
* \brief Construct a PixelFormat with an invalid format
*
* PixelFormat instances constructed with the default constructor are
* invalid, calling the isValid() function returns false.
*/
> + : fourcc_(0)
> +{
> +}
> +
> +/**
> + * \brief Construct a PixelFormat from a DRM FourCC and a set of modifiers
> + * \param[in] fourcc A DRM FourCC
> + * \param[in] modifiers A set of DRM FourCC modifiers
> + */
> +PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
> + : fourcc_(fourcc), modifiers_(modifiers)
> +{
> +}
> +
> +/**
> + * \brief Compare pixel formats for equality
> + * \return True if the two pixel formats are equal, false otherwise
> + */
> +bool PixelFormat::operator==(const PixelFormat &other) const
> +{
> + return fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;
> +}
> +
> +/**
> + * \fn bool PixelFormat::operator!=(const PixelFormat &other) const
> + * \brief Compare pixel formats for inequality
> + * \return True if the two pixel formats are not equal, false otherwise
> + */
> +
> +/**
> + * \brief Compare pixel formats for smaller than order
> + * \return True if \a this is smaller than \a other, false otherwise
> + */
> +bool PixelFormat::operator<(const PixelFormat &other) const
> +{
> + if (fourcc_ < other.fourcc_)
> + return true;
> + if (fourcc_ > other.fourcc_)
> + return false;
> + return modifiers_ < modifiers_;
> +}
> +
> +/**
> + * \fn bool PixelFormat::isValid() const
> + * \brief Check if the pixel format is valid
> + * \return True if the pixel format has a non-zero value, false otherwise
Maybe we should avoid talking about a non-zero value if we want to keep
that an implementation detail ?
/**
* \fn bool PixelFormat::isValid() const
* \brief Check if the pixel format is valid
*
* PixelFormat instances constructed with the default constructor are
* invalid. Instances constructed with a FourCC defined in the DRM API
* are valid. The behaviour is undefined otherwise.
*
* \return True if the pixel format is valid, false otherwise
*/
Up to you.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> +/**
> + * \fn PixelFormat::operator uint32_t() const
> + * \brief Convert the the pixel format numerical value
> + * \return The pixel format numerical value
> + */
> +
> +/**
> + * \fn PixelFormat::fourcc() const
> + * \brief Retrieve the pixel format FourCC
> + * \return DRM FourCC
> + */
> +
> +/**
> + * \fn PixelFormat::modifiers() const
> + * \brief Retrieve the pixel format modifiers
> + * \return Set of DRM modifiers
> + */
> +
> +/**
> + * \brief Assemble and return a string describing the pixel format
> + * \return A string describing the pixel format
> + */
> +std::string PixelFormat::toString() const
> +{
> + char str[11];
> + snprintf(str, 11, "0x%08x", fourcc_);
> + return str;
> +}
> +
> } /* namespace libcamera */
> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> index 13789e9eb344f95c..0716de388bd81d80 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -347,9 +347,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)
> */
> std::string StreamConfiguration::toString() const
> {
> - std::stringstream ss;
> - ss << size.toString() << "-" << utils::hex(pixelFormat);
> - return ss.str();
> + return size.toString() + "-" + pixelFormat.toString();
> }
>
> /**
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 3869766046236f34..c8ba0f8cebedb91a 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1647,7 +1647,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)
> */
> uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)
> {
> - switch (pixelFormat) {
> + switch (pixelFormat.fourcc()) {
> /* RGB formats. */
> case DRM_FORMAT_BGR888:
> return V4L2_PIX_FMT_RGB24;
> @@ -1691,8 +1691,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar
> * class. Until we fix the logger, work around it.
> */
> libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), LogError).stream()
> - << "Unsupported V4L2 pixel format "
> - << utils::hex(pixelFormat);
> + << "Unsupported V4L2 pixel format " << pixelFormat.toString();
> return 0;
> }
>
> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
> index 7b8ad77c42fe85d4..66d07025ac9578ca 100644
> --- a/src/qcam/format_converter.cpp
> +++ b/src/qcam/format_converter.cpp
> @@ -28,7 +28,7 @@
> int FormatConverter::configure(libcamera::PixelFormat format, unsigned int width,
> unsigned int height)
> {
> - switch (format) {
> + switch (format.fourcc()) {
> case DRM_FORMAT_NV12:
> formatFamily_ = NV;
> horzSubSample_ = 2;
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index e349fbddc2a4d5a2..c6d1e5030b58b630 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -533,7 +533,7 @@ struct PixelFormatInfo {
>
> namespace {
>
> -constexpr std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
> +static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
> /* RGB formats. */
> { DRM_FORMAT_RGB888, V4L2_PIX_FMT_BGR24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> { DRM_FORMAT_BGR888, V4L2_PIX_FMT_RGB24, 1, {{ { 24, 1, 1 }, { 0, 0, 0 }, { 0, 0, 0 } }} },
> @@ -605,7 +605,7 @@ uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)
> return info.format == format;
> });
> if (info == pixelFormatInfo.end())
> - return format;
> + return format.fourcc();
>
> return info->v4l2Format;
> }
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list