[{"id":4046,"web_url":"https://patchwork.libcamera.org/comment/4046/","msgid":"<20200317111206.GF4864@pendragon.ideasonboard.com>","date":"2020-03-17T11:12:06","subject":"Re: [libcamera-devel] [PATCH v2 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 Tue, Mar 17, 2020 at 04:52:37AM +0100, Niklas Söderlund wrote:\n> Create a class to represent a pixel formats. This is done to add support\n\ns/formats/format/\n\n> for modifiers for the formats. So far no formats are added by any\n\nDo you mean no modifiers, not no formats ?\n\n> pipeline handler, all plumbing to deal with them is however in place.\n> \n> Pipelines that adds modifiers will come when support for RAW capture is\n\ns/Pipelines/Pipeline handlers/\ns/adds/add/\n\nI think I'd drop this sentence anyway.\n\n> added.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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         | 21 ++++++-\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/pixelformats.cpp           | 78 ++++++++++++++++++++++--\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>  10 files changed, 110 insertions(+), 25 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 6e25b8d8b76e5961..5ba1ba1b324272b9 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -7,11 +7,30 @@\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>  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> +\tuint32_t fourcc() const { return fourcc_; }\n\nWill you add a uint32_t conversion operator ? It could be done on top,\nbut it would remove the need for some of the explicit calls to fourcc()\nbelow, so I think it would be nice to add it to this patch.\n\n> +\tconst std::set<uint64_t> &modifiers() const { return modifiers_; }\n> +\tstd::string toString() const;\n> +\n> +private:\n> +\tuint32_t fourcc_;\n> +\tstd::set<uint64_t> modifiers_;\n> +};\n> +\n> +bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);\n> +bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);\n> +bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);\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>  \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 01977ad697a91a44..8223b82c4a9c773c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -790,7 +790,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>  \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 5ebd83f3c2099ffe..12af164590020142 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/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index c03335400b709d9b..fe9a6a2576978647 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -7,6 +7,8 @@\n>  \n>  #include <libcamera/pixelformats.h>\n>  \n> +#include \"log.h\"\n> +\n\nNot needed.\n\n>  /**\n>   * \\file pixelformats.h\n>   * \\brief libcamera pixel formats\n> @@ -15,14 +17,80 @@\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> +\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> + * \\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> +/**\n> + * \\brief Compare pixel formats for equality\n> + * \\return True if the two pixel formats are equal, false otherwise\n> + */\n> +bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)\n> +{\n> +\treturn lhs.fourcc() == rhs.fourcc() &&\n> +\t       lhs.modifiers() == rhs.modifiers();\n> +}\n> +\n> +/**\n> + * \\brief Compare pixel formats for inequality\n> + * \\return True if the two pixel formats are not equal, false otherwise\n> + */\n> +bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)\n> +{\n> +\treturn !(lhs == rhs);\n> +}\n\nThis one should really be inlined, it will save a function call.\n\n> +\n> +/**\n> + * \\brief Compare pixel formats for smaller than order\n> + * \\todo Take modifiers into account if \\a lhs == \\a rhs.\n\nHow about doing so already ?\n\n> + * \\return True if \\a lhs is smaller than \\a rhs, false otherwise\n> + */\n> +bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)\n> +{\n> +\treturn lhs.fourcc() < rhs.fourcc();\n\n\tif (lhs.fourcc() < rhs.fourcc())\n\t\treturn true;\n\tif (lhs.fourcc() > rhs.fourcc())\n\t\treturn false;\n\treturn lhs.modifiers() < rhs.modifiers();\n\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 280cf1877c8936d7..b5762a7eabcf4e25 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1643,7 +1643,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> @@ -1687,8 +1687,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 368cb43fbf17ae4d..c071ea9b4022750c 100644\n> --- a/src/qcam/format_converter.cpp\n> +++ b/src/qcam/format_converter.cpp\n> @@ -30,7 +30,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 b620f236499cf77d..3bbbbf79cdb475db 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -534,7 +534,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> @@ -606,7 +606,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 8040362923\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Mar 2020 12:12:12 +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 E78FAF9;\n\tTue, 17 Mar 2020 12:12:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584443532;\n\tbh=t+vbUpkSkRYviT0iGmEspEVQ/Yp4dvX296YIDnu1qwU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gm5ZXYR+AN7UnZiXH3feBsJUsjrag8sMs4893m85I/J1n8V6pD+/fT9/s3L8qNkun\n\t7ocznZpRl+CCdxxrxrCw3qbT8ZhzSQQlWvSGmlfQncn6b0oap+ufi9GzZNRE36JMiu\n\tgiw5h0rvnj9CzvDmU2uyAybdLqK+xKrk5gawY29U=","Date":"Tue, 17 Mar 2020 13:12:06 +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":"<20200317111206.GF4864@pendragon.ideasonboard.com>","References":"<20200317035239.2697679-1-niklas.soderlund@ragnatech.se>\n\t<20200317035239.2697679-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":"<20200317035239.2697679-7-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 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":"Tue, 17 Mar 2020 11:12:12 -0000"}}]