[libcamera-devel] [PATCH v2 6/8] libcamera: PixelFormat: Turn into a class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 17 12:12:06 CET 2020
Hi Niklas,
Thank you for the patch.
On Tue, Mar 17, 2020 at 04:52:37AM +0100, Niklas Söderlund wrote:
> Create a class to represent a pixel formats. This is done to add support
s/formats/format/
> for modifiers for the formats. So far no formats are added by any
Do you mean no modifiers, not no formats ?
> pipeline handler, all plumbing to deal with them is however in place.
>
> Pipelines that adds modifiers will come when support for RAW capture is
s/Pipelines/Pipeline handlers/
s/adds/add/
I think I'd drop this sentence anyway.
> added.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * 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 | 21 ++++++-
> 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/pixelformats.cpp | 78 ++++++++++++++++++++++--
> src/libcamera/stream.cpp | 4 +-
> src/libcamera/v4l2_videodevice.cpp | 5 +-
> src/qcam/format_converter.cpp | 2 +-
> src/v4l2/v4l2_camera_proxy.cpp | 4 +-
> 10 files changed, 110 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> index 6e25b8d8b76e5961..5ba1ba1b324272b9 100644
> --- a/include/libcamera/pixelformats.h
> +++ b/include/libcamera/pixelformats.h
> @@ -7,11 +7,30 @@
> #ifndef __LIBCAMERA_PIXEL_FORMATS_H__
> #define __LIBCAMERA_PIXEL_FORMATS_H__
>
> +#include <set>
> #include <stdint.h>
> +#include <string>
>
> namespace libcamera {
>
> -using PixelFormat = uint32_t;
> +class PixelFormat
> +{
> +public:
> + PixelFormat();
> + PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> +
> + uint32_t fourcc() const { return fourcc_; }
Will you add a uint32_t conversion operator ? It could be done on top,
but it would remove the need for some of the explicit calls to fourcc()
below, so I think it would be nice to add it to this patch.
> + const std::set<uint64_t> &modifiers() const { return modifiers_; }
> + std::string toString() const;
> +
> +private:
> + uint32_t fourcc_;
> + std::set<uint64_t> modifiers_;
> +};
> +
> +bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);
> +bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);
> +bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);
>
> } /* 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());
>
> 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 01977ad697a91a44..8223b82c4a9c773c 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -790,7 +790,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(),
> .size = data->stream_.configuration().size,
> };
>
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 5ebd83f3c2099ffe..12af164590020142 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/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index c03335400b709d9b..fe9a6a2576978647 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -7,6 +7,8 @@
>
> #include <libcamera/pixelformats.h>
>
> +#include "log.h"
> +
Not needed.
> /**
> * \file pixelformats.h
> * \brief libcamera pixel formats
> @@ -15,14 +17,80 @@
> 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()
> + : 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)
> +{
> +}
> +
> +/**
> + * \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;
> +}
> +
> +/**
> + * \brief Compare pixel formats for equality
> + * \return True if the two pixel formats are equal, false otherwise
> + */
> +bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)
> +{
> + return lhs.fourcc() == rhs.fourcc() &&
> + lhs.modifiers() == rhs.modifiers();
> +}
> +
> +/**
> + * \brief Compare pixel formats for inequality
> + * \return True if the two pixel formats are not equal, false otherwise
> + */
> +bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)
> +{
> + return !(lhs == rhs);
> +}
This one should really be inlined, it will save a function call.
> +
> +/**
> + * \brief Compare pixel formats for smaller than order
> + * \todo Take modifiers into account if \a lhs == \a rhs.
How about doing so already ?
> + * \return True if \a lhs is smaller than \a rhs, false otherwise
> + */
> +bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)
> +{
> + return lhs.fourcc() < rhs.fourcc();
if (lhs.fourcc() < rhs.fourcc())
return true;
if (lhs.fourcc() > rhs.fourcc())
return false;
return lhs.modifiers() < rhs.modifiers();
> +}
> +
> } /* 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 280cf1877c8936d7..b5762a7eabcf4e25 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1643,7 +1643,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;
> @@ -1687,8 +1687,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 368cb43fbf17ae4d..c071ea9b4022750c 100644
> --- a/src/qcam/format_converter.cpp
> +++ b/src/qcam/format_converter.cpp
> @@ -30,7 +30,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 b620f236499cf77d..3bbbbf79cdb475db 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -534,7 +534,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 } }} },
> @@ -606,7 +606,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