[{"id":4020,"web_url":"https://patchwork.libcamera.org/comment/4020/","msgid":"<20200316074159.GI4732@pendragon.ideasonboard.com>","date":"2020-03-16T07:41:59","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: PixelFormat: Turn into\n\ta 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 Mon, Mar 16, 2020 at 03:40:36AM +0100, Niklas Söderlund wrote:\n> Create a class to represent a pixel formats. This is done to add support\n> for modifiers for the formats. So far no formats are added by any\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> added.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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         |  24 +++++-\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           | 101 +++++++++++++++++++++--\n>  src/libcamera/stream.cpp                 |   2 +-\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, 136 insertions(+), 23 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 6e25b8d8b76e5961..ce8f9f93b767ec23 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -7,11 +7,33 @@\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\nShould this be marked as explicit ? I know it will result in a larger\npatch, but I think it would be useful to review all call sites. You will\nfind a rather interesting one in the UVC pipeline handler :-) You may\nwant to write a preparatory patch to fix the UVC issue, and maybe add\nthe explicit keyword in a separate patch on top of this one.\n\n> +\n> +\tPixelFormat(const PixelFormat &other);\n> +\tPixelFormat &operator=(const PixelFormat &other);\n> +\n> +\tuint32_t fourcc() const { return fourcc_; }\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\nTo make this class more lightweight, could this be turned into an array\n? DRM has a limit of 4 modifiers, we could do the same too. It may allow\nturning the constructors into constexpr.\n\nI'm also wondering if we shouldn't turn most PixelFormat arguments to\nfunction to const references instead of values. That can of course be\ndone on top of this patch.\n\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\nIf the constructor becomes explicit, you could make these functions\nclass members. Up to you.\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>  \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 b5f98a96a14fe348..d1954c429cb300e8 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..46f2e3883989f522 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>  /**\n>   * \\file pixelformats.h\n>   * \\brief libcamera pixel formats\n> @@ -14,15 +16,104 @@\n>  \n>  namespace libcamera {\n>  \n> +LOG_DEFINE_CATEGORY(PixelFormats)\n> +\n\nYou don't log any message here, I think you can drop that.\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 values are defined in the Linux kernel DRM/KMS API (see\n\nI would write \"The FourCC and modifiers values are defined in ...\".\n\n> + * 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\ns/fourcc/FourCC/ here and below in all comments (but not for variable\nnames).\n\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\nI would inline the constructors.\n\n> +\n> +/**\n> + * \\brief Construct a PixelFormat from \\a other\n> + * \\param[in] other Other PixelFormat\n> + */\n> +PixelFormat::PixelFormat(const PixelFormat &other)\n> +\t: PixelFormat(other.fourcc_, other.modifiers_)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Copy assignment operator, copy the pixel format from \\a other\n> + * \\param[in] other Other PixelFormat\n> + */\n> +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> +{\n> +\tfourcc_ = other.fourcc_;\n> +\tmodifiers_ = other.modifiers_;\n> +\n> +\treturn *this;\n> +}\n\nI don't think you need custom a copy constructor and copy assignment\noperator, the defaults should do.\n\n> +\n> +/**\n> + * \\fn PixelFormat::fourcc() const\n> + * \\brief Retrive the pixel formats fourcc\n\ns/Retrive/Retrieve/\ns/formats/format/\n\n> + * \\return DRM fourcc code\n> + */\n> +\n> +/**\n> + * \\fn PixelFormat::modifiers() const\n> + * \\brief Retrive the pixel formats modifiers\n\nDitto\n\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> +\tstd::stringstream ss;\n> +\tss << utils::hex(fourcc_, 8);\n\nNo need to specify the second argument.\n\n> +\treturn ss.str();\n\nThis is pretty heavy just to print a hex number, isn't it ? How about\n\n\tchar str[11];\n\tsnprintf(str, \"0x%08x\", fourcc_);\n\treturn str;\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 operator==(const PixelFormat &lhs, const PixelFormat &rhs)\n> +{\n> +\treturn lhs.fourcc() == rhs.fourcc() &&\n> +\t\tlhs.modifiers() == rhs.modifiers();\n\nAlignment.\n\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\nYou can inline this.\n\n> +\n> +/**\n> + * \\brief Compare pixel formats for smaller than order\n> + * \\todo Take modifiers into account if \\a lhs == \\a rhs.\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\nWhy do you need this operator ?\n\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 13789e9eb344f95c..f34ea08de03b744e 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n>  std::string StreamConfiguration::toString() const\n>  {\n>  \tstd::stringstream ss;\n> -\tss << size.toString() << \"-\" << utils::hex(pixelFormat);\n> +\tss << size.toString() << \"-\" << pixelFormat.toString();\n>  \treturn ss.str();\n\nWould\n\n\treturn size.toString() + \"-\" + pixelFormat.toString();\n\nbe more efficient ?\n\n>  }\n>  \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index f9c59d5e3fb65737..66f7347dc2e4f7fd 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 C276E60417\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Mar 2020 08:42:05 +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 D1B492D6;\n\tMon, 16 Mar 2020 08:42:04 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584344525;\n\tbh=/WtljzPZq7keYLAG+T/4fem5VeUJabHeeHgoXcNNjnY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HIeRzLFVD1twKaaNoT/VpgxpYIBIKo8CMAjziuCI9UGi4jockSWlxE2W2nHGdsbSb\n\tBwgFXLtJxmhjXsh2VFPwRCw37rHrREKXkeJCR2M20WLkIYhrb74ScYcW9hvm8Rfpvd\n\twx7eLMyV+eBSeEPMGRd1IWRWAnUtID30jxNLJ3es=","Date":"Mon, 16 Mar 2020 09:41:59 +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":"<20200316074159.GI4732@pendragon.ideasonboard.com>","References":"<20200316024036.2474307-1-niklas.soderlund@ragnatech.se>\n\t<20200316024036.2474307-5-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":"<20200316024036.2474307-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: PixelFormat: Turn into\n\ta 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":"Mon, 16 Mar 2020 07:42:06 -0000"}},{"id":4037,"web_url":"https://patchwork.libcamera.org/comment/4037/","msgid":"<20200317034130.GA2412909@oden.dyn.berto.se>","date":"2020-03-17T03:41:30","subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: PixelFormat: Turn into\n\ta class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-03-16 09:41:59 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Mon, Mar 16, 2020 at 03:40:36AM +0100, Niklas Söderlund wrote:\n> > Create a class to represent a pixel formats. This is done to add support\n> > for modifiers for the formats. So far no formats are added by any\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> > added.\n> > \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\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         |  24 +++++-\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           | 101 +++++++++++++++++++++--\n> >  src/libcamera/stream.cpp                 |   2 +-\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, 136 insertions(+), 23 deletions(-)\n> > \n> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> > index 6e25b8d8b76e5961..ce8f9f93b767ec23 100644\n> > --- a/include/libcamera/pixelformats.h\n> > +++ b/include/libcamera/pixelformats.h\n> > @@ -7,11 +7,33 @@\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> Should this be marked as explicit ? I know it will result in a larger\n> patch, but I think it would be useful to review all call sites. You will\n> find a rather interesting one in the UVC pipeline handler :-) You may\n> want to write a preparatory patch to fix the UVC issue, and maybe add\n> the explicit keyword in a separate patch on top of this one.\n\nI will mimic your V4L2PixelFormat work and have it done on top.\n\n> \n> > +\n> > +\tPixelFormat(const PixelFormat &other);\n> > +\tPixelFormat &operator=(const PixelFormat &other);\n> > +\n> > +\tuint32_t fourcc() const { return fourcc_; }\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> To make this class more lightweight, could this be turned into an array\n> ? DRM has a limit of 4 modifiers, we could do the same too. It may allow\n> turning the constructors into constexpr.\n\nWe could, but not the API be more cumber some to use?\n\nWhat I like about std::set here is that the API guarantees we have no \nduplicated modifiers in the same group and that the iteration order over \nthem will always be the same as it's sorted.\n\n> \n> I'm also wondering if we shouldn't turn most PixelFormat arguments to\n> function to const references instead of values. That can of course be\n> done on top of this patch.\n\nGood idea!\n\n> \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> If the constructor becomes explicit, you could make these functions\n> class members. Up to you.\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> >  \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 b5f98a96a14fe348..d1954c429cb300e8 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..46f2e3883989f522 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> >  /**\n> >   * \\file pixelformats.h\n> >   * \\brief libcamera pixel formats\n> > @@ -14,15 +16,104 @@\n> >  \n> >  namespace libcamera {\n> >  \n> > +LOG_DEFINE_CATEGORY(PixelFormats)\n> > +\n> \n> You don't log any message here, I think you can drop that.\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 values are defined in the Linux kernel DRM/KMS API (see\n> \n> I would write \"The FourCC and modifiers values are defined in ...\".\n> \n> > + * 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> \n> s/fourcc/FourCC/ here and below in all comments (but not for variable\n> names).\n> \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> I would inline the constructors.\n\nOver all in the public headers we don't don't inline constructors, check \ntimer.h for example. I don't feel strongly about this but I slightly \nprefer to keep the documentation and implementation close to each other, \nelse it's so easy to forget it.\n\n> \n> > +\n> > +/**\n> > + * \\brief Construct a PixelFormat from \\a other\n> > + * \\param[in] other Other PixelFormat\n> > + */\n> > +PixelFormat::PixelFormat(const PixelFormat &other)\n> > +\t: PixelFormat(other.fourcc_, other.modifiers_)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Copy assignment operator, copy the pixel format from \\a other\n> > + * \\param[in] other Other PixelFormat\n> > + */\n> > +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> > +{\n> > +\tfourcc_ = other.fourcc_;\n> > +\tmodifiers_ = other.modifiers_;\n> > +\n> > +\treturn *this;\n> > +}\n> \n> I don't think you need custom a copy constructor and copy assignment\n> operator, the defaults should do.\n\nYou are correct, this is a relic form the RFC. Thanks!\n\n> \n> > +\n> > +/**\n> > + * \\fn PixelFormat::fourcc() const\n> > + * \\brief Retrive the pixel formats fourcc\n> \n> s/Retrive/Retrieve/\n> s/formats/format/\n> \n> > + * \\return DRM fourcc code\n> > + */\n> > +\n> > +/**\n> > + * \\fn PixelFormat::modifiers() const\n> > + * \\brief Retrive the pixel formats modifiers\n> \n> Ditto\n> \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> > +\tstd::stringstream ss;\n> > +\tss << utils::hex(fourcc_, 8);\n> \n> No need to specify the second argument.\n> \n> > +\treturn ss.str();\n> \n> This is pretty heavy just to print a hex number, isn't it ? How about\n> \n> \tchar str[11];\n> \tsnprintf(str, \"0x%08x\", fourcc_);\n> \treturn str;\n\nI love it, I have little love for streams ;-)\n\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 operator==(const PixelFormat &lhs, const PixelFormat &rhs)\n> > +{\n> > +\treturn lhs.fourcc() == rhs.fourcc() &&\n> > +\t\tlhs.modifiers() == rhs.modifiers();\n> \n> Alignment.\n> \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> \n> You can inline this.\n> \n> > +\n> > +/**\n> > + * \\brief Compare pixel formats for smaller than order\n> > + * \\todo Take modifiers into account if \\a lhs == \\a rhs.\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> \n> Why do you need this operator ?\n\nIt's needed as we store PixelFormats in containers with a sorting order \nsuch as std::map<PixelFormat, ...>.\n\n> \n> > +\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index 13789e9eb344f95c..f34ea08de03b744e 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -348,7 +348,7 @@ StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> >  std::string StreamConfiguration::toString() const\n> >  {\n> >  \tstd::stringstream ss;\n> > -\tss << size.toString() << \"-\" << utils::hex(pixelFormat);\n> > +\tss << size.toString() << \"-\" << pixelFormat.toString();\n> >  \treturn ss.str();\n> \n> Would\n> \n> \treturn size.toString() + \"-\" + pixelFormat.toString();\n> \n> be more efficient ?\n> \n> >  }\n> >  \n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index f9c59d5e3fb65737..66f7347dc2e4f7fd 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> >  }\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x229.google.com (mail-lj1-x229.google.com\n\t[IPv6:2a00:1450:4864:20::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F0A6D60416\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Mar 2020 04:41:32 +0100 (CET)","by mail-lj1-x229.google.com with SMTP id r24so21189929ljd.4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 16 Mar 2020 20:41:32 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tj84sm1290379lfd.77.2020.03.16.20.41.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 16 Mar 2020 20:41:30 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=8La97GfNcmOhvS5FRZ0ZousAqMqaht1GgStri3uawl4=;\n\tb=gFLcgWI5iWUXc8RoAxpnXWcmUFH+a9v7QwXF0if1vPgWArw0Uq4H7+PhSA2jKropOR\n\tObAVN/71d+8GaoRQre4sYgr20DJ+/K1qts5eksnKnhpY45kWRHIRN9ywpC/wHpNZveQx\n\tvkRJ0uCcpSNbM6r2bYpSOYf7bJ0nj07r2lK5blRH18cNDfhri7qh07ZbnQrr5/25r1iM\n\tH1dy3laLx15gDmOEznf02iqxPU8Ib3I5yY+ZPMd+NpCXTKMOXWXLI80fgmGOrK7GxA2I\n\tFas+lv1k3jPKcDguySCz0w3vT0iw7dG1GTzjHEPN/G4j1Y/4VOnGIi1Mcxa663H45akl\n\thk1g==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=8La97GfNcmOhvS5FRZ0ZousAqMqaht1GgStri3uawl4=;\n\tb=KidtLHeNnBI04TA3uO+a6QDBTKPL0hPMCT6a6qoKQ2BmtF7FqbEKkEUGbhkujDjsgZ\n\tk21Aricrm2BBhCUOLUugBPD1fflRS2X1g9rYrUuYuIVptDAAB/6pGce+1EmsYYilNwnu\n\t4Nko++KOyOC1QBMSiQgbNi2J//csv6Zc1veCcJdOxsq4SXZi68Hy1/aPCA2dAJhbAFLq\n\tNqkTB5xRHkcBmtFgSHV61Iqvy3hv4WbHRoGQfvmVwcqBxuiQI0H+KIAR7LDcb+W6MU9M\n\tytvWtMndy1W9FrSDu5oktJ7SWGNFcTD5P6T4eOSfAqNVEH4gsXxjJBSeK5OxUk5lfDvE\n\tTs7w==","X-Gm-Message-State":"ANhLgQ03CkpZlKD+Mq6RzLNJOhRkJvJLQ3AQZ+FA6wMfvqDIuDgGoEgz\n\txcoeRRLYrFXE+6fyLC4i7OB/nFDHL84=","X-Google-Smtp-Source":"ADFU+vtyX9EoqYuYx2raWE0M/YmN864f8PQ0WVSmdoUYe1ATVmLpY6Nn0V5/srD+Ybo+T7rbMYkYeg==","X-Received":"by 2002:a2e:b008:: with SMTP id y8mr1366597ljk.107.1584416491815;\n\tMon, 16 Mar 2020 20:41:31 -0700 (PDT)","Date":"Tue, 17 Mar 2020 04:41:30 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200317034130.GA2412909@oden.dyn.berto.se>","References":"<20200316024036.2474307-1-niklas.soderlund@ragnatech.se>\n\t<20200316024036.2474307-5-niklas.soderlund@ragnatech.se>\n\t<20200316074159.GI4732@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200316074159.GI4732@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 4/4] libcamera: PixelFormat: Turn into\n\ta 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 03:41:33 -0000"}}]