[{"id":4077,"web_url":"https://patchwork.libcamera.org/comment/4077/","msgid":"<20200318133739.GL4733@pendragon.ideasonboard.com>","date":"2020-03-18T13:37:39","subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: PixelFormat: Turn\n\tinto a class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Mar 18, 2020 at 04:31:58AM +0100, Niklas Söderlund wrote:\n> Create a class to represent a pixel format. This is done to add support\n> for modifiers for the formats. So far no modifiers are added by any\n> pipeline handler, all plumbing to deal with them is however in place.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n> * Changes since v2\n> - Add isValid()\n> - Add operator uint32_t()\n> - Handle modifiers in operator<()\n> - Remove include for log.h\n> \n> * Changes since v1\n> - Remove copy constructor and operator=\n> - Removed LOG_DEFINE_CATEGORY\n> - Updated documentation\n> - Improved toString()\n> \n> * Changes since RFC\n> - Drop table of translation from V4L2 to DRM fourcc and turn PixelFormat\n>   into a more basic data container class.\n> ---\n>  include/libcamera/pixelformats.h         | 25 ++++++-\n>  src/cam/main.cpp                         |  6 +-\n>  src/gstreamer/gstlibcamera-utils.cpp     |  8 +--\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  2 +-\n>  src/libcamera/pipeline/uvcvideo.cpp      |  5 +-\n>  src/libcamera/pipeline/vimc.cpp          |  2 +-\n>  src/libcamera/pixelformats.cpp           | 87 ++++++++++++++++++++++--\n>  src/libcamera/stream.cpp                 |  4 +-\n>  src/libcamera/v4l2_videodevice.cpp       |  5 +-\n>  src/qcam/format_converter.cpp            |  2 +-\n>  src/v4l2/v4l2_camera_proxy.cpp           |  4 +-\n>  11 files changed, 124 insertions(+), 26 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 544363af7a8c6f05..eb40e55ac159505a 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -7,13 +7,36 @@\n>  #ifndef __LIBCAMERA_PIXEL_FORMATS_H__\n>  #define __LIBCAMERA_PIXEL_FORMATS_H__\n>  \n> +#include <set>\n>  #include <stdint.h>\n> +#include <string>\n>  \n>  #include <linux/drm_fourcc.h>\n>  \n>  namespace libcamera {\n>  \n> -using PixelFormat = uint32_t;\n> +class PixelFormat\n> +{\n> +public:\n> +\tPixelFormat();\n> +\tPixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});\n> +\n> +\tbool operator==(const PixelFormat &other) const;\n> +\tbool operator!=(const PixelFormat &other) const { return !(*this == other); }\n> +\tbool operator<(const PixelFormat &other) const;\n> +\n> +\tbool isValid() const { return fourcc_ != 0; }\n> +\n> +\toperator uint32_t() const { return fourcc_; }\n> +\tuint32_t fourcc() const { return fourcc_; }\n> +\tconst std::set<uint64_t> &modifiers() const { return modifiers_; }\n> +\n> +\tstd::string toString() const;\n> +\n> +private:\n> +\tuint32_t fourcc_;\n> +\tstd::set<uint64_t> modifiers_;\n> +};\n>  \n>  } /* namespace libcamera */\n>  \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index af516f1cbf23974a..f73e77f381779853 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -253,7 +253,7 @@ int CamApp::prepareConfig()\n>  \n>  \t\t\t/* TODO: Translate 4CC string to ID. */\n>  \t\t\tif (opt.isSet(\"pixelformat\"))\n> -\t\t\t\tcfg.pixelFormat = opt[\"pixelformat\"];\n> +\t\t\t\tcfg.pixelFormat = PixelFormat(opt[\"pixelformat\"]);\n>  \t\t}\n>  \t}\n>  \n> @@ -304,8 +304,8 @@ int CamApp::infoConfiguration()\n>  \n>  \t\tconst StreamFormats &formats = cfg.formats();\n>  \t\tfor (PixelFormat pixelformat : formats.pixelformats()) {\n> -\t\t\tstd::cout << \" * Pixelformat: 0x\" << std::hex\n> -\t\t\t\t  << std::setw(8) << pixelformat << \" \"\n> +\t\t\tstd::cout << \" * Pixelformat: \"\n> +\t\t\t\t  << pixelformat.toString() << \" \"\n>  \t\t\t\t  << formats.range(pixelformat).toString()\n>  \t\t\t\t  << std::endl;\n>  \n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index 3b3973bcea3dc759..f21e94c3eef92737 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -80,11 +80,11 @@ gst_libcamera_stream_formats_to_caps(const StreamFormats &formats)\n>  \tGstCaps *caps = gst_caps_new_empty();\n>  \n>  \tfor (PixelFormat pixelformat : formats.pixelformats()) {\n> -\t\tg_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(pixelformat);\n> +\t\tg_autoptr(GstStructure) bare_s = bare_structure_from_fourcc(pixelformat.fourcc());\n\nWith the uint32_t conversion operator, I think you wouldn't have to call\n.fourcc() explicitly here and in several locations below. It's of course\nup to you.\n\n>  \n>  \t\tif (!bare_s) {\n>  \t\t\tGST_WARNING(\"Unsupported DRM format %\" GST_FOURCC_FORMAT,\n> -\t\t\t\t    GST_FOURCC_ARGS(pixelformat));\n> +\t\t\t\t    GST_FOURCC_ARGS(pixelformat.fourcc()));\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> @@ -120,7 +120,7 @@ GstCaps *\n>  gst_libcamera_stream_configuration_to_caps(const StreamConfiguration &stream_cfg)\n>  {\n>  \tGstCaps *caps = gst_caps_new_empty();\n> -\tGstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat);\n> +\tGstStructure *s = bare_structure_from_fourcc(stream_cfg.pixelFormat.fourcc());\n>  \n>  \tgst_structure_set(s,\n>  \t\t\t  \"width\", G_TYPE_INT, stream_cfg.size.width,\n> @@ -135,7 +135,7 @@ void\n>  gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>  \t\t\t\t\t GstCaps *caps)\n>  {\n> -\tGstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat);\n> +\tGstVideoFormat gst_format = drm_to_gst_format(stream_cfg.pixelFormat.fourcc());\n>  \n>  \t/* First fixate the caps using default configuration value. */\n>  \tg_assert(gst_caps_is_writable(caps));\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index a95ae48ac8cfbc98..8a11deb814bc0bfb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -789,7 +789,7 @@ int PipelineHandlerRkISP1::start(Camera *camera)\n>  \t/* Inform IPA of stream configuration and sensor controls. */\n>  \tstd::map<unsigned int, IPAStream> streamConfig;\n>  \tstreamConfig[0] = {\n> -\t\t.pixelFormat = data->stream_.configuration().pixelFormat,\n> +\t\t.pixelFormat = data->stream_.configuration().pixelFormat.fourcc(),\n\nNote for later, IPAStream needs to be extended with modifiers. Could you\nwrite this down in the tasks list ?\n\n>  \t\t.size = data->stream_.configuration().size,\n>  \t};\n>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 1de091e0c0e57f7c..731149755728f209 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -113,8 +113,9 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()\n>  \tif (iter == pixelFormats.end()) {\n>  \t\tcfg.pixelFormat = pixelFormats.front();\n>  \t\tLOG(UVC, Debug)\n> -\t\t\t<< \"Adjusting pixel format from \" << pixelFormat\n> -\t\t\t<< \" to \" << cfg.pixelFormat;\n> +\t\t\t<< \"Adjusting pixel format from \"\n> +\t\t\t<< pixelFormat.toString() << \" to \"\n> +\t\t\t<< cfg.pixelFormat.toString();\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 04cad94e739e9ae9..2e2162b2bf4477c5 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -103,7 +103,7 @@ private:\n>  \n>  namespace {\n>  \n> -constexpr std::array<PixelFormat, 3> pixelformats{\n> +static const std::array<PixelFormat, 3> pixelformats{\n>  \tDRM_FORMAT_RGB888,\n>  \tDRM_FORMAT_BGR888,\n>  \tDRM_FORMAT_BGRA8888,\n> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index c03335400b709d9b..929578fed3e22c92 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -15,14 +15,91 @@\n>  namespace libcamera {\n>  \n>  /**\n> - * \\typedef PixelFormat\n> + * \\class PixelFormat\n>   * \\brief libcamera image pixel format\n>   *\n>   * The PixelFormat type describes the format of images in the public libcamera\n> - * API. It stores a FourCC value as a 32-bit unsigned integer. The values are\n> - * defined in the Linux kernel DRM/KMS API (see linux/drm_fourcc.h).\n> - *\n> - * \\todo Add support for format modifiers\n> + * API. It stores a FourCC value as a 32-bit unsigned integer and a set of\n> + * modifiers. The FourCC and modifiers values are defined in the Linux kernel\n> + * DRM/KMS API (see linux/drm_fourcc.h).\n>   */\n>  \n> +PixelFormat::PixelFormat()\n\nThis should be documented.\n\n/**\n * \\brief Construct a PixelFormat with an invalid format\n *\n * PixelFormat instances constructed with the default constructor are\n * invalid, calling the isValid() function returns false.\n */\n\n> +\t: fourcc_(0)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Construct a PixelFormat from a DRM FourCC and a set of modifiers\n> + * \\param[in] fourcc A DRM FourCC\n> + * \\param[in] modifiers A set of DRM FourCC modifiers\n> + */\n> +PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)\n> +\t: fourcc_(fourcc), modifiers_(modifiers)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Compare pixel formats for equality\n> + * \\return True if the two pixel formats are equal, false otherwise\n> + */\n> +bool PixelFormat::operator==(const PixelFormat &other) const\n> +{\n> +\treturn fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;\n> +}\n> +\n> +/**\n> + * \\fn bool PixelFormat::operator!=(const PixelFormat &other) const\n> + * \\brief Compare pixel formats for inequality\n> + * \\return True if the two pixel formats are not equal, false otherwise\n> + */\n> +\n> +/**\n> + * \\brief Compare pixel formats for smaller than order\n> + * \\return True if \\a this is smaller than \\a other, false otherwise\n> + */\n> +bool PixelFormat::operator<(const PixelFormat &other) const\n> +{\n> +\tif (fourcc_ < other.fourcc_)\n> +\t\treturn true;\n> +\tif (fourcc_ > other.fourcc_)\n> +\t\treturn false;\n> +\treturn modifiers_ < modifiers_;\n> +}\n> +\n> +/**\n> + * \\fn bool PixelFormat::isValid() const\n> + * \\brief Check if the pixel format is valid\n> + * \\return True if the pixel format has a non-zero value, false otherwise\n\nMaybe we should avoid talking about a non-zero value if we want to keep\nthat an implementation detail ?\n\n/**\n * \\fn bool PixelFormat::isValid() const\n * \\brief Check if the pixel format is valid\n *\n * PixelFormat instances constructed with the default constructor are\n * invalid. Instances constructed with a FourCC defined in the DRM API\n * are valid. The behaviour is undefined otherwise.\n *\n * \\return True if the pixel format is valid, false otherwise\n */\n\nUp to you.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> + */\n> +\n> +/**\n> + * \\fn PixelFormat::operator uint32_t() const\n> + * \\brief Convert the the pixel format numerical value\n> + * \\return The pixel format numerical value\n> + */\n> +\n> +/**\n> + * \\fn PixelFormat::fourcc() const\n> + * \\brief Retrieve the pixel format FourCC\n> + * \\return DRM FourCC\n> + */\n> +\n> +/**\n> + * \\fn PixelFormat::modifiers() const\n> + * \\brief Retrieve the pixel format modifiers\n> + * \\return Set of DRM modifiers\n> + */\n> +\n> +/**\n> + * \\brief Assemble and return a string describing the pixel format\n> + * \\return A string describing the pixel format\n> + */\n> +std::string PixelFormat::toString() const\n> +{\n> +\tchar str[11];\n> +\tsnprintf(str, 11, \"0x%08x\", fourcc_);\n> +\treturn str;\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 13789e9eb344f95c..0716de388bd81d80 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -347,9 +347,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n>   */\n>  std::string StreamConfiguration::toString() const\n>  {\n> -\tstd::stringstream ss;\n> -\tss << size.toString() << \"-\" << utils::hex(pixelFormat);\n> -\treturn ss.str();\n> +\treturn size.toString() + \"-\" + pixelFormat.toString();\n>  }\n>  \n>  /**\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 3869766046236f34..c8ba0f8cebedb91a 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1647,7 +1647,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n>   */\n>  uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n>  {\n> -\tswitch (pixelFormat) {\n> +\tswitch (pixelFormat.fourcc()) {\n>  \t/* RGB formats. */\n>  \tcase DRM_FORMAT_BGR888:\n>  \t\treturn V4L2_PIX_FMT_RGB24;\n> @@ -1691,8 +1691,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar\n>  \t * class. Until we fix the logger, work around it.\n>  \t */\n>  \tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), LogError).stream()\n> -\t\t<< \"Unsupported V4L2 pixel format \"\n> -\t\t<< utils::hex(pixelFormat);\n> +\t\t<< \"Unsupported V4L2 pixel format \" << pixelFormat.toString();\n>  \treturn 0;\n>  }\n>  \n> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp\n> index 7b8ad77c42fe85d4..66d07025ac9578ca 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -28,7 +28,7 @@\n>  int FormatConverter::configure(libcamera::PixelFormat format, unsigned int width,\n>  \t\t\t       unsigned int height)\n>  {\n> -\tswitch (format) {\n> +\tswitch (format.fourcc()) {\n>  \tcase DRM_FORMAT_NV12:\n>  \t\tformatFamily_ = NV;\n>  \t\thorzSubSample_ = 2;\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index e349fbddc2a4d5a2..c6d1e5030b58b630 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -533,7 +533,7 @@ struct PixelFormatInfo {\n>  \n>  namespace {\n>  \n> -constexpr std::array<PixelFormatInfo, 13> pixelFormatInfo = {{\n> +static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{\n>  \t/* RGB formats. */\n>  \t{ DRM_FORMAT_RGB888,\tV4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n>  \t{ DRM_FORMAT_BGR888,\tV4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> @@ -605,7 +605,7 @@ uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)\n>  \t\t\t\t\t return info.format == format;\n>  \t\t\t\t });\n>  \tif (info == pixelFormatInfo.end())\n> -\t\treturn format;\n> +\t\treturn format.fourcc();\n>  \n>  \treturn info->v4l2Format;\n>  }","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B7A060418\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 18 Mar 2020 14:37:45 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D55ACF9;\n\tWed, 18 Mar 2020 14:37:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584538665;\n\tbh=PCuvrQJ4pU1yXPPiNz76lMdVwJMWnT/BYR3WzjYRLdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iXWQ6hSda+yVBaCtsZbrHwNwS1v6EIgQWnOWaDEOuRX2sEwopzVVcwBKb3yYHlvbQ\n\tN+wp8zoLOzMaG0Bz+l8OpLYRW/YX4KVA4IMJKBcsmEwjTfp7a1JS5J5DLqeFLxogPq\n\tRq8bi1YIgAXQpySsAWr4YwBCiMmvwRaM5IKw1Fkg=","Date":"Wed, 18 Mar 2020 15:37:39 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200318133739.GL4733@pendragon.ideasonboard.com>","References":"<20200318033200.3042855-1-niklas.soderlund@ragnatech.se>\n\t<20200318033200.3042855-7-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200318033200.3042855-7-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 6/8] libcamera: PixelFormat: Turn\n\tinto a class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Wed, 18 Mar 2020 13:37:45 -0000"}}]