[{"id":3850,"web_url":"https://patchwork.libcamera.org/comment/3850/","msgid":"<20200228102737.6w3dw2fcm2bcjmxu@uno.localdomain>","date":"2020-02-28T10:27:37","subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Feb 28, 2020 at 04:29:10AM +0100, Niklas Söderlund wrote:\n> Create a class to represent the pixel formats. This is done to prepare\n> to add support for modifiers for the formats. One bonus with this change\n> it that it's allows it to make it impossible for other then designated\n> classes (V4L2VideoDevice and V4L2CameraProxy) to create PixelFormat\n> instances from a V4L2 fourcc limiting the risk of users of the class to\n> mix the two fourcc namespaces unintentionally.\n>\n> The patch is unfortunately quiet large as it have to touch a lot of\n> different ares of the code to simultaneously switch to the new class\n> based PixelFormat implementation.\n>\n\nI prasie the effort of making PixelFormat more descriptive and\ncentralize there the conversion between different 4cc sets, but I'm\nnot sure I like the way pipeline handlers can now construct\nPixelFormat from both DRM and V4L2 fourcc. Please see below.\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/pixelformats.h         |  40 +++++++-\n>  src/cam/main.cpp                         |   4 +-\n>  src/libcamera/formats.cpp                |   3 +\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   6 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  22 ++--\n>  src/libcamera/pipeline/uvcvideo.cpp      |   4 +-\n>  src/libcamera/pipeline/vimc.cpp          |  12 +--\n>  src/libcamera/pixelformats.cpp           | 124 +++++++++++++++++++++++\n>  src/libcamera/stream.cpp                 |   6 +-\n>  src/libcamera/v4l2_videodevice.cpp       | 101 +-----------------\n>  src/qcam/format_converter.cpp            |   2 +-\n>  src/qcam/viewfinder.cpp                  |   2 +-\n>  src/v4l2/v4l2_camera_proxy.cpp           |  49 ++++-----\n>  test/stream/stream_formats.cpp           |  33 +++---\n>  14 files changed, 236 insertions(+), 172 deletions(-)\n>\n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 6e25b8d8b76e5961..f0951e983192d5e8 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -7,11 +7,49 @@\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> +/* Needs to be forward declared in the global namespace. */\n> +class V4L2CameraProxy;\n>\n>  namespace libcamera {\n>\n> -using PixelFormat = uint32_t;\n> +struct PixelFormatEntry;\n> +\n> +class PixelFormat\n> +{\n> +public:\n> +\tPixelFormat();\n> +\tPixelFormat(const PixelFormat &other);\n> +\texplicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);\n> +\n> +\tPixelFormat &operator=(const PixelFormat &other);\n> +\n> +\tbool operator==(const PixelFormat &other) const;\n> +\tbool operator!=(const PixelFormat &other) const;\n> +\tbool operator<(const PixelFormat &other) const;\n> +\n> +\tuint32_t v4l2() const;\n> +\tuint32_t fourcc() const;\n> +\tconst std::set<uint32_t> &modifiers() const;\n> +\n> +protected:\n> +\t/* Needed so V4L2VideoDevice can create PixelFormat from V4L2 fourcc. */\n> +\tfriend class V4L2VideoDevice;\n> +\n> +\t/* Needed so V4L2CameraProxy can create PixelFormat from V4L2 fourcc. */\n> +\tfriend V4L2CameraProxy;\n> +\n> +\texplicit PixelFormat(uint32_t v4l2_fourcc);\n> +\n> +private:\n> +\tconst PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;\n> +\tconst PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;\n> +\n> +\tconst PixelFormatEntry *format_;\n> +};\n>\n>  } /* namespace libcamera */\n>\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index ad4be55fc114fe22..c8ef79daea37d8b6 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -249,7 +249,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> @@ -283,7 +283,7 @@ int CamApp::infoConfiguration()\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\t\t  << std::setw(8) << pixelformat.fourcc() << \" \"\n>  \t\t\t\t  << formats.range(pixelformat).toString()\n>  \t\t\t\t  << std::endl;\n>\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 98817deee2b54c84..e3a89121e3c60151 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -9,6 +9,8 @@\n>\n>  #include <errno.h>\n>\n> +#include <libcamera/pixelformats.h>\n> +\n>  /**\n>   * \\file formats.h\n>   * \\brief Types and helper methods to handle libcamera image formats\n> @@ -110,5 +112,6 @@ const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const\n>  }\n>\n>  template class ImageFormats<unsigned int>;\n> +template class ImageFormats<PixelFormat>;\n>\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 33f97b340716abd0..71ac0ae33921f548 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -247,7 +247,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n>  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n>  {\n>  \t/* The only pixel format the driver supports is NV12. */\n> -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n\nThis is good. However you use the second parameter to differentiate\nbetween constructing a PixelFormat from DRM or V4L2, which seems a bit\nfragile and prevents you from defaulting the second parameter to an\nempty modifier. What is the point of requesting to provide {} if no\nmodifier is associated with the format ?\n\n>\n>  \tif (scale) {\n>  \t\t/*\n> @@ -402,7 +402,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\tStreamConfiguration cfg = {};\n>  \t\tIPU3Stream *stream = nullptr;\n>\n> -\t\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>\n>  \t\tswitch (role) {\n>  \t\tcase StreamRole::StillCapture:\n> @@ -1142,7 +1142,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n>  \t\treturn 0;\n>\n>  \tV4L2DeviceFormat outputFormat = {};\n> -\toutputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);\n> +\toutputFormat.fourcc = PixelFormat(DRM_FORMAT_NV12, {}).v4l2();\n\nHave you consider providing a static method to perform the DRM<->v4l2\nfourcc conversion ? Here you create a PixelFormat instance just to\ncall .v4l2() on it. The only thing you care about is the translation\nbetween the two 4cc codes, wouldn't this be better expressed as\n        PixelFormat::toV4L2(DRM_FORMAT_NV12);\n?\n\n>  \toutputFormat.size = cfg.size;\n>  \toutputFormat.planesCount = 2;\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 13433b216747cb8b..323fa3596c6ee242 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -433,14 +433,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>\n>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  {\n> -\tstatic const std::array<unsigned int, 8> formats{\n> -\t\tDRM_FORMAT_YUYV,\n> -\t\tDRM_FORMAT_YVYU,\n> -\t\tDRM_FORMAT_VYUY,\n> -\t\tDRM_FORMAT_NV16,\n> -\t\tDRM_FORMAT_NV61,\n> -\t\tDRM_FORMAT_NV21,\n> -\t\tDRM_FORMAT_NV12,\n> +\tstatic const std::array<PixelFormat, 8> formats{\n> +\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> +\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> +\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV16, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV61, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV21, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV12, {}),\n>  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>  \t};\n>\n> @@ -462,7 +462,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n>  \t    formats.end()) {\n\nI am not sure what we get by making pipeline handler use the\nPixelFormat class here just to enumerate and match 4cc codes.\n\n>  \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> -\t\tcfg.pixelFormat = DRM_FORMAT_NV12,\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n\nHere instead is good. Application should be provided a PixelFormat\nclass\n\n>  \t\tstatus = Adjusted;\n>  \t}\n>\n> @@ -541,7 +541,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n>  \t\treturn config;\n>\n>  \tStreamConfiguration cfg{};\n> -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>  \tcfg.size = data->sensor_->resolution();\n>\n>  \tconfig->addConfiguration(cfg);\n> @@ -796,7 +796,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 8efd188e75a3d135..5f3e52f691aaeae4 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -115,8 +115,8 @@ 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 \" << pixelFormat.fourcc()\n> +\t\t\t<< \" to \" << cfg.pixelFormat.fourcc();\n>  \t\tstatus = Adjusted;\n>  \t}\n>\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 9fbe33c626e327d4..a591c424919b0783 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -106,10 +106,10 @@ private:\n>\n>  namespace {\n>\n> -constexpr std::array<unsigned int, 3> pixelformats{\n> -\tDRM_FORMAT_RGB888,\n> -\tDRM_FORMAT_BGR888,\n> -\tDRM_FORMAT_BGRA8888,\n> +static const std::array<PixelFormat, 3> pixelformats{\n> +\tPixelFormat(DRM_FORMAT_RGB888, {}),\n> +\tPixelFormat(DRM_FORMAT_BGR888, {}),\n> +\tPixelFormat(DRM_FORMAT_BGRA8888, {}),\n>  };\n>\n>  } /* namespace */\n> @@ -138,7 +138,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n>  \tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n>  \t    pixelformats.end()) {\n>  \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> -\t\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n>  \t\tstatus = Adjusted;\n>  \t}\n>\n> @@ -187,7 +187,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>\n>  \tStreamConfiguration cfg(formats.data());\n>\n> -\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n>  \tcfg.size = { 1920, 1080 };\n>  \tcfg.bufferCount = 4;\n>\n> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index c03335400b709d9b..b2aacbc39b9ca16a 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -7,6 +7,13 @@\n>\n>  #include <libcamera/pixelformats.h>\n>\n> +#include <vector>\n> +\n> +#include <linux/drm_fourcc.h>\n> +#include <linux/videodev2.h>\n> +\n> +#include \"log.h\"\n> +\n>  /**\n>   * \\file pixelformats.h\n>   * \\brief libcamera pixel formats\n> @@ -14,6 +21,8 @@\n>\n>  namespace libcamera {\n>\n> +LOG_DEFINE_CATEGORY(PixelFormats)\n> +\n>  /**\n>   * \\typedef PixelFormat\n>   * \\brief libcamera image pixel format\n> @@ -25,4 +34,119 @@ namespace libcamera {\n>   * \\todo Add support for format modifiers\n>   */\n>\n> +struct PixelFormatEntry {\n> +\tuint32_t v4l2;\n> +\tuint32_t drm;\n> +\tstd::set<uint32_t> modifiers;\n> +};\n> +\n> +static const std::vector<PixelFormatEntry> pixelFormats = {\n> +\t/* Invalid format, important to be first in list. */\n> +\t{ .v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n> +\t/* RGB formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n> +\t/* YUV packed formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n> +\t/* YUY planar formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t/* Compressed formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n> +};\n> +\n> +PixelFormat::PixelFormat()\n> +\t: format_(&pixelFormats[0])\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(const PixelFormat &other)\n> +\t: format_(other.format_)\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers)\n> +\t: format_(fromDRM(drm_fourcc, drm_modifiers))\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(uint32_t v4l2_fourcc)\n> +\t: format_(fromV4L2(v4l2_fourcc))\n> +{\n> +}\n\nThis seems very fragile. The distinction between constructing a\nPixelFormat with a DRM 4cc or a V4L2 fourcc (which are both uint32_t)\nonly depends on the presence of the second, right now empty,\nparameter.\n\nWhat I would do is instead make it mandatory to construct a\nPixelFormat with a valid DRM 4cc, and provide to pipeline handler a\nstatic method(s) to convert between drm/v4l2 whenever they consider it\noppotune.\n\n> +\n> +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> +{\n> +\tformat_ = other.format_;\n> +\n> +\treturn *this;\n> +}\n> +\n> +bool PixelFormat::operator==(const PixelFormat &other) const\n> +{\n> +\treturn format_ == other.format_;\n> +}\n> +\n> +bool PixelFormat::operator!=(const PixelFormat &other) const\n> +{\n> +\treturn format_ != other.format_;\n> +}\n> +\n> +bool PixelFormat::operator<(const PixelFormat &other) const\n> +{\n> +\treturn format_ > other.format_;\n> +}\n> +\n> +uint32_t PixelFormat::v4l2() const\n> +{\n> +\treturn format_->v4l2;\n> +}\n> +\n> +uint32_t PixelFormat::fourcc() const\n> +{\n> +\treturn format_->drm;\n> +}\n> +\n> +const std::set<uint32_t> &PixelFormat::modifiers() const\n> +{\n> +\treturn format_->modifiers;\n> +}\n> +\n> +const PixelFormatEntry *PixelFormat::fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const\n> +{\n> +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> +\t\tif (entry.drm == drm_fourcc &&\n> +\t\t    entry.modifiers == drm_modifiers)\n> +\t\t\treturn &entry;\n> +\n> +\tLOG(PixelFormats, Error)\n> +\t\t<< \"Unsupported DRM pixel format \"\n> +\t\t<< utils::hex(drm_fourcc);\n> +\n> +\treturn &pixelFormats[0];\n> +}\n> +\n> +const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const\n> +{\n> +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> +\t\tif (entry.v4l2 == v4l2_fourcc)\n> +\t\t\treturn &entry;\n> +\n> +\tLOG(PixelFormats, Error)\n> +\t\t<< \"Unsupported V4L2 pixel format \"\n> +\t\t<< utils::hex(v4l2_fourcc);\n> +\n> +\treturn &pixelFormats[0];\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 13789e9eb344f95c..dbce550ca8d0b7b1 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(PixelFormat pixelformat) const\n>   * handlers provied StreamFormats.\n>   */\n>  StreamConfiguration::StreamConfiguration()\n> -\t: pixelFormat(0), stream_(nullptr)\n> +\t: pixelFormat(), stream_(nullptr)\n>  {\n>  }\n>\n> @@ -287,7 +287,7 @@ StreamConfiguration::StreamConfiguration()\n>   * \\brief Construct a configuration with stream formats\n>   */\n>  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> -\t: pixelFormat(0), stream_(nullptr), formats_(formats)\n> +\t: pixelFormat(), stream_(nullptr), formats_(formats)\n>  {\n>  }\n>\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() << \"-\" << utils::hex(pixelFormat.fourcc());\n>  \treturn ss.str();\n>  }\n>\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index f84bd00570afa38c..e9d3e60198e140a0 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -846,7 +846,7 @@ ImageFormats<PixelFormat> V4L2VideoDevice::formats()\n>  \t\tif (sizes.empty())\n>  \t\t\treturn {};\n>\n> -\t\tif (formats.addFormat(pixelformat, sizes)) {\n> +\t\tif (formats.addFormat(PixelFormat(pixelformat), sizes)) {\n>  \t\t\tLOG(V4L2, Error)\n>  \t\t\t\t<< \"Could not add sizes for pixel format \"\n>  \t\t\t\t<< pixelformat;\n> @@ -1417,56 +1417,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>   */\n>  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n>  {\n> -\tswitch (v4l2Fourcc) {\n> -\t/* RGB formats. */\n> -\tcase V4L2_PIX_FMT_RGB24:\n> -\t\treturn DRM_FORMAT_BGR888;\n> -\tcase V4L2_PIX_FMT_BGR24:\n> -\t\treturn DRM_FORMAT_RGB888;\n> -\tcase V4L2_PIX_FMT_ARGB32:\n> -\t\treturn DRM_FORMAT_BGRA8888;\n> -\n> -\t/* YUV packed formats. */\n> -\tcase V4L2_PIX_FMT_YUYV:\n> -\t\treturn DRM_FORMAT_YUYV;\n> -\tcase V4L2_PIX_FMT_YVYU:\n> -\t\treturn DRM_FORMAT_YVYU;\n> -\tcase V4L2_PIX_FMT_UYVY:\n> -\t\treturn DRM_FORMAT_UYVY;\n> -\tcase V4L2_PIX_FMT_VYUY:\n> -\t\treturn DRM_FORMAT_VYUY;\n> -\n> -\t/* YUY planar formats. */\n> -\tcase V4L2_PIX_FMT_NV16:\n> -\tcase V4L2_PIX_FMT_NV16M:\n> -\t\treturn DRM_FORMAT_NV16;\n> -\tcase V4L2_PIX_FMT_NV61:\n> -\tcase V4L2_PIX_FMT_NV61M:\n> -\t\treturn DRM_FORMAT_NV61;\n> -\tcase V4L2_PIX_FMT_NV12:\n> -\tcase V4L2_PIX_FMT_NV12M:\n> -\t\treturn DRM_FORMAT_NV12;\n> -\tcase V4L2_PIX_FMT_NV21:\n> -\tcase V4L2_PIX_FMT_NV21M:\n> -\t\treturn DRM_FORMAT_NV21;\n> -\n> -\t/* Compressed formats. */\n> -\tcase V4L2_PIX_FMT_MJPEG:\n> -\t\treturn DRM_FORMAT_MJPEG;\n> -\n> -\t/* V4L2 formats not yet supported by DRM. */\n> -\tcase V4L2_PIX_FMT_GREY:\n> -\tdefault:\n> -\t\t/*\n> -\t\t * \\todo We can't use LOG() in a static method of a Loggable\n> -\t\t * class. Until we fix the logger, work around it.\n> -\t\t */\n> -\t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> -\t\t\t\tLogError).stream()\n> -\t\t\t<< \"Unsupported V4L2 pixel format \"\n> -\t\t\t<< utils::hex(v4l2Fourcc);\n> -\t\treturn 0;\n> -\t}\n> +\treturn PixelFormat(v4l2Fourcc);\n>  }\n\nShouldn't we aim to replace all usages of this (and the following)\nmethods by centralizing the conversion in PixelFormat ? This just\nserved as an utility function for that purpose. Same for the V4L2\nwrapper.\n\nIf I recall correctly, the single plane/multiplane capabilities of the\nV4L2 device should be taken into account to perform the conversion.\nCould this be an additional paramter to\n                PixelFormat::toDRM4cc(uint32_t v4l24cc, bool molutplanar) ?\n\n>\n>  /**\n> @@ -1500,53 +1451,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n>   */\n>  uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n>  {\n> -\tswitch (pixelFormat) {\n> -\t/* RGB formats. */\n> -\tcase DRM_FORMAT_BGR888:\n> -\t\treturn V4L2_PIX_FMT_RGB24;\n> -\tcase DRM_FORMAT_RGB888:\n> -\t\treturn V4L2_PIX_FMT_BGR24;\n> -\tcase DRM_FORMAT_BGRA8888:\n> -\t\treturn V4L2_PIX_FMT_ARGB32;\n> -\n> -\t/* YUV packed formats. */\n> -\tcase DRM_FORMAT_YUYV:\n> -\t\treturn V4L2_PIX_FMT_YUYV;\n> -\tcase DRM_FORMAT_YVYU:\n> -\t\treturn V4L2_PIX_FMT_YVYU;\n> -\tcase DRM_FORMAT_UYVY:\n> -\t\treturn V4L2_PIX_FMT_UYVY;\n> -\tcase DRM_FORMAT_VYUY:\n> -\t\treturn V4L2_PIX_FMT_VYUY;\n> -\n> -\t/*\n> -\t * YUY planar formats.\n> -\t * \\todo Add support for non-contiguous memory planes\n> -\t * \\todo Select the format variant not only based on \\a multiplanar but\n> -\t * also take into account the formats supported by the device.\n> -\t */\n> -\tcase DRM_FORMAT_NV16:\n> -\t\treturn V4L2_PIX_FMT_NV16;\n> -\tcase DRM_FORMAT_NV61:\n> -\t\treturn V4L2_PIX_FMT_NV61;\n> -\tcase DRM_FORMAT_NV12:\n> -\t\treturn V4L2_PIX_FMT_NV12;\n> -\tcase DRM_FORMAT_NV21:\n> -\t\treturn V4L2_PIX_FMT_NV21;\n> -\n> -\t/* Compressed formats. */\n> -\tcase DRM_FORMAT_MJPEG:\n> -\t\treturn V4L2_PIX_FMT_MJPEG;\n> -\t}\n> -\n> -\t/*\n> -\t * \\todo We can't use LOG() in a static method of a Loggable\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> -\treturn 0;\n> +\treturn pixelFormat.v4l2();\n>  }\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/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> index 0ebb8edd49efd1b1..d86c97dab8374d5e 100644\n> --- a/src/qcam/viewfinder.cpp\n> +++ b/src/qcam/viewfinder.cpp\n> @@ -14,7 +14,7 @@\n>  #include \"viewfinder.h\"\n>\n>  ViewFinder::ViewFinder(QWidget *parent)\n> -\t: QWidget(parent), format_(0), width_(0), height_(0), image_(nullptr)\n> +\t: QWidget(parent), format_(), width_(0), height_(0), image_(nullptr)\n>  {\n>  }\n>\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 622520479be01f58..e9d7bfd8ee243b78 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -525,7 +525,6 @@ struct PixelFormatPlaneInfo {\n>  };\n>\n>  struct PixelFormatInfo {\n> -\tPixelFormat format;\n>  \tuint32_t v4l2Format;\n>  \tunsigned int numPlanes;\n>  \tstd::array<PixelFormatPlaneInfo, 3> planes;\n> @@ -533,24 +532,24 @@ 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> -\t{ DRM_FORMAT_BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n>  \t/* YUV packed formats. */\n> -\t{ DRM_FORMAT_UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n>  \t/* YUY planar formats. */\n> -\t{ DRM_FORMAT_NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> -}};\n> +\t{ V4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> +} };\n\nShouldn't we aim to centralize all of this in PixelFormat ?\n\nThanks\n   j\n\n>\n>  } /* namespace */\n>\n> @@ -588,24 +587,10 @@ unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n>\n>  PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n>  {\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.v4l2Format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn format;\n> -\n> -\treturn info->format;\n> +\treturn PixelFormat(format);\n>  }\n>\n>  uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)\n>  {\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\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> -\n> -\treturn info->v4l2Format;\n> +\treturn format.v4l2();\n>  }\n> diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> index a391f5cd087d3872..a904d681c1691889 100644\n> --- a/test/stream/stream_formats.cpp\n> +++ b/test/stream/stream_formats.cpp\n> @@ -7,6 +7,8 @@\n>\n>  #include <iostream>\n>\n> +#include <linux/drm_fourcc.h>\n> +\n>  #include <libcamera/geometry.h>\n>  #include <libcamera/stream.h>\n>\n> @@ -53,42 +55,49 @@ protected:\n>\n>  \tint run()\n>  \t{\n> +\t\tstd::vector<PixelFormat> pixelFormats = {\n> +\t\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_UYVY, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> +\t\t};\n> +\n>  \t\t/* Test discrete sizes */\n>  \t\tStreamFormats discrete({\n> -\t\t\t{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },\n> -\t\t\t{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },\n> +\t\t\t{ pixelFormats[0], { SizeRange(100, 100), SizeRange(200, 200) } },\n> +\t\t\t{ pixelFormats[1], { SizeRange(300, 300), SizeRange(400, 400) } },\n>  \t\t});\n>\n> -\t\tif (testSizes(\"discrete 1\", discrete.sizes(1),\n> +\t\tif (testSizes(\"discrete 1\", discrete.sizes(pixelFormats[0]),\n>  \t\t\t      { Size(100, 100), Size(200, 200) }))\n>  \t\t\treturn TestFail;\n> -\t\tif (testSizes(\"discrete 2\", discrete.sizes(2),\n> +\t\tif (testSizes(\"discrete 2\", discrete.sizes(pixelFormats[1]),\n>  \t\t\t      { Size(300, 300), Size(400, 400) }))\n>  \t\t\treturn TestFail;\n>\n>  \t\t/* Test range sizes */\n>  \t\tStreamFormats range({\n> -\t\t\t{ 1, { SizeRange(640, 480, 640, 480) } },\n> -\t\t\t{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },\n> -\t\t\t{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },\n> -\t\t\t{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> +\t\t\t{ pixelFormats[0], { SizeRange(640, 480, 640, 480) } },\n> +\t\t\t{ pixelFormats[1], { SizeRange(640, 480, 800, 600, 8, 8) } },\n> +\t\t\t{ pixelFormats[2], { SizeRange(640, 480, 800, 600, 16, 16) } },\n> +\t\t\t{ pixelFormats[3], { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n>  \t\t});\n>\n> -\t\tif (testSizes(\"range 1\", range.sizes(1), { Size(640, 480) }))\n> +\t\tif (testSizes(\"range 1\", range.sizes(pixelFormats[0]), { Size(640, 480) }))\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (testSizes(\"range 2\", range.sizes(2), {\n> +\t\tif (testSizes(\"range 2\", range.sizes(pixelFormats[1]), {\n>  \t\t\t      Size(640, 480), Size(720, 480),\n>  \t\t\t      Size(720, 576), Size(768, 480),\n>  \t\t\t      Size(800, 600) }))\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (testSizes(\"range 3\", range.sizes(3), {\n> +\t\tif (testSizes(\"range 3\", range.sizes(pixelFormats[2]), {\n>  \t\t\t      Size(640, 480), Size(720, 480),\n>  \t\t\t      Size(720, 576), Size(768, 480) }))\n>  \t\t\treturn TestFail;\n>\n> -\t\tif (testSizes(\"range 4\", range.sizes(4), {\n> +\t\tif (testSizes(\"range 4\", range.sizes(pixelFormats[3]), {\n>  \t\t\t      Size(1024, 768), Size(1280, 1024),\n>  \t\t\t      Size(2048, 1152), Size(2048, 1536),\n>  \t\t\t      Size(2560, 2048), Size(3200, 2048), }))\n> --\n> 2.25.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0493A6042A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Feb 2020 11:24:50 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 70CAAC0003;\n\tFri, 28 Feb 2020 10:24:49 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Fri, 28 Feb 2020 11:27:37 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200228102737.6w3dw2fcm2bcjmxu@uno.localdomain>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"mcxtqp2xmh3yqh3r\"","Content-Disposition":"inline","In-Reply-To":"<20200228032913.497826-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","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":"Fri, 28 Feb 2020 10:24:50 -0000"}},{"id":3851,"web_url":"https://patchwork.libcamera.org/comment/3851/","msgid":"<20200228113617.GA299107@oden.dyn.berto.se>","date":"2020-02-28T11:36:17","subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your feedback.\n\nOn 2020-02-28 11:27:37 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Fri, Feb 28, 2020 at 04:29:10AM +0100, Niklas Söderlund wrote:\n> > Create a class to represent the pixel formats. This is done to prepare\n> > to add support for modifiers for the formats. One bonus with this change\n> > it that it's allows it to make it impossible for other then designated\n> > classes (V4L2VideoDevice and V4L2CameraProxy) to create PixelFormat\n> > instances from a V4L2 fourcc limiting the risk of users of the class to\n> > mix the two fourcc namespaces unintentionally.\n> >\n> > The patch is unfortunately quiet large as it have to touch a lot of\n> > different ares of the code to simultaneously switch to the new class\n> > based PixelFormat implementation.\n> >\n> \n> I prasie the effort of making PixelFormat more descriptive and\n> centralize there the conversion between different 4cc sets, but I'm\n> not sure I like the way pipeline handlers can now construct\n> PixelFormat from both DRM and V4L2 fourcc. Please see below.\n\nNo they can't the constructor to create a PixelFormat from a V4L2 fourcc \nis protected and only allowed to be called from V4L2VideoDevice and \nV4L2CameraProxy. This I think is fair as they really are the two \nlocations where we have V4L2 fourc and need to create a PixelFormat from \nthem :-)\n\n> \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/pixelformats.h         |  40 +++++++-\n> >  src/cam/main.cpp                         |   4 +-\n> >  src/libcamera/formats.cpp                |   3 +\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |   6 +-\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  22 ++--\n> >  src/libcamera/pipeline/uvcvideo.cpp      |   4 +-\n> >  src/libcamera/pipeline/vimc.cpp          |  12 +--\n> >  src/libcamera/pixelformats.cpp           | 124 +++++++++++++++++++++++\n> >  src/libcamera/stream.cpp                 |   6 +-\n> >  src/libcamera/v4l2_videodevice.cpp       | 101 +-----------------\n> >  src/qcam/format_converter.cpp            |   2 +-\n> >  src/qcam/viewfinder.cpp                  |   2 +-\n> >  src/v4l2/v4l2_camera_proxy.cpp           |  49 ++++-----\n> >  test/stream/stream_formats.cpp           |  33 +++---\n> >  14 files changed, 236 insertions(+), 172 deletions(-)\n> >\n> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> > index 6e25b8d8b76e5961..f0951e983192d5e8 100644\n> > --- a/include/libcamera/pixelformats.h\n> > +++ b/include/libcamera/pixelformats.h\n> > @@ -7,11 +7,49 @@\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> > +/* Needs to be forward declared in the global namespace. */\n> > +class V4L2CameraProxy;\n> >\n> >  namespace libcamera {\n> >\n> > -using PixelFormat = uint32_t;\n> > +struct PixelFormatEntry;\n> > +\n> > +class PixelFormat\n> > +{\n> > +public:\n> > +\tPixelFormat();\n> > +\tPixelFormat(const PixelFormat &other);\n> > +\texplicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);\n> > +\n> > +\tPixelFormat &operator=(const PixelFormat &other);\n> > +\n> > +\tbool operator==(const PixelFormat &other) const;\n> > +\tbool operator!=(const PixelFormat &other) const;\n> > +\tbool operator<(const PixelFormat &other) const;\n> > +\n> > +\tuint32_t v4l2() const;\n> > +\tuint32_t fourcc() const;\n> > +\tconst std::set<uint32_t> &modifiers() const;\n> > +\n> > +protected:\n> > +\t/* Needed so V4L2VideoDevice can create PixelFormat from V4L2 fourcc. */\n> > +\tfriend class V4L2VideoDevice;\n> > +\n> > +\t/* Needed so V4L2CameraProxy can create PixelFormat from V4L2 fourcc. */\n> > +\tfriend V4L2CameraProxy;\n> > +\n> > +\texplicit PixelFormat(uint32_t v4l2_fourcc);\n> > +\n> > +private:\n> > +\tconst PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;\n> > +\tconst PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;\n> > +\n> > +\tconst PixelFormatEntry *format_;\n> > +};\n> >\n> >  } /* namespace libcamera */\n> >\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index ad4be55fc114fe22..c8ef79daea37d8b6 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -249,7 +249,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> > @@ -283,7 +283,7 @@ int CamApp::infoConfiguration()\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\t\t  << std::setw(8) << pixelformat.fourcc() << \" \"\n> >  \t\t\t\t  << formats.range(pixelformat).toString()\n> >  \t\t\t\t  << std::endl;\n> >\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 98817deee2b54c84..e3a89121e3c60151 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -9,6 +9,8 @@\n> >\n> >  #include <errno.h>\n> >\n> > +#include <libcamera/pixelformats.h>\n> > +\n> >  /**\n> >   * \\file formats.h\n> >   * \\brief Types and helper methods to handle libcamera image formats\n> > @@ -110,5 +112,6 @@ const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const\n> >  }\n> >\n> >  template class ImageFormats<unsigned int>;\n> > +template class ImageFormats<PixelFormat>;\n> >\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 33f97b340716abd0..71ac0ae33921f548 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -247,7 +247,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> >  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> >  {\n> >  \t/* The only pixel format the driver supports is NV12. */\n> > -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> \n> This is good. However you use the second parameter to differentiate\n> between constructing a PixelFormat from DRM or V4L2, which seems a bit\n> fragile and prevents you from defaulting the second parameter to an\n> empty modifier. What is the point of requesting to provide {} if no\n> modifier is associated with the format ?\n\nI agree it would be nice to find a way to make the modifiers argument \noptional. I'm toying with the idea of replacing the \nPixelFormat(v4l2_fourcc) with a protected factory function as it can \nonly be called from two classes to make it more explicit that it is \ndealing with special cases.\n\n> \n> >\n> >  \tif (scale) {\n> >  \t\t/*\n> > @@ -402,7 +402,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >  \t\tStreamConfiguration cfg = {};\n> >  \t\tIPU3Stream *stream = nullptr;\n> >\n> > -\t\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> >\n> >  \t\tswitch (role) {\n> >  \t\tcase StreamRole::StillCapture:\n> > @@ -1142,7 +1142,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> >  \t\treturn 0;\n> >\n> >  \tV4L2DeviceFormat outputFormat = {};\n> > -\toutputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);\n> > +\toutputFormat.fourcc = PixelFormat(DRM_FORMAT_NV12, {}).v4l2();\n> \n> Have you consider providing a static method to perform the DRM<->v4l2\n> fourcc conversion ? Here you create a PixelFormat instance just to\n> call .v4l2() on it. The only thing you care about is the translation\n> between the two 4cc codes, wouldn't this be better expressed as\n>         PixelFormat::toV4L2(DRM_FORMAT_NV12);\n\nI think this is a bad idea. What we want is a type to represent \nPixelFormat that (from most places) only can be constructed from DRM \nfourccs.\n\nI toyed with the possibility of makeing PixelFormat::v4l2() protected to \n\"hide\" it form all but the two V4L2 centric classes. This would be nice \nexpect then we would need DRM fourcc for all Meta formats which is \nprobably not what we want.\n\n> ?\n> \n> >  \toutputFormat.size = cfg.size;\n> >  \toutputFormat.planesCount = 2;\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 13433b216747cb8b..323fa3596c6ee242 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -433,14 +433,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> >\n> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  {\n> > -\tstatic const std::array<unsigned int, 8> formats{\n> > -\t\tDRM_FORMAT_YUYV,\n> > -\t\tDRM_FORMAT_YVYU,\n> > -\t\tDRM_FORMAT_VYUY,\n> > -\t\tDRM_FORMAT_NV16,\n> > -\t\tDRM_FORMAT_NV61,\n> > -\t\tDRM_FORMAT_NV21,\n> > -\t\tDRM_FORMAT_NV12,\n> > +\tstatic const std::array<PixelFormat, 8> formats{\n> > +\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_NV16, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_NV61, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_NV21, {}),\n> > +\t\tPixelFormat(DRM_FORMAT_NV12, {}),\n> >  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> >  \t};\n> >\n> > @@ -462,7 +462,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> >  \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> >  \t    formats.end()) {\n> \n> I am not sure what we get by making pipeline handler use the\n> PixelFormat class here just to enumerate and match 4cc codes.\n\ncfg.pixelFormat comes from the application and is a PixelFormat. It may \ncontains modifiers so we can not just comapre cfg.pixelFormat.fourcc() \nwith the V4L2 fourcc we need to compare the whole thing.\n\n> \n> >  \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > -\t\tcfg.pixelFormat = DRM_FORMAT_NV12,\n> > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> \n> Here instead is good. Application should be provided a PixelFormat\n> class\n> \n> >  \t\tstatus = Adjusted;\n> >  \t}\n> >\n> > @@ -541,7 +541,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> >  \t\treturn config;\n> >\n> >  \tStreamConfiguration cfg{};\n> > -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> >  \tcfg.size = data->sensor_->resolution();\n> >\n> >  \tconfig->addConfiguration(cfg);\n> > @@ -796,7 +796,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 8efd188e75a3d135..5f3e52f691aaeae4 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -115,8 +115,8 @@ 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 \" << pixelFormat.fourcc()\n> > +\t\t\t<< \" to \" << cfg.pixelFormat.fourcc();\n> >  \t\tstatus = Adjusted;\n> >  \t}\n> >\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 9fbe33c626e327d4..a591c424919b0783 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -106,10 +106,10 @@ private:\n> >\n> >  namespace {\n> >\n> > -constexpr std::array<unsigned int, 3> pixelformats{\n> > -\tDRM_FORMAT_RGB888,\n> > -\tDRM_FORMAT_BGR888,\n> > -\tDRM_FORMAT_BGRA8888,\n> > +static const std::array<PixelFormat, 3> pixelformats{\n> > +\tPixelFormat(DRM_FORMAT_RGB888, {}),\n> > +\tPixelFormat(DRM_FORMAT_BGR888, {}),\n> > +\tPixelFormat(DRM_FORMAT_BGRA8888, {}),\n> >  };\n> >\n> >  } /* namespace */\n> > @@ -138,7 +138,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> >  \tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> >  \t    pixelformats.end()) {\n> >  \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> > -\t\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n> >  \t\tstatus = Adjusted;\n> >  \t}\n> >\n> > @@ -187,7 +187,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >\n> >  \tStreamConfiguration cfg(formats.data());\n> >\n> > -\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n> >  \tcfg.size = { 1920, 1080 };\n> >  \tcfg.bufferCount = 4;\n> >\n> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> > index c03335400b709d9b..b2aacbc39b9ca16a 100644\n> > --- a/src/libcamera/pixelformats.cpp\n> > +++ b/src/libcamera/pixelformats.cpp\n> > @@ -7,6 +7,13 @@\n> >\n> >  #include <libcamera/pixelformats.h>\n> >\n> > +#include <vector>\n> > +\n> > +#include <linux/drm_fourcc.h>\n> > +#include <linux/videodev2.h>\n> > +\n> > +#include \"log.h\"\n> > +\n> >  /**\n> >   * \\file pixelformats.h\n> >   * \\brief libcamera pixel formats\n> > @@ -14,6 +21,8 @@\n> >\n> >  namespace libcamera {\n> >\n> > +LOG_DEFINE_CATEGORY(PixelFormats)\n> > +\n> >  /**\n> >   * \\typedef PixelFormat\n> >   * \\brief libcamera image pixel format\n> > @@ -25,4 +34,119 @@ namespace libcamera {\n> >   * \\todo Add support for format modifiers\n> >   */\n> >\n> > +struct PixelFormatEntry {\n> > +\tuint32_t v4l2;\n> > +\tuint32_t drm;\n> > +\tstd::set<uint32_t> modifiers;\n> > +};\n> > +\n> > +static const std::vector<PixelFormatEntry> pixelFormats = {\n> > +\t/* Invalid format, important to be first in list. */\n> > +\t{ .v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n> > +\t/* RGB formats. */\n> > +\t{ .v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n> > +\t/* YUV packed formats. */\n> > +\t{ .v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n> > +\t/* YUY planar formats. */\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> > +\t{ .v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> > +\t/* Compressed formats. */\n> > +\t{ .v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n> > +};\n> > +\n> > +PixelFormat::PixelFormat()\n> > +\t: format_(&pixelFormats[0])\n> > +{\n> > +}\n> > +\n> > +PixelFormat::PixelFormat(const PixelFormat &other)\n> > +\t: format_(other.format_)\n> > +{\n> > +}\n> > +\n> > +PixelFormat::PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers)\n> > +\t: format_(fromDRM(drm_fourcc, drm_modifiers))\n> > +{\n> > +}\n> > +\n> > +PixelFormat::PixelFormat(uint32_t v4l2_fourcc)\n> > +\t: format_(fromV4L2(v4l2_fourcc))\n> > +{\n> > +}\n> \n> This seems very fragile. The distinction between constructing a\n> PixelFormat with a DRM 4cc or a V4L2 fourcc (which are both uint32_t)\n> only depends on the presence of the second, right now empty,\n> parameter.\n> \n> What I would do is instead make it mandatory to construct a\n> PixelFormat with a valid DRM 4cc, and provide to pipeline handler a\n> static method(s) to convert between drm/v4l2 whenever they consider it\n> oppotune.\n\nIt is already mandatory to create it from a DRM fourcc.\n\nI do not like the static methods that allow converting between drm/v4l2 \nas the conversion is not as simple fourcc to fourcc it's also modifiers.  \nBy not having them we have one way to do things. The static methods of \nthe past IMHO have contributed to the mess we have today where one is \nnot sure what namespace the fourcc is in as it's converted back and \nforth at different parts of the code.\n\n> \n> > +\n> > +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> > +{\n> > +\tformat_ = other.format_;\n> > +\n> > +\treturn *this;\n> > +}\n> > +\n> > +bool PixelFormat::operator==(const PixelFormat &other) const\n> > +{\n> > +\treturn format_ == other.format_;\n> > +}\n> > +\n> > +bool PixelFormat::operator!=(const PixelFormat &other) const\n> > +{\n> > +\treturn format_ != other.format_;\n> > +}\n> > +\n> > +bool PixelFormat::operator<(const PixelFormat &other) const\n> > +{\n> > +\treturn format_ > other.format_;\n> > +}\n> > +\n> > +uint32_t PixelFormat::v4l2() const\n> > +{\n> > +\treturn format_->v4l2;\n> > +}\n> > +\n> > +uint32_t PixelFormat::fourcc() const\n> > +{\n> > +\treturn format_->drm;\n> > +}\n> > +\n> > +const std::set<uint32_t> &PixelFormat::modifiers() const\n> > +{\n> > +\treturn format_->modifiers;\n> > +}\n> > +\n> > +const PixelFormatEntry *PixelFormat::fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const\n> > +{\n> > +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> > +\t\tif (entry.drm == drm_fourcc &&\n> > +\t\t    entry.modifiers == drm_modifiers)\n> > +\t\t\treturn &entry;\n> > +\n> > +\tLOG(PixelFormats, Error)\n> > +\t\t<< \"Unsupported DRM pixel format \"\n> > +\t\t<< utils::hex(drm_fourcc);\n> > +\n> > +\treturn &pixelFormats[0];\n> > +}\n> > +\n> > +const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const\n> > +{\n> > +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> > +\t\tif (entry.v4l2 == v4l2_fourcc)\n> > +\t\t\treturn &entry;\n> > +\n> > +\tLOG(PixelFormats, Error)\n> > +\t\t<< \"Unsupported V4L2 pixel format \"\n> > +\t\t<< utils::hex(v4l2_fourcc);\n> > +\n> > +\treturn &pixelFormats[0];\n> > +}\n> > +\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > index 13789e9eb344f95c..dbce550ca8d0b7b1 100644\n> > --- a/src/libcamera/stream.cpp\n> > +++ b/src/libcamera/stream.cpp\n> > @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(PixelFormat pixelformat) const\n> >   * handlers provied StreamFormats.\n> >   */\n> >  StreamConfiguration::StreamConfiguration()\n> > -\t: pixelFormat(0), stream_(nullptr)\n> > +\t: pixelFormat(), stream_(nullptr)\n> >  {\n> >  }\n> >\n> > @@ -287,7 +287,7 @@ StreamConfiguration::StreamConfiguration()\n> >   * \\brief Construct a configuration with stream formats\n> >   */\n> >  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> > -\t: pixelFormat(0), stream_(nullptr), formats_(formats)\n> > +\t: pixelFormat(), stream_(nullptr), formats_(formats)\n> >  {\n> >  }\n> >\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() << \"-\" << utils::hex(pixelFormat.fourcc());\n> >  \treturn ss.str();\n> >  }\n> >\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index f84bd00570afa38c..e9d3e60198e140a0 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -846,7 +846,7 @@ ImageFormats<PixelFormat> V4L2VideoDevice::formats()\n> >  \t\tif (sizes.empty())\n> >  \t\t\treturn {};\n> >\n> > -\t\tif (formats.addFormat(pixelformat, sizes)) {\n> > +\t\tif (formats.addFormat(PixelFormat(pixelformat), sizes)) {\n> >  \t\t\tLOG(V4L2, Error)\n> >  \t\t\t\t<< \"Could not add sizes for pixel format \"\n> >  \t\t\t\t<< pixelformat;\n> > @@ -1417,56 +1417,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> >   */\n> >  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> >  {\n> > -\tswitch (v4l2Fourcc) {\n> > -\t/* RGB formats. */\n> > -\tcase V4L2_PIX_FMT_RGB24:\n> > -\t\treturn DRM_FORMAT_BGR888;\n> > -\tcase V4L2_PIX_FMT_BGR24:\n> > -\t\treturn DRM_FORMAT_RGB888;\n> > -\tcase V4L2_PIX_FMT_ARGB32:\n> > -\t\treturn DRM_FORMAT_BGRA8888;\n> > -\n> > -\t/* YUV packed formats. */\n> > -\tcase V4L2_PIX_FMT_YUYV:\n> > -\t\treturn DRM_FORMAT_YUYV;\n> > -\tcase V4L2_PIX_FMT_YVYU:\n> > -\t\treturn DRM_FORMAT_YVYU;\n> > -\tcase V4L2_PIX_FMT_UYVY:\n> > -\t\treturn DRM_FORMAT_UYVY;\n> > -\tcase V4L2_PIX_FMT_VYUY:\n> > -\t\treturn DRM_FORMAT_VYUY;\n> > -\n> > -\t/* YUY planar formats. */\n> > -\tcase V4L2_PIX_FMT_NV16:\n> > -\tcase V4L2_PIX_FMT_NV16M:\n> > -\t\treturn DRM_FORMAT_NV16;\n> > -\tcase V4L2_PIX_FMT_NV61:\n> > -\tcase V4L2_PIX_FMT_NV61M:\n> > -\t\treturn DRM_FORMAT_NV61;\n> > -\tcase V4L2_PIX_FMT_NV12:\n> > -\tcase V4L2_PIX_FMT_NV12M:\n> > -\t\treturn DRM_FORMAT_NV12;\n> > -\tcase V4L2_PIX_FMT_NV21:\n> > -\tcase V4L2_PIX_FMT_NV21M:\n> > -\t\treturn DRM_FORMAT_NV21;\n> > -\n> > -\t/* Compressed formats. */\n> > -\tcase V4L2_PIX_FMT_MJPEG:\n> > -\t\treturn DRM_FORMAT_MJPEG;\n> > -\n> > -\t/* V4L2 formats not yet supported by DRM. */\n> > -\tcase V4L2_PIX_FMT_GREY:\n> > -\tdefault:\n> > -\t\t/*\n> > -\t\t * \\todo We can't use LOG() in a static method of a Loggable\n> > -\t\t * class. Until we fix the logger, work around it.\n> > -\t\t */\n> > -\t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> > -\t\t\t\tLogError).stream()\n> > -\t\t\t<< \"Unsupported V4L2 pixel format \"\n> > -\t\t\t<< utils::hex(v4l2Fourcc);\n> > -\t\treturn 0;\n> > -\t}\n> > +\treturn PixelFormat(v4l2Fourcc);\n> >  }\n> \n> Shouldn't we aim to replace all usages of this (and the following)\n> methods by centralizing the conversion in PixelFormat ? This just\n> served as an utility function for that purpose. Same for the V4L2\n> wrapper.\n\nPlease see patch 5/6 and 6/6.\n\n> \n> If I recall correctly, the single plane/multiplane capabilities of the\n> V4L2 device should be taken into account to perform the conversion.\n> Could this be an additional paramter to\n>                 PixelFormat::toDRM4cc(uint32_t v4l24cc, bool molutplanar) ?\n\nIt could but we have no consumer of such information today and the goal \nof this series is not to add it but to allow for format enumeration with \nmodifiers to be built on top. When we need multiplane information to \nconvert from V4L2 fourcc we can add it on top.\n\n> \n> >\n> >  /**\n> > @@ -1500,53 +1451,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> >   */\n> >  uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> >  {\n> > -\tswitch (pixelFormat) {\n> > -\t/* RGB formats. */\n> > -\tcase DRM_FORMAT_BGR888:\n> > -\t\treturn V4L2_PIX_FMT_RGB24;\n> > -\tcase DRM_FORMAT_RGB888:\n> > -\t\treturn V4L2_PIX_FMT_BGR24;\n> > -\tcase DRM_FORMAT_BGRA8888:\n> > -\t\treturn V4L2_PIX_FMT_ARGB32;\n> > -\n> > -\t/* YUV packed formats. */\n> > -\tcase DRM_FORMAT_YUYV:\n> > -\t\treturn V4L2_PIX_FMT_YUYV;\n> > -\tcase DRM_FORMAT_YVYU:\n> > -\t\treturn V4L2_PIX_FMT_YVYU;\n> > -\tcase DRM_FORMAT_UYVY:\n> > -\t\treturn V4L2_PIX_FMT_UYVY;\n> > -\tcase DRM_FORMAT_VYUY:\n> > -\t\treturn V4L2_PIX_FMT_VYUY;\n> > -\n> > -\t/*\n> > -\t * YUY planar formats.\n> > -\t * \\todo Add support for non-contiguous memory planes\n> > -\t * \\todo Select the format variant not only based on \\a multiplanar but\n> > -\t * also take into account the formats supported by the device.\n> > -\t */\n> > -\tcase DRM_FORMAT_NV16:\n> > -\t\treturn V4L2_PIX_FMT_NV16;\n> > -\tcase DRM_FORMAT_NV61:\n> > -\t\treturn V4L2_PIX_FMT_NV61;\n> > -\tcase DRM_FORMAT_NV12:\n> > -\t\treturn V4L2_PIX_FMT_NV12;\n> > -\tcase DRM_FORMAT_NV21:\n> > -\t\treturn V4L2_PIX_FMT_NV21;\n> > -\n> > -\t/* Compressed formats. */\n> > -\tcase DRM_FORMAT_MJPEG:\n> > -\t\treturn V4L2_PIX_FMT_MJPEG;\n> > -\t}\n> > -\n> > -\t/*\n> > -\t * \\todo We can't use LOG() in a static method of a Loggable\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> > -\treturn 0;\n> > +\treturn pixelFormat.v4l2();\n> >  }\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/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> > index 0ebb8edd49efd1b1..d86c97dab8374d5e 100644\n> > --- a/src/qcam/viewfinder.cpp\n> > +++ b/src/qcam/viewfinder.cpp\n> > @@ -14,7 +14,7 @@\n> >  #include \"viewfinder.h\"\n> >\n> >  ViewFinder::ViewFinder(QWidget *parent)\n> > -\t: QWidget(parent), format_(0), width_(0), height_(0), image_(nullptr)\n> > +\t: QWidget(parent), format_(), width_(0), height_(0), image_(nullptr)\n> >  {\n> >  }\n> >\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index 622520479be01f58..e9d7bfd8ee243b78 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -525,7 +525,6 @@ struct PixelFormatPlaneInfo {\n> >  };\n> >\n> >  struct PixelFormatInfo {\n> > -\tPixelFormat format;\n> >  \tuint32_t v4l2Format;\n> >  \tunsigned int numPlanes;\n> >  \tstd::array<PixelFormatPlaneInfo, 3> planes;\n> > @@ -533,24 +532,24 @@ 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> > -\t{ DRM_FORMAT_BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> >  \t/* YUV packed formats. */\n> > -\t{ DRM_FORMAT_UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> >  \t/* YUY planar formats. */\n> > -\t{ DRM_FORMAT_NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > -\t{ DRM_FORMAT_NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > -}};\n> > +\t{ V4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ V4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > +} };\n> \n> Shouldn't we aim to centralize all of this in PixelFormat ?\n\nI thought about it and then decided against it, the information recorded \nhere is specific to the v4l2 wrapper and can be indexed directly on V4L2 \nfourcc so I think we should leave it where it is.\n\n> \n> Thanks\n>    j\n> \n> >\n> >  } /* namespace */\n> >\n> > @@ -588,24 +587,10 @@ unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n> >\n> >  PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n> >  {\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > -\t\t\t\t\t return info.v4l2Format == format;\n> > -\t\t\t\t });\n> > -\tif (info == pixelFormatInfo.end())\n> > -\t\treturn format;\n> > -\n> > -\treturn info->format;\n> > +\treturn PixelFormat(format);\n> >  }\n> >\n> >  uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)\n> >  {\n> > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > -\t\t\t\t [format](const PixelFormatInfo &info) {\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> > -\n> > -\treturn info->v4l2Format;\n> > +\treturn format.v4l2();\n> >  }\n> > diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> > index a391f5cd087d3872..a904d681c1691889 100644\n> > --- a/test/stream/stream_formats.cpp\n> > +++ b/test/stream/stream_formats.cpp\n> > @@ -7,6 +7,8 @@\n> >\n> >  #include <iostream>\n> >\n> > +#include <linux/drm_fourcc.h>\n> > +\n> >  #include <libcamera/geometry.h>\n> >  #include <libcamera/stream.h>\n> >\n> > @@ -53,42 +55,49 @@ protected:\n> >\n> >  \tint run()\n> >  \t{\n> > +\t\tstd::vector<PixelFormat> pixelFormats = {\n> > +\t\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> > +\t\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> > +\t\t\tPixelFormat(DRM_FORMAT_UYVY, {}),\n> > +\t\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> > +\t\t};\n> > +\n> >  \t\t/* Test discrete sizes */\n> >  \t\tStreamFormats discrete({\n> > -\t\t\t{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },\n> > -\t\t\t{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },\n> > +\t\t\t{ pixelFormats[0], { SizeRange(100, 100), SizeRange(200, 200) } },\n> > +\t\t\t{ pixelFormats[1], { SizeRange(300, 300), SizeRange(400, 400) } },\n> >  \t\t});\n> >\n> > -\t\tif (testSizes(\"discrete 1\", discrete.sizes(1),\n> > +\t\tif (testSizes(\"discrete 1\", discrete.sizes(pixelFormats[0]),\n> >  \t\t\t      { Size(100, 100), Size(200, 200) }))\n> >  \t\t\treturn TestFail;\n> > -\t\tif (testSizes(\"discrete 2\", discrete.sizes(2),\n> > +\t\tif (testSizes(\"discrete 2\", discrete.sizes(pixelFormats[1]),\n> >  \t\t\t      { Size(300, 300), Size(400, 400) }))\n> >  \t\t\treturn TestFail;\n> >\n> >  \t\t/* Test range sizes */\n> >  \t\tStreamFormats range({\n> > -\t\t\t{ 1, { SizeRange(640, 480, 640, 480) } },\n> > -\t\t\t{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },\n> > -\t\t\t{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },\n> > -\t\t\t{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> > +\t\t\t{ pixelFormats[0], { SizeRange(640, 480, 640, 480) } },\n> > +\t\t\t{ pixelFormats[1], { SizeRange(640, 480, 800, 600, 8, 8) } },\n> > +\t\t\t{ pixelFormats[2], { SizeRange(640, 480, 800, 600, 16, 16) } },\n> > +\t\t\t{ pixelFormats[3], { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> >  \t\t});\n> >\n> > -\t\tif (testSizes(\"range 1\", range.sizes(1), { Size(640, 480) }))\n> > +\t\tif (testSizes(\"range 1\", range.sizes(pixelFormats[0]), { Size(640, 480) }))\n> >  \t\t\treturn TestFail;\n> >\n> > -\t\tif (testSizes(\"range 2\", range.sizes(2), {\n> > +\t\tif (testSizes(\"range 2\", range.sizes(pixelFormats[1]), {\n> >  \t\t\t      Size(640, 480), Size(720, 480),\n> >  \t\t\t      Size(720, 576), Size(768, 480),\n> >  \t\t\t      Size(800, 600) }))\n> >  \t\t\treturn TestFail;\n> >\n> > -\t\tif (testSizes(\"range 3\", range.sizes(3), {\n> > +\t\tif (testSizes(\"range 3\", range.sizes(pixelFormats[2]), {\n> >  \t\t\t      Size(640, 480), Size(720, 480),\n> >  \t\t\t      Size(720, 576), Size(768, 480) }))\n> >  \t\t\treturn TestFail;\n> >\n> > -\t\tif (testSizes(\"range 4\", range.sizes(4), {\n> > +\t\tif (testSizes(\"range 4\", range.sizes(pixelFormats[3]), {\n> >  \t\t\t      Size(1024, 768), Size(1280, 1024),\n> >  \t\t\t      Size(2048, 1152), Size(2048, 1536),\n> >  \t\t\t      Size(2560, 2048), Size(3200, 2048), }))\n> > --\n> > 2.25.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5583A6268F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Feb 2020 12:36:21 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id x7so2985503ljc.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Feb 2020 03:36:21 -0800 (PST)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tb17sm4702823lff.79.2020.02.28.03.36.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 28 Feb 2020 03:36:19 -0800 (PST)"],"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=sk8d4x9XCftpJVd3gajwY9cy+06GWwb/GpmZe01Os4c=;\n\tb=McD4c85l3rGfiB4jS0EYmv6vxd/K4ZnsfsmeeRQCBR7CzPsRSqHzUA4+QAJZOn1WjV\n\tpLnlXPC6h8nS2EU1mu1cWVIsV4MArXhfS+JJcmIAJIxRqpogIBE+Nm3ozkJ1SHdinGU0\n\tSqjXfMk00cJcMB1qZv2YdYcb7WzQVwO8nqwWT0KRu51UQYEe/4vnSF0IS43gorBI41H+\n\tMPUpPFfouQD2gwlgifJUlRcmvINIdoq7EtBaaMYrw1qfuX2CG5t/jX1RFrcb1YbFF1cr\n\t/p3Y18k7qLg5hVu0A+Ehxoqz87P+in+zfI9MclQf0I9Xba6ZXs00TGFnPJYVx8ZCtTxT\n\tQ0Lw==","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=sk8d4x9XCftpJVd3gajwY9cy+06GWwb/GpmZe01Os4c=;\n\tb=Ma2uSLfBxV9BkUTmDIBkG1PZLvgfo31uVeBLy/MbgDcHRrnxMXk/qb/GOZKNgt8weo\n\tQhEclV4Hn8E8fsZ1wJr/wJxULEG8hoVe6qk71aoCs4l+/wZsnRnC7h9TS1vWE1UFKcWk\n\t1wV9bmB5gwObYnMoUXBN1Ywol/hdin4167Y0oC0s+6hIqnT2WM3nhwQ8lOfU3Ra9B6nI\n\twGDIMw5qoJ+Wijq3lTMKd/e6mCAPC5qDzuJ9sawpSGxh8laSU0Ef/QhN/mpIWQgZEwEi\n\tkS6sbR51A4/ZjclJ7Xmz2ZC7gPGKPElj99mw5JP2NKAZ6r6O6Mkf6jJEl5D6j31aW0kI\n\tXzxA==","X-Gm-Message-State":"ANhLgQ2bXh8avyGOxExudeA2NOZQZgB3ycF0zbw8hms4X0c9UNN/EHyR\n\tEjbsUM5UtrW8Y5eTQFLG2Hykr0migT4=","X-Google-Smtp-Source":"ADFU+vv4jeC2Ob7G/nTx4BjNuqmmtOoYRACO7UaxSUVtgSPynPiXbrsXbDt+pqELDtQcbw7EKurtHQ==","X-Received":"by 2002:a2e:9a93:: with SMTP id\n\tp19mr2683522lji.177.1582889779936; \n\tFri, 28 Feb 2020 03:36:19 -0800 (PST)","Date":"Fri, 28 Feb 2020 12:36:17 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200228113617.GA299107@oden.dyn.berto.se>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-4-niklas.soderlund@ragnatech.se>\n\t<20200228102737.6w3dw2fcm2bcjmxu@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228102737.6w3dw2fcm2bcjmxu@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","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":"Fri, 28 Feb 2020 11:36:21 -0000"}},{"id":3853,"web_url":"https://patchwork.libcamera.org/comment/3853/","msgid":"<20200228171958.GC299107@oden.dyn.berto.se>","date":"2020-02-28T17:19:58","subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"On 2020-02-28 04:29:10 +0100, Niklas Söderlund wrote:\n> Create a class to represent the pixel formats. This is done to prepare\n> to add support for modifiers for the formats. One bonus with this change\n> it that it's allows it to make it impossible for other then designated\n> classes (V4L2VideoDevice and V4L2CameraProxy) to create PixelFormat\n> instances from a V4L2 fourcc limiting the risk of users of the class to\n> mix the two fourcc namespaces unintentionally.\n> \n> The patch is unfortunately quiet large as it have to touch a lot of\n> different ares of the code to simultaneously switch to the new class\n> based PixelFormat implementation.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/pixelformats.h         |  40 +++++++-\n>  src/cam/main.cpp                         |   4 +-\n>  src/libcamera/formats.cpp                |   3 +\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |   6 +-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  22 ++--\n>  src/libcamera/pipeline/uvcvideo.cpp      |   4 +-\n>  src/libcamera/pipeline/vimc.cpp          |  12 +--\n>  src/libcamera/pixelformats.cpp           | 124 +++++++++++++++++++++++\n>  src/libcamera/stream.cpp                 |   6 +-\n>  src/libcamera/v4l2_videodevice.cpp       | 101 +-----------------\n>  src/qcam/format_converter.cpp            |   2 +-\n>  src/qcam/viewfinder.cpp                  |   2 +-\n>  src/v4l2/v4l2_camera_proxy.cpp           |  49 ++++-----\n>  test/stream/stream_formats.cpp           |  33 +++---\n>  14 files changed, 236 insertions(+), 172 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 6e25b8d8b76e5961..f0951e983192d5e8 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -7,11 +7,49 @@\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> +/* Needs to be forward declared in the global namespace. */\n> +class V4L2CameraProxy;\n>  \n>  namespace libcamera {\n>  \n> -using PixelFormat = uint32_t;\n> +struct PixelFormatEntry;\n> +\n> +class PixelFormat\n> +{\n> +public:\n> +\tPixelFormat();\n> +\tPixelFormat(const PixelFormat &other);\n> +\texplicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);\n> +\n> +\tPixelFormat &operator=(const PixelFormat &other);\n> +\n> +\tbool operator==(const PixelFormat &other) const;\n> +\tbool operator!=(const PixelFormat &other) const;\n> +\tbool operator<(const PixelFormat &other) const;\n> +\n> +\tuint32_t v4l2() const;\n> +\tuint32_t fourcc() const;\n> +\tconst std::set<uint32_t> &modifiers() const;\n> +\n> +protected:\n> +\t/* Needed so V4L2VideoDevice can create PixelFormat from V4L2 fourcc. */\n> +\tfriend class V4L2VideoDevice;\n> +\n> +\t/* Needed so V4L2CameraProxy can create PixelFormat from V4L2 fourcc. */\n> +\tfriend V4L2CameraProxy;\n> +\n> +\texplicit PixelFormat(uint32_t v4l2_fourcc);\n> +\n> +private:\n> +\tconst PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;\n> +\tconst PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;\n> +\n> +\tconst PixelFormatEntry *format_;\n> +};\n>  \n>  } /* namespace libcamera */\n>  \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index ad4be55fc114fe22..c8ef79daea37d8b6 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -249,7 +249,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> @@ -283,7 +283,7 @@ int CamApp::infoConfiguration()\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\t\t  << std::setw(8) << pixelformat.fourcc() << \" \"\n>  \t\t\t\t  << formats.range(pixelformat).toString()\n>  \t\t\t\t  << std::endl;\n>  \n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 98817deee2b54c84..e3a89121e3c60151 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -9,6 +9,8 @@\n>  \n>  #include <errno.h>\n>  \n> +#include <libcamera/pixelformats.h>\n> +\n>  /**\n>   * \\file formats.h\n>   * \\brief Types and helper methods to handle libcamera image formats\n> @@ -110,5 +112,6 @@ const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const\n>  }\n>  \n>  template class ImageFormats<unsigned int>;\n> +template class ImageFormats<PixelFormat>;\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 33f97b340716abd0..71ac0ae33921f548 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -247,7 +247,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n>  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n>  {\n>  \t/* The only pixel format the driver supports is NV12. */\n> -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>  \n>  \tif (scale) {\n>  \t\t/*\n> @@ -402,7 +402,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\tStreamConfiguration cfg = {};\n>  \t\tIPU3Stream *stream = nullptr;\n>  \n> -\t\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>  \n>  \t\tswitch (role) {\n>  \t\tcase StreamRole::StillCapture:\n> @@ -1142,7 +1142,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n>  \t\treturn 0;\n>  \n>  \tV4L2DeviceFormat outputFormat = {};\n> -\toutputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);\n> +\toutputFormat.fourcc = PixelFormat(DRM_FORMAT_NV12, {}).v4l2();\n>  \toutputFormat.size = cfg.size;\n>  \toutputFormat.planesCount = 2;\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 13433b216747cb8b..323fa3596c6ee242 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -433,14 +433,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n>  \n>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  {\n> -\tstatic const std::array<unsigned int, 8> formats{\n> -\t\tDRM_FORMAT_YUYV,\n> -\t\tDRM_FORMAT_YVYU,\n> -\t\tDRM_FORMAT_VYUY,\n> -\t\tDRM_FORMAT_NV16,\n> -\t\tDRM_FORMAT_NV61,\n> -\t\tDRM_FORMAT_NV21,\n> -\t\tDRM_FORMAT_NV12,\n> +\tstatic const std::array<PixelFormat, 8> formats{\n> +\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> +\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> +\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV16, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV61, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV21, {}),\n> +\t\tPixelFormat(DRM_FORMAT_NV12, {}),\n>  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n>  \t};\n>  \n> @@ -462,7 +462,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n>  \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n>  \t    formats.end()) {\n>  \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> -\t\tcfg.pixelFormat = DRM_FORMAT_NV12,\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> @@ -541,7 +541,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n>  \t\treturn config;\n>  \n>  \tStreamConfiguration cfg{};\n> -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n>  \tcfg.size = data->sensor_->resolution();\n>  \n>  \tconfig->addConfiguration(cfg);\n> @@ -796,7 +796,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 8efd188e75a3d135..5f3e52f691aaeae4 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -115,8 +115,8 @@ 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 \" << pixelFormat.fourcc()\n> +\t\t\t<< \" to \" << cfg.pixelFormat.fourcc();\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 9fbe33c626e327d4..a591c424919b0783 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -106,10 +106,10 @@ private:\n>  \n>  namespace {\n>  \n> -constexpr std::array<unsigned int, 3> pixelformats{\n> -\tDRM_FORMAT_RGB888,\n> -\tDRM_FORMAT_BGR888,\n> -\tDRM_FORMAT_BGRA8888,\n> +static const std::array<PixelFormat, 3> pixelformats{\n> +\tPixelFormat(DRM_FORMAT_RGB888, {}),\n> +\tPixelFormat(DRM_FORMAT_BGR888, {}),\n> +\tPixelFormat(DRM_FORMAT_BGRA8888, {}),\n>  };\n>  \n>  } /* namespace */\n> @@ -138,7 +138,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n>  \tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n>  \t    pixelformats.end()) {\n>  \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> -\t\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n>  \t\tstatus = Adjusted;\n>  \t}\n>  \n> @@ -187,7 +187,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>  \n>  \tStreamConfiguration cfg(formats.data());\n>  \n> -\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n>  \tcfg.size = { 1920, 1080 };\n>  \tcfg.bufferCount = 4;\n>  \n> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> index c03335400b709d9b..b2aacbc39b9ca16a 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -7,6 +7,13 @@\n>  \n>  #include <libcamera/pixelformats.h>\n>  \n> +#include <vector>\n> +\n> +#include <linux/drm_fourcc.h>\n> +#include <linux/videodev2.h>\n> +\n> +#include \"log.h\"\n> +\n>  /**\n>   * \\file pixelformats.h\n>   * \\brief libcamera pixel formats\n> @@ -14,6 +21,8 @@\n>  \n>  namespace libcamera {\n>  \n> +LOG_DEFINE_CATEGORY(PixelFormats)\n> +\n>  /**\n>   * \\typedef PixelFormat\n>   * \\brief libcamera image pixel format\n> @@ -25,4 +34,119 @@ namespace libcamera {\n>   * \\todo Add support for format modifiers\n>   */\n>  \n> +struct PixelFormatEntry {\n> +\tuint32_t v4l2;\n> +\tuint32_t drm;\n> +\tstd::set<uint32_t> modifiers;\n\nThis should be an uint64_t as modifiers are long unsigned int. This will \nalso effect the methods in PixelFormat that deals with modifiers.\n\n> +};\n> +\n> +static const std::vector<PixelFormatEntry> pixelFormats = {\n> +\t/* Invalid format, important to be first in list. */\n> +\t{ .v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n> +\t/* RGB formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n> +\t/* YUV packed formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n> +\t/* YUY planar formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t{ .v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> +\t/* Compressed formats. */\n> +\t{ .v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n> +};\n> +\n> +PixelFormat::PixelFormat()\n> +\t: format_(&pixelFormats[0])\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(const PixelFormat &other)\n> +\t: format_(other.format_)\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers)\n> +\t: format_(fromDRM(drm_fourcc, drm_modifiers))\n> +{\n> +}\n> +\n> +PixelFormat::PixelFormat(uint32_t v4l2_fourcc)\n> +\t: format_(fromV4L2(v4l2_fourcc))\n> +{\n> +}\n> +\n> +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> +{\n> +\tformat_ = other.format_;\n> +\n> +\treturn *this;\n> +}\n> +\n> +bool PixelFormat::operator==(const PixelFormat &other) const\n> +{\n> +\treturn format_ == other.format_;\n> +}\n> +\n> +bool PixelFormat::operator!=(const PixelFormat &other) const\n> +{\n> +\treturn format_ != other.format_;\n> +}\n> +\n> +bool PixelFormat::operator<(const PixelFormat &other) const\n> +{\n> +\treturn format_ > other.format_;\n> +}\n> +\n> +uint32_t PixelFormat::v4l2() const\n> +{\n> +\treturn format_->v4l2;\n> +}\n> +\n> +uint32_t PixelFormat::fourcc() const\n> +{\n> +\treturn format_->drm;\n> +}\n> +\n> +const std::set<uint32_t> &PixelFormat::modifiers() const\n> +{\n> +\treturn format_->modifiers;\n> +}\n> +\n> +const PixelFormatEntry *PixelFormat::fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const\n> +{\n> +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> +\t\tif (entry.drm == drm_fourcc &&\n> +\t\t    entry.modifiers == drm_modifiers)\n> +\t\t\treturn &entry;\n> +\n> +\tLOG(PixelFormats, Error)\n> +\t\t<< \"Unsupported DRM pixel format \"\n> +\t\t<< utils::hex(drm_fourcc);\n> +\n> +\treturn &pixelFormats[0];\n> +}\n> +\n> +const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const\n> +{\n> +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> +\t\tif (entry.v4l2 == v4l2_fourcc)\n> +\t\t\treturn &entry;\n> +\n> +\tLOG(PixelFormats, Error)\n> +\t\t<< \"Unsupported V4L2 pixel format \"\n> +\t\t<< utils::hex(v4l2_fourcc);\n> +\n> +\treturn &pixelFormats[0];\n> +}\n> +\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> index 13789e9eb344f95c..dbce550ca8d0b7b1 100644\n> --- a/src/libcamera/stream.cpp\n> +++ b/src/libcamera/stream.cpp\n> @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(PixelFormat pixelformat) const\n>   * handlers provied StreamFormats.\n>   */\n>  StreamConfiguration::StreamConfiguration()\n> -\t: pixelFormat(0), stream_(nullptr)\n> +\t: pixelFormat(), stream_(nullptr)\n>  {\n>  }\n>  \n> @@ -287,7 +287,7 @@ StreamConfiguration::StreamConfiguration()\n>   * \\brief Construct a configuration with stream formats\n>   */\n>  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> -\t: pixelFormat(0), stream_(nullptr), formats_(formats)\n> +\t: pixelFormat(), stream_(nullptr), formats_(formats)\n>  {\n>  }\n>  \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() << \"-\" << utils::hex(pixelFormat.fourcc());\n>  \treturn ss.str();\n>  }\n>  \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index f84bd00570afa38c..e9d3e60198e140a0 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -846,7 +846,7 @@ ImageFormats<PixelFormat> V4L2VideoDevice::formats()\n>  \t\tif (sizes.empty())\n>  \t\t\treturn {};\n>  \n> -\t\tif (formats.addFormat(pixelformat, sizes)) {\n> +\t\tif (formats.addFormat(PixelFormat(pixelformat), sizes)) {\n>  \t\t\tLOG(V4L2, Error)\n>  \t\t\t\t<< \"Could not add sizes for pixel format \"\n>  \t\t\t\t<< pixelformat;\n> @@ -1417,56 +1417,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n>   */\n>  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n>  {\n> -\tswitch (v4l2Fourcc) {\n> -\t/* RGB formats. */\n> -\tcase V4L2_PIX_FMT_RGB24:\n> -\t\treturn DRM_FORMAT_BGR888;\n> -\tcase V4L2_PIX_FMT_BGR24:\n> -\t\treturn DRM_FORMAT_RGB888;\n> -\tcase V4L2_PIX_FMT_ARGB32:\n> -\t\treturn DRM_FORMAT_BGRA8888;\n> -\n> -\t/* YUV packed formats. */\n> -\tcase V4L2_PIX_FMT_YUYV:\n> -\t\treturn DRM_FORMAT_YUYV;\n> -\tcase V4L2_PIX_FMT_YVYU:\n> -\t\treturn DRM_FORMAT_YVYU;\n> -\tcase V4L2_PIX_FMT_UYVY:\n> -\t\treturn DRM_FORMAT_UYVY;\n> -\tcase V4L2_PIX_FMT_VYUY:\n> -\t\treturn DRM_FORMAT_VYUY;\n> -\n> -\t/* YUY planar formats. */\n> -\tcase V4L2_PIX_FMT_NV16:\n> -\tcase V4L2_PIX_FMT_NV16M:\n> -\t\treturn DRM_FORMAT_NV16;\n> -\tcase V4L2_PIX_FMT_NV61:\n> -\tcase V4L2_PIX_FMT_NV61M:\n> -\t\treturn DRM_FORMAT_NV61;\n> -\tcase V4L2_PIX_FMT_NV12:\n> -\tcase V4L2_PIX_FMT_NV12M:\n> -\t\treturn DRM_FORMAT_NV12;\n> -\tcase V4L2_PIX_FMT_NV21:\n> -\tcase V4L2_PIX_FMT_NV21M:\n> -\t\treturn DRM_FORMAT_NV21;\n> -\n> -\t/* Compressed formats. */\n> -\tcase V4L2_PIX_FMT_MJPEG:\n> -\t\treturn DRM_FORMAT_MJPEG;\n> -\n> -\t/* V4L2 formats not yet supported by DRM. */\n> -\tcase V4L2_PIX_FMT_GREY:\n> -\tdefault:\n> -\t\t/*\n> -\t\t * \\todo We can't use LOG() in a static method of a Loggable\n> -\t\t * class. Until we fix the logger, work around it.\n> -\t\t */\n> -\t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> -\t\t\t\tLogError).stream()\n> -\t\t\t<< \"Unsupported V4L2 pixel format \"\n> -\t\t\t<< utils::hex(v4l2Fourcc);\n> -\t\treturn 0;\n> -\t}\n> +\treturn PixelFormat(v4l2Fourcc);\n>  }\n>  \n>  /**\n> @@ -1500,53 +1451,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n>   */\n>  uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n>  {\n> -\tswitch (pixelFormat) {\n> -\t/* RGB formats. */\n> -\tcase DRM_FORMAT_BGR888:\n> -\t\treturn V4L2_PIX_FMT_RGB24;\n> -\tcase DRM_FORMAT_RGB888:\n> -\t\treturn V4L2_PIX_FMT_BGR24;\n> -\tcase DRM_FORMAT_BGRA8888:\n> -\t\treturn V4L2_PIX_FMT_ARGB32;\n> -\n> -\t/* YUV packed formats. */\n> -\tcase DRM_FORMAT_YUYV:\n> -\t\treturn V4L2_PIX_FMT_YUYV;\n> -\tcase DRM_FORMAT_YVYU:\n> -\t\treturn V4L2_PIX_FMT_YVYU;\n> -\tcase DRM_FORMAT_UYVY:\n> -\t\treturn V4L2_PIX_FMT_UYVY;\n> -\tcase DRM_FORMAT_VYUY:\n> -\t\treturn V4L2_PIX_FMT_VYUY;\n> -\n> -\t/*\n> -\t * YUY planar formats.\n> -\t * \\todo Add support for non-contiguous memory planes\n> -\t * \\todo Select the format variant not only based on \\a multiplanar but\n> -\t * also take into account the formats supported by the device.\n> -\t */\n> -\tcase DRM_FORMAT_NV16:\n> -\t\treturn V4L2_PIX_FMT_NV16;\n> -\tcase DRM_FORMAT_NV61:\n> -\t\treturn V4L2_PIX_FMT_NV61;\n> -\tcase DRM_FORMAT_NV12:\n> -\t\treturn V4L2_PIX_FMT_NV12;\n> -\tcase DRM_FORMAT_NV21:\n> -\t\treturn V4L2_PIX_FMT_NV21;\n> -\n> -\t/* Compressed formats. */\n> -\tcase DRM_FORMAT_MJPEG:\n> -\t\treturn V4L2_PIX_FMT_MJPEG;\n> -\t}\n> -\n> -\t/*\n> -\t * \\todo We can't use LOG() in a static method of a Loggable\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> -\treturn 0;\n> +\treturn pixelFormat.v4l2();\n>  }\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/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> index 0ebb8edd49efd1b1..d86c97dab8374d5e 100644\n> --- a/src/qcam/viewfinder.cpp\n> +++ b/src/qcam/viewfinder.cpp\n> @@ -14,7 +14,7 @@\n>  #include \"viewfinder.h\"\n>  \n>  ViewFinder::ViewFinder(QWidget *parent)\n> -\t: QWidget(parent), format_(0), width_(0), height_(0), image_(nullptr)\n> +\t: QWidget(parent), format_(), width_(0), height_(0), image_(nullptr)\n>  {\n>  }\n>  \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 622520479be01f58..e9d7bfd8ee243b78 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -525,7 +525,6 @@ struct PixelFormatPlaneInfo {\n>  };\n>  \n>  struct PixelFormatInfo {\n> -\tPixelFormat format;\n>  \tuint32_t v4l2Format;\n>  \tunsigned int numPlanes;\n>  \tstd::array<PixelFormatPlaneInfo, 3> planes;\n> @@ -533,24 +532,24 @@ 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> -\t{ DRM_FORMAT_BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n>  \t/* YUV packed formats. */\n> -\t{ DRM_FORMAT_UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n>  \t/* YUY planar formats. */\n> -\t{ DRM_FORMAT_NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> -\t{ DRM_FORMAT_NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> -}};\n> +\t{ V4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ V4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> +} };\n>  \n>  } /* namespace */\n>  \n> @@ -588,24 +587,10 @@ unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n>  \n>  PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n>  {\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\n> -\t\t\t\t\t return info.v4l2Format == format;\n> -\t\t\t\t });\n> -\tif (info == pixelFormatInfo.end())\n> -\t\treturn format;\n> -\n> -\treturn info->format;\n> +\treturn PixelFormat(format);\n>  }\n>  \n>  uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)\n>  {\n> -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> -\t\t\t\t [format](const PixelFormatInfo &info) {\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> -\n> -\treturn info->v4l2Format;\n> +\treturn format.v4l2();\n>  }\n> diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> index a391f5cd087d3872..a904d681c1691889 100644\n> --- a/test/stream/stream_formats.cpp\n> +++ b/test/stream/stream_formats.cpp\n> @@ -7,6 +7,8 @@\n>  \n>  #include <iostream>\n>  \n> +#include <linux/drm_fourcc.h>\n> +\n>  #include <libcamera/geometry.h>\n>  #include <libcamera/stream.h>\n>  \n> @@ -53,42 +55,49 @@ protected:\n>  \n>  \tint run()\n>  \t{\n> +\t\tstd::vector<PixelFormat> pixelFormats = {\n> +\t\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_UYVY, {}),\n> +\t\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> +\t\t};\n> +\n>  \t\t/* Test discrete sizes */\n>  \t\tStreamFormats discrete({\n> -\t\t\t{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },\n> -\t\t\t{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },\n> +\t\t\t{ pixelFormats[0], { SizeRange(100, 100), SizeRange(200, 200) } },\n> +\t\t\t{ pixelFormats[1], { SizeRange(300, 300), SizeRange(400, 400) } },\n>  \t\t});\n>  \n> -\t\tif (testSizes(\"discrete 1\", discrete.sizes(1),\n> +\t\tif (testSizes(\"discrete 1\", discrete.sizes(pixelFormats[0]),\n>  \t\t\t      { Size(100, 100), Size(200, 200) }))\n>  \t\t\treturn TestFail;\n> -\t\tif (testSizes(\"discrete 2\", discrete.sizes(2),\n> +\t\tif (testSizes(\"discrete 2\", discrete.sizes(pixelFormats[1]),\n>  \t\t\t      { Size(300, 300), Size(400, 400) }))\n>  \t\t\treturn TestFail;\n>  \n>  \t\t/* Test range sizes */\n>  \t\tStreamFormats range({\n> -\t\t\t{ 1, { SizeRange(640, 480, 640, 480) } },\n> -\t\t\t{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },\n> -\t\t\t{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },\n> -\t\t\t{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> +\t\t\t{ pixelFormats[0], { SizeRange(640, 480, 640, 480) } },\n> +\t\t\t{ pixelFormats[1], { SizeRange(640, 480, 800, 600, 8, 8) } },\n> +\t\t\t{ pixelFormats[2], { SizeRange(640, 480, 800, 600, 16, 16) } },\n> +\t\t\t{ pixelFormats[3], { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n>  \t\t});\n>  \n> -\t\tif (testSizes(\"range 1\", range.sizes(1), { Size(640, 480) }))\n> +\t\tif (testSizes(\"range 1\", range.sizes(pixelFormats[0]), { Size(640, 480) }))\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testSizes(\"range 2\", range.sizes(2), {\n> +\t\tif (testSizes(\"range 2\", range.sizes(pixelFormats[1]), {\n>  \t\t\t      Size(640, 480), Size(720, 480),\n>  \t\t\t      Size(720, 576), Size(768, 480),\n>  \t\t\t      Size(800, 600) }))\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testSizes(\"range 3\", range.sizes(3), {\n> +\t\tif (testSizes(\"range 3\", range.sizes(pixelFormats[2]), {\n>  \t\t\t      Size(640, 480), Size(720, 480),\n>  \t\t\t      Size(720, 576), Size(768, 480) }))\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (testSizes(\"range 4\", range.sizes(4), {\n> +\t\tif (testSizes(\"range 4\", range.sizes(pixelFormats[3]), {\n>  \t\t\t      Size(1024, 768), Size(1280, 1024),\n>  \t\t\t      Size(2048, 1152), Size(2048, 1536),\n>  \t\t\t      Size(2560, 2048), Size(3200, 2048), }))\n> -- \n> 2.25.1\n>","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com\n\t[IPv6:2a00:1450:4864:20::12b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B3FC62734\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Feb 2020 18:20:01 +0100 (CET)","by mail-lf1-x12b.google.com with SMTP id w27so2686719lfc.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Feb 2020 09:20:01 -0800 (PST)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\tm24sm7501194ljb.81.2020.02.28.09.19.58\n\tfor <libcamera-devel@lists.libcamera.org>\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 28 Feb 2020 09:19:58 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=3WcJr0ysiinHw5PQ1MDs+ILHXvr/5dSrhYqlx60tIFQ=;\n\tb=Z0sKpLQ5jRp2EQNBmmsRCcQb8Ce38sDvqve0JOrfKxXtdo4FI6LxTgGkG7stDQX/Xj\n\tk4N2kvU8VUqd2LOKT5jWdwTvyE9fdb4ubkd6Fjbmtg2upKdXieA928MaYMvh/DjOEx5G\n\tholVQC8y+pxy8s1xlKpyJ8XE5UXN9AVJ9AnAOzoXX2pDksdx7rn3rjr898BWj26L9jda\n\t6CXG1WtetV/mWdL+Ry041iHEzuWMc3Aio75Tw4gOUDZh96rSOjepZYYYs02NoRs7ThHk\n\twy9Ql0lp465ReKUQhAgIWmGddoeZHSfLNMRJ/NQC46WugGW/9tFSGWcs0aiZvqjFWW0n\n\teG3w==","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:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=3WcJr0ysiinHw5PQ1MDs+ILHXvr/5dSrhYqlx60tIFQ=;\n\tb=AP+spOHfISg1EiWGNvvOigNVyDojYEwOnWdFBFq/ky8UpKlaOUX+BykbeZeMMvVeja\n\twEy3NRuk9YbfgvDkY4lNCTA1fvcUjFaGqVwN9rqtxhr8layxzCf4bkbWoCo3XruN+2Fs\n\tlTB3hfrBgKcuEy7zNMv/7MNgDAd3OB5/RXeyCb3gVgytSdmUr7GuGNHEzYxi0BxE4xIk\n\tCEFTvg+7TwY3+cYyZcOLb23GRLrX8CipeCGlSz9O8hOJefR46BiHoBrd91L4vHrmVt4j\n\tLtD4aJaxTBwlXMQD44zv1cIIHY5d8qtdK3b26INZ931LtyjBOSoLArZhLgh5a743xEMC\n\tKI7g==","X-Gm-Message-State":"ANhLgQ0t2BvO9r6cQFvZEe/dnyBBk0TcrlkV4AFMi1/SVH16HJiQxQ+G\n\tOFdrOhyFPBZUNVXyvyOxDLFUZFqPQq0=","X-Google-Smtp-Source":"ADFU+vslPgENgYI1o2Bzw5B4W6p+3JypNp5kKKxiq7yRCQe/M/TiForaYoi93hGIPg50FJc/OVFgJQ==","X-Received":"by 2002:a05:6512:3a6:: with SMTP id\n\tv6mr3261142lfp.205.1582910399488; \n\tFri, 28 Feb 2020 09:19:59 -0800 (PST)","Date":"Fri, 28 Feb 2020 18:19:58 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200228171958.GC299107@oden.dyn.berto.se>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228032913.497826-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","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":"Fri, 28 Feb 2020 17:20:01 -0000"}},{"id":3857,"web_url":"https://patchwork.libcamera.org/comment/3857/","msgid":"<20200229125748.GD18738@pendragon.ideasonboard.com>","date":"2020-02-29T12:57:48","subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","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 Fri, Feb 28, 2020 at 12:36:17PM +0100, Niklas Söderlund wrote:\n> On 2020-02-28 11:27:37 +0100, Jacopo Mondi wrote:\n> > On Fri, Feb 28, 2020 at 04:29:10AM +0100, Niklas Söderlund wrote:\n> > > Create a class to represent the pixel formats. This is done to prepare\n> > > to add support for modifiers for the formats. One bonus with this change\n> > > it that it's allows it to make it impossible for other then designated\n\n\"is that it disallows creation of PixelFormat instances from a V4L2\nfourcc for classes other than the explicitly designed ones\n(V4L2VideoDevice and V4L2CameraProxy). This limits the risk ...\"\n\n> > > classes (V4L2VideoDevice and V4L2CameraProxy) to create PixelFormat\n> > > instances from a V4L2 fourcc limiting the risk of users of the class to\n> > > mix the two fourcc namespaces unintentionally.\n> > >\n> > > The patch is unfortunately quiet large as it have to touch a lot of\n\ns/quiet/quite/\ns/have/has/\n\n> > > different ares of the code to simultaneously switch to the new class\n> > > based PixelFormat implementation.\n\ns/class based/class-based/\n\n> > \n> > I prasie the effort of making PixelFormat more descriptive and\n> > centralize there the conversion between different 4cc sets, but I'm\n> > not sure I like the way pipeline handlers can now construct\n> > PixelFormat from both DRM and V4L2 fourcc. Please see below.\n> \n> No they can't the constructor to create a PixelFormat from a V4L2 fourcc \n> is protected and only allowed to be called from V4L2VideoDevice and \n> V4L2CameraProxy. This I think is fair as they really are the two \n> locations where we have V4L2 fourc and need to create a PixelFormat from \n> them :-)\n> \n> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > > ---\n> > >  include/libcamera/pixelformats.h         |  40 +++++++-\n> > >  src/cam/main.cpp                         |   4 +-\n> > >  src/libcamera/formats.cpp                |   3 +\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp     |   6 +-\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  22 ++--\n> > >  src/libcamera/pipeline/uvcvideo.cpp      |   4 +-\n> > >  src/libcamera/pipeline/vimc.cpp          |  12 +--\n> > >  src/libcamera/pixelformats.cpp           | 124 +++++++++++++++++++++++\n> > >  src/libcamera/stream.cpp                 |   6 +-\n> > >  src/libcamera/v4l2_videodevice.cpp       | 101 +-----------------\n> > >  src/qcam/format_converter.cpp            |   2 +-\n> > >  src/qcam/viewfinder.cpp                  |   2 +-\n> > >  src/v4l2/v4l2_camera_proxy.cpp           |  49 ++++-----\n> > >  test/stream/stream_formats.cpp           |  33 +++---\n> > >  14 files changed, 236 insertions(+), 172 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> > > index 6e25b8d8b76e5961..f0951e983192d5e8 100644\n> > > --- a/include/libcamera/pixelformats.h\n> > > +++ b/include/libcamera/pixelformats.h\n> > > @@ -7,11 +7,49 @@\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> > > +/* Needs to be forward declared in the global namespace. */\n> > > +class V4L2CameraProxy;\n> > >\n\nI'm afraid this is a showstopper. I think it's bad enough to expose V4L2\nbelow in the public libcamera API, but exposing an internal class of the\nV4L2 compatibility layer, which isn't part of the libcamera.so, is even\nworse.\n\n> > >  namespace libcamera {\n> > >\n> > > -using PixelFormat = uint32_t;\n> > > +struct PixelFormatEntry;\n> > > +\n> > > +class PixelFormat\n> > > +{\n> > > +public:\n> > > +\tPixelFormat();\n> > > +\tPixelFormat(const PixelFormat &other);\n> > > +\texplicit PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers);\n\nNo need for explicit, a constructor with two arguments is always\nexplicit. However, you may want to set a default value for the modifiers\nparameter (= {}), in which case the explicit keyword would make sense.\n\n> > > +\n> > > +\tPixelFormat &operator=(const PixelFormat &other);\n> > > +\n> > > +\tbool operator==(const PixelFormat &other) const;\n> > > +\tbool operator!=(const PixelFormat &other) const;\n\nYou can define this inline as\n\n\t{\n\t\treturn !(*this == other);\n\t}\n\n> > > +\tbool operator<(const PixelFormat &other) const;\n\nA recommended practice is to define those operators as non-member\nfunction:\n\nbool operator==(const PixelFormat &lhs, const PixelFormat &rhs) const;\nbool operator!=(const PixelFormat &lhs, const PixelFormat &rhs) const;\nbool operator<(const PixelFormat &lhs, const PixelFormat &rhs) const;\n\nThe reason is that, otherwise, implicit conversions would work\ndifferently on the left- and right-hand side (although in this case all\nconstructors are explicit, but I think it's still a good idea to follow\nbest practice rules).\n\nCompare those two usages of operator== (assuming the constructor that\ntakes a DRM 4CC loses its explicit keyword, and gets a default value for\nthe last parameter).\n\n\tPixelFormat fmt = ...;\n\n\tif (fmt == DRM_FORMAT_NV12)\n\t\t...\n\tif (DRM_FORMAT_NV12 == fmt)\n\t\t...\n\nWith operator== defined as a member function, the first comparison would\ncompile fine with PixelFormat(DRM_FORMAT_NV12) being called implicitly,\nwhile the second would fail to compile. With non-member comparison, both\nwill compile.\n\n> > > +\n> > > +\tuint32_t v4l2() const;\n> > > +\tuint32_t fourcc() const;\n> > > +\tconst std::set<uint32_t> &modifiers() const;\n> > > +\n> > > +protected:\n> > > +\t/* Needed so V4L2VideoDevice can create PixelFormat from V4L2 fourcc. */\n> > > +\tfriend class V4L2VideoDevice;\n> > > +\n> > > +\t/* Needed so V4L2CameraProxy can create PixelFormat from V4L2 fourcc. */\n> > > +\tfriend V4L2CameraProxy;\n> > > +\n> > > +\texplicit PixelFormat(uint32_t v4l2_fourcc);\n\nThis should be moved somewhere else, V4L2 should not appear in this\nheader at all. I'd even rename drm_fourcc to fourcc for this reason (as\nwell as drm_modifiers to modifiers), and fromDRM to fromFourcc (or\nfrom4CC) for the same reason.\n\n> > > +\n> > > +private:\n> > > +\tconst PixelFormatEntry *fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const;\n> > > +\tconst PixelFormatEntry *fromV4L2(uint32_t v4l2_fourcc) const;\n> > > +\n> > > +\tconst PixelFormatEntry *format_;\n> > > +};\n> > >\n> > >  } /* namespace libcamera */\n> > >\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index ad4be55fc114fe22..c8ef79daea37d8b6 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -249,7 +249,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\nYes, a default value for the second parameter is useful. I know it will\nnot allow having a V4L2 constructor taking an unsigned int, but that\nshould be removed anyway.\n\n> > >  \t\t}\n> > >  \t}\n> > >\n> > > @@ -283,7 +283,7 @@ int CamApp::infoConfiguration()\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\t\t  << std::setw(8) << pixelformat.fourcc() << \" \"\n> > >  \t\t\t\t  << formats.range(pixelformat).toString()\n> > >  \t\t\t\t  << std::endl;\n> > >\n> > > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > > index 98817deee2b54c84..e3a89121e3c60151 100644\n> > > --- a/src/libcamera/formats.cpp\n> > > +++ b/src/libcamera/formats.cpp\n> > > @@ -9,6 +9,8 @@\n> > >\n> > >  #include <errno.h>\n> > >\n> > > +#include <libcamera/pixelformats.h>\n> > > +\n> > >  /**\n> > >   * \\file formats.h\n> > >   * \\brief Types and helper methods to handle libcamera image formats\n> > > @@ -110,5 +112,6 @@ const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const\n> > >  }\n> > >\n> > >  template class ImageFormats<unsigned int>;\n> > > +template class ImageFormats<PixelFormat>;\n\nLooks like we should possibly split the class in two. I think\nrefactoring is needed in any case, this doesn't feel right.\n\n> > >\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 33f97b340716abd0..71ac0ae33921f548 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -247,7 +247,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,\n> > >  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)\n> > >  {\n> > >  \t/* The only pixel format the driver supports is NV12. */\n> > > -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> > \n> > This is good. However you use the second parameter to differentiate\n> > between constructing a PixelFormat from DRM or V4L2, which seems a bit\n> > fragile and prevents you from defaulting the second parameter to an\n> > empty modifier. What is the point of requesting to provide {} if no\n> > modifier is associated with the format ?\n> \n> I agree it would be nice to find a way to make the modifiers argument \n> optional. I'm toying with the idea of replacing the \n> PixelFormat(v4l2_fourcc) with a protected factory function as it can \n> only be called from two classes to make it more explicit that it is \n> dealing with special cases.\n> \n> > >\n> > >  \tif (scale) {\n> > >  \t\t/*\n> > > @@ -402,7 +402,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > >  \t\tStreamConfiguration cfg = {};\n> > >  \t\tIPU3Stream *stream = nullptr;\n> > >\n> > > -\t\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n\nShould this constructor (with a default second parameter) really be\nexplicit ? \n\n\t\tcfg.pixelFormat = DRM_FORMAT_NV12;\n\ndoesn't seem bad.\n\n> > >\n> > >  \t\tswitch (role) {\n> > >  \t\tcase StreamRole::StillCapture:\n> > > @@ -1142,7 +1142,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,\n> > >  \t\treturn 0;\n> > >\n> > >  \tV4L2DeviceFormat outputFormat = {};\n> > > -\toutputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);\n> > > +\toutputFormat.fourcc = PixelFormat(DRM_FORMAT_NV12, {}).v4l2();\n> > \n> > Have you consider providing a static method to perform the DRM<->v4l2\n> > fourcc conversion ? Here you create a PixelFormat instance just to\n> > call .v4l2() on it. The only thing you care about is the translation\n> > between the two 4cc codes, wouldn't this be better expressed as\n> >         PixelFormat::toV4L2(DRM_FORMAT_NV12);\n> \n> I think this is a bad idea. What we want is a type to represent \n> PixelFormat that (from most places) only can be constructed from DRM \n> fourccs.\n> \n> I toyed with the possibility of makeing PixelFormat::v4l2() protected to \n> \"hide\" it form all but the two V4L2 centric classes. This would be nice \n> expect then we would need DRM fourcc for all Meta formats which is \n> probably not what we want.\n\nI'm sorry, but I don't see why we need to move V4L2 4CC handling to the\nPixelFormat class. Keeping it in internal V4L2 helpers is better, it\nshould really not be part of the public API.\n\n> > ?\n> > \n> > >  \toutputFormat.size = cfg.size;\n> > >  \toutputFormat.planesCount = 2;\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 13433b216747cb8b..323fa3596c6ee242 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -433,14 +433,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,\n> > >\n> > >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  {\n> > > -\tstatic const std::array<unsigned int, 8> formats{\n> > > -\t\tDRM_FORMAT_YUYV,\n> > > -\t\tDRM_FORMAT_YVYU,\n> > > -\t\tDRM_FORMAT_VYUY,\n> > > -\t\tDRM_FORMAT_NV16,\n> > > -\t\tDRM_FORMAT_NV61,\n> > > -\t\tDRM_FORMAT_NV21,\n> > > -\t\tDRM_FORMAT_NV12,\n> > > +\tstatic const std::array<PixelFormat, 8> formats{\n> > > +\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_NV16, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_NV61, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_NV21, {}),\n> > > +\t\tPixelFormat(DRM_FORMAT_NV12, {}),\n> > >  \t\t/* \\todo Add support for 8-bit greyscale to DRM formats */\n> > >  \t};\n> > >\n> > > @@ -462,7 +462,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> > >  \tif (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==\n> > >  \t    formats.end()) {\n> > \n> > I am not sure what we get by making pipeline handler use the\n> > PixelFormat class here just to enumerate and match 4cc codes.\n> \n> cfg.pixelFormat comes from the application and is a PixelFormat. It may \n> contains modifiers so we can not just comapre cfg.pixelFormat.fourcc() \n> with the V4L2 fourcc we need to compare the whole thing.\n> \n> > >  \t\tLOG(RkISP1, Debug) << \"Adjusting format to NV12\";\n> > > -\t\tcfg.pixelFormat = DRM_FORMAT_NV12,\n> > > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> > \n> > Here instead is good. Application should be provided a PixelFormat\n> > class\n> > \n> > >  \t\tstatus = Adjusted;\n> > >  \t}\n> > >\n> > > @@ -541,7 +541,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera\n> > >  \t\treturn config;\n> > >\n> > >  \tStreamConfiguration cfg{};\n> > > -\tcfg.pixelFormat = DRM_FORMAT_NV12;\n> > > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12, {});\n> > >  \tcfg.size = data->sensor_->resolution();\n> > >\n> > >  \tconfig->addConfiguration(cfg);\n> > > @@ -796,7 +796,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 8efd188e75a3d135..5f3e52f691aaeae4 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > > @@ -115,8 +115,8 @@ 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 \" << pixelFormat.fourcc()\n> > > +\t\t\t<< \" to \" << cfg.pixelFormat.fourcc();\n> > >  \t\tstatus = Adjusted;\n> > >  \t}\n> > >\n> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > > index 9fbe33c626e327d4..a591c424919b0783 100644\n> > > --- a/src/libcamera/pipeline/vimc.cpp\n> > > +++ b/src/libcamera/pipeline/vimc.cpp\n> > > @@ -106,10 +106,10 @@ private:\n> > >\n> > >  namespace {\n> > >\n> > > -constexpr std::array<unsigned int, 3> pixelformats{\n> > > -\tDRM_FORMAT_RGB888,\n> > > -\tDRM_FORMAT_BGR888,\n> > > -\tDRM_FORMAT_BGRA8888,\n> > > +static const std::array<PixelFormat, 3> pixelformats{\n> > > +\tPixelFormat(DRM_FORMAT_RGB888, {}),\n> > > +\tPixelFormat(DRM_FORMAT_BGR888, {}),\n> > > +\tPixelFormat(DRM_FORMAT_BGRA8888, {}),\n> > >  };\n> > >\n> > >  } /* namespace */\n> > > @@ -138,7 +138,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()\n> > >  \tif (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==\n> > >  \t    pixelformats.end()) {\n> > >  \t\tLOG(VIMC, Debug) << \"Adjusting format to RGB24\";\n> > > -\t\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> > > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n> > >  \t\tstatus = Adjusted;\n> > >  \t}\n> > >\n> > > @@ -187,7 +187,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > >\n> > >  \tStreamConfiguration cfg(formats.data());\n> > >\n> > > -\tcfg.pixelFormat = DRM_FORMAT_BGR888;\n> > > +\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888, {});\n> > >  \tcfg.size = { 1920, 1080 };\n> > >  \tcfg.bufferCount = 4;\n> > >\n> > > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp\n> > > index c03335400b709d9b..b2aacbc39b9ca16a 100644\n> > > --- a/src/libcamera/pixelformats.cpp\n> > > +++ b/src/libcamera/pixelformats.cpp\n> > > @@ -7,6 +7,13 @@\n> > >\n> > >  #include <libcamera/pixelformats.h>\n> > >\n> > > +#include <vector>\n> > > +\n> > > +#include <linux/drm_fourcc.h>\n> > > +#include <linux/videodev2.h>\n> > > +\n> > > +#include \"log.h\"\n> > > +\n> > >  /**\n> > >   * \\file pixelformats.h\n> > >   * \\brief libcamera pixel formats\n> > > @@ -14,6 +21,8 @@\n> > >\n> > >  namespace libcamera {\n> > >\n> > > +LOG_DEFINE_CATEGORY(PixelFormats)\n> > > +\n> > >  /**\n> > >   * \\typedef PixelFormat\n> > >   * \\brief libcamera image pixel format\n> > > @@ -25,4 +34,119 @@ namespace libcamera {\n> > >   * \\todo Add support for format modifiers\n> > >   */\n> > >\n> > > +struct PixelFormatEntry {\n> > > +\tuint32_t v4l2;\n> > > +\tuint32_t drm;\n> > > +\tstd::set<uint32_t> modifiers;\n> > > +};\n> > > +\n> > > +static const std::vector<PixelFormatEntry> pixelFormats = {\n> > > +\t/* Invalid format, important to be first in list. */\n> > > +\t{ .v4l2 = 0,\t\t\t.drm = DRM_FORMAT_INVALID,\t.modifiers = {} },\n> > > +\t/* RGB formats. */\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_RGB24,\t.drm = DRM_FORMAT_BGR888,\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_BGR24,\t.drm = DRM_FORMAT_RGB888,\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_ARGB32,\t.drm = DRM_FORMAT_BGRA8888,\t.modifiers = {} },\n> > > +\t/* YUV packed formats. */\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_YUYV,\t.drm = DRM_FORMAT_YUYV,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_YVYU,\t.drm = DRM_FORMAT_YVYU,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_UYVY,\t.drm = DRM_FORMAT_UYVY,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_VYUY,\t.drm = DRM_FORMAT_VYUY,\t\t.modifiers = {} },\n> > > +\t/* YUY planar formats. */\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV16,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV16M,\t.drm = DRM_FORMAT_NV16,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV61,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV61M,\t.drm = DRM_FORMAT_NV61,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV12,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV12M,\t.drm = DRM_FORMAT_NV12,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV21,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_NV21M,\t.drm = DRM_FORMAT_NV21,\t\t.modifiers = {} },\n> > > +\t/* Compressed formats. */\n> > > +\t{ .v4l2 = V4L2_PIX_FMT_MJPEG,\t.drm = DRM_FORMAT_MJPEG,\t.modifiers = {} },\n> > > +};\n> > > +\n> > > +PixelFormat::PixelFormat()\n> > > +\t: format_(&pixelFormats[0])\n> > > +{\n> > > +}\n> > > +\n> > > +PixelFormat::PixelFormat(const PixelFormat &other)\n> > > +\t: format_(other.format_)\n> > > +{\n> > > +}\n> > > +\n> > > +PixelFormat::PixelFormat(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers)\n> > > +\t: format_(fromDRM(drm_fourcc, drm_modifiers))\n> > > +{\n> > > +}\n> > > +\n> > > +PixelFormat::PixelFormat(uint32_t v4l2_fourcc)\n> > > +\t: format_(fromV4L2(v4l2_fourcc))\n> > > +{\n> > > +}\n> > \n> > This seems very fragile. The distinction between constructing a\n> > PixelFormat with a DRM 4cc or a V4L2 fourcc (which are both uint32_t)\n> > only depends on the presence of the second, right now empty,\n> > parameter.\n> > \n> > What I would do is instead make it mandatory to construct a\n> > PixelFormat with a valid DRM 4cc, and provide to pipeline handler a\n> > static method(s) to convert between drm/v4l2 whenever they consider it\n> > oppotune.\n> \n> It is already mandatory to create it from a DRM fourcc.\n> \n> I do not like the static methods that allow converting between drm/v4l2 \n> as the conversion is not as simple fourcc to fourcc it's also modifiers.  \n> By not having them we have one way to do things. The static methods of \n> the past IMHO have contributed to the mess we have today where one is \n> not sure what namespace the fourcc is in as it's converted back and \n> forth at different parts of the code.\n\nWe could create a V4L2PixelFormat struct to fix this\n\nstruct V4L2PixelFormat\n{\n\tuint32_t fourcc;\n};\n\n(with a constructor taking an uint32_t if needed).\n\nYou could extend that with a constructor that takes a PixelFormat, and\nadd a toPixelFormat() method. I think this would be better than trying\nto use PixelFormat for both. It will also make the PixelFormat class\nlighter, removing the lookup from the pixelFormats array every time a\nPixelFormat is constructed. The array will still be needed to get names,\nbut it shouldn't contain V4L2 4CCs, and shouldn't contain modifiers\neither.\n\n> > > +\n> > > +PixelFormat &PixelFormat::operator=(const PixelFormat &other)\n> > > +{\n> > > +\tformat_ = other.format_;\n> > > +\n> > > +\treturn *this;\n> > > +}\n> > > +\n> > > +bool PixelFormat::operator==(const PixelFormat &other) const\n> > > +{\n> > > +\treturn format_ == other.format_;\n> > > +}\n> > > +\n> > > +bool PixelFormat::operator!=(const PixelFormat &other) const\n> > > +{\n> > > +\treturn format_ != other.format_;\n> > > +}\n> > > +\n> > > +bool PixelFormat::operator<(const PixelFormat &other) const\n> > > +{\n> > > +\treturn format_ > other.format_;\n\nShould this be < ?\n\n> > > +}\n> > > +\n> > > +uint32_t PixelFormat::v4l2() const\n> > > +{\n> > > +\treturn format_->v4l2;\n> > > +}\n> > > +\n> > > +uint32_t PixelFormat::fourcc() const\n> > > +{\n> > > +\treturn format_->drm;\n> > > +}\n> > > +\n> > > +const std::set<uint32_t> &PixelFormat::modifiers() const\n> > > +{\n> > > +\treturn format_->modifiers;\n> > > +}\n> > > +\n> > > +const PixelFormatEntry *PixelFormat::fromDRM(uint32_t drm_fourcc, const std::set<uint32_t> &drm_modifiers) const\n> > > +{\n> > > +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> > > +\t\tif (entry.drm == drm_fourcc &&\n> > > +\t\t    entry.modifiers == drm_modifiers)\n> > > +\t\t\treturn &entry;\n> > > +\n> > > +\tLOG(PixelFormats, Error)\n> > > +\t\t<< \"Unsupported DRM pixel format \"\n> > > +\t\t<< utils::hex(drm_fourcc);\n> > > +\n> > > +\treturn &pixelFormats[0];\n\nI think we should drop validation, and store the fourcc and modifiers\ndirectly in PixelFormat. The class should really be a lightweight\nwrapper.\n\nThis being said, I may be convinced that validation at construction time\nis a good thing, but not for the purpose of supporting V4L2. I also\nthink the current API would be awkward for application, which\napplication would really write\n\n\tPixelFormat format(DRM_FORMAT_NV12);\n\tif (format.fourcc() == 0) {\n\t\t/* Handle error. */\n\t}\n\n?\n\nMaking PixelFormat just a wrapper around fourcc + modifiers seems better\nto me. The name() function will need to be adjusted to perform the\nlookup on demand (possibly caching it ?), and returning a name\nconstructed from the fourcc if no name is found in the database.\n\n> > > +}\n> > > +\n> > > +const PixelFormatEntry *PixelFormat::fromV4L2(uint32_t v4l2_fourcc) const\n> > > +{\n> > > +\tfor (const PixelFormatEntry &entry : pixelFormats)\n> > > +\t\tif (entry.v4l2 == v4l2_fourcc)\n> > > +\t\t\treturn &entry;\n> > > +\n> > > +\tLOG(PixelFormats, Error)\n> > > +\t\t<< \"Unsupported V4L2 pixel format \"\n> > > +\t\t<< utils::hex(v4l2_fourcc);\n> > > +\n> > > +\treturn &pixelFormats[0];\n> > > +}\n> > > +\n> > >  } /* namespace libcamera */\n> > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp\n> > > index 13789e9eb344f95c..dbce550ca8d0b7b1 100644\n> > > --- a/src/libcamera/stream.cpp\n> > > +++ b/src/libcamera/stream.cpp\n> > > @@ -279,7 +279,7 @@ SizeRange StreamFormats::range(PixelFormat pixelformat) const\n> > >   * handlers provied StreamFormats.\n> > >   */\n> > >  StreamConfiguration::StreamConfiguration()\n> > > -\t: pixelFormat(0), stream_(nullptr)\n> > > +\t: pixelFormat(), stream_(nullptr)\n> > >  {\n> > >  }\n> > >\n> > > @@ -287,7 +287,7 @@ StreamConfiguration::StreamConfiguration()\n> > >   * \\brief Construct a configuration with stream formats\n> > >   */\n> > >  StreamConfiguration::StreamConfiguration(const StreamFormats &formats)\n> > > -\t: pixelFormat(0), stream_(nullptr), formats_(formats)\n> > > +\t: pixelFormat(), stream_(nullptr), formats_(formats)\n> > >  {\n> > >  }\n> > >\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() << \"-\" << utils::hex(pixelFormat.fourcc());\n> > >  \treturn ss.str();\n> > >  }\n> > >\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index f84bd00570afa38c..e9d3e60198e140a0 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -846,7 +846,7 @@ ImageFormats<PixelFormat> V4L2VideoDevice::formats()\n> > >  \t\tif (sizes.empty())\n> > >  \t\t\treturn {};\n> > >\n> > > -\t\tif (formats.addFormat(pixelformat, sizes)) {\n> > > +\t\tif (formats.addFormat(PixelFormat(pixelformat), sizes)) {\n> > >  \t\t\tLOG(V4L2, Error)\n> > >  \t\t\t\t<< \"Could not add sizes for pixel format \"\n> > >  \t\t\t\t<< pixelformat;\n> > > @@ -1417,56 +1417,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media,\n> > >   */\n> > >  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> > >  {\n> > > -\tswitch (v4l2Fourcc) {\n> > > -\t/* RGB formats. */\n> > > -\tcase V4L2_PIX_FMT_RGB24:\n> > > -\t\treturn DRM_FORMAT_BGR888;\n> > > -\tcase V4L2_PIX_FMT_BGR24:\n> > > -\t\treturn DRM_FORMAT_RGB888;\n> > > -\tcase V4L2_PIX_FMT_ARGB32:\n> > > -\t\treturn DRM_FORMAT_BGRA8888;\n> > > -\n> > > -\t/* YUV packed formats. */\n> > > -\tcase V4L2_PIX_FMT_YUYV:\n> > > -\t\treturn DRM_FORMAT_YUYV;\n> > > -\tcase V4L2_PIX_FMT_YVYU:\n> > > -\t\treturn DRM_FORMAT_YVYU;\n> > > -\tcase V4L2_PIX_FMT_UYVY:\n> > > -\t\treturn DRM_FORMAT_UYVY;\n> > > -\tcase V4L2_PIX_FMT_VYUY:\n> > > -\t\treturn DRM_FORMAT_VYUY;\n> > > -\n> > > -\t/* YUY planar formats. */\n> > > -\tcase V4L2_PIX_FMT_NV16:\n> > > -\tcase V4L2_PIX_FMT_NV16M:\n> > > -\t\treturn DRM_FORMAT_NV16;\n> > > -\tcase V4L2_PIX_FMT_NV61:\n> > > -\tcase V4L2_PIX_FMT_NV61M:\n> > > -\t\treturn DRM_FORMAT_NV61;\n> > > -\tcase V4L2_PIX_FMT_NV12:\n> > > -\tcase V4L2_PIX_FMT_NV12M:\n> > > -\t\treturn DRM_FORMAT_NV12;\n> > > -\tcase V4L2_PIX_FMT_NV21:\n> > > -\tcase V4L2_PIX_FMT_NV21M:\n> > > -\t\treturn DRM_FORMAT_NV21;\n> > > -\n> > > -\t/* Compressed formats. */\n> > > -\tcase V4L2_PIX_FMT_MJPEG:\n> > > -\t\treturn DRM_FORMAT_MJPEG;\n> > > -\n> > > -\t/* V4L2 formats not yet supported by DRM. */\n> > > -\tcase V4L2_PIX_FMT_GREY:\n> > > -\tdefault:\n> > > -\t\t/*\n> > > -\t\t * \\todo We can't use LOG() in a static method of a Loggable\n> > > -\t\t * class. Until we fix the logger, work around it.\n> > > -\t\t */\n> > > -\t\tlibcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(),\n> > > -\t\t\t\tLogError).stream()\n> > > -\t\t\t<< \"Unsupported V4L2 pixel format \"\n> > > -\t\t\t<< utils::hex(v4l2Fourcc);\n> > > -\t\treturn 0;\n> > > -\t}\n> > > +\treturn PixelFormat(v4l2Fourcc);\n> > >  }\n> > \n> > Shouldn't we aim to replace all usages of this (and the following)\n> > methods by centralizing the conversion in PixelFormat ? This just\n> > served as an utility function for that purpose. Same for the V4L2\n> > wrapper.\n> \n> Please see patch 5/6 and 6/6.\n> \n> > \n> > If I recall correctly, the single plane/multiplane capabilities of the\n> > V4L2 device should be taken into account to perform the conversion.\n> > Could this be an additional paramter to\n> >                 PixelFormat::toDRM4cc(uint32_t v4l24cc, bool molutplanar) ?\n> \n> It could but we have no consumer of such information today and the goal \n> of this series is not to add it but to allow for format enumeration with \n> modifiers to be built on top. When we need multiplane information to \n> convert from V4L2 fourcc we can add it on top.\n> \n> > >\n> > >  /**\n> > > @@ -1500,53 +1451,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat)\n> > >   */\n> > >  uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar)\n> > >  {\n> > > -\tswitch (pixelFormat) {\n> > > -\t/* RGB formats. */\n> > > -\tcase DRM_FORMAT_BGR888:\n> > > -\t\treturn V4L2_PIX_FMT_RGB24;\n> > > -\tcase DRM_FORMAT_RGB888:\n> > > -\t\treturn V4L2_PIX_FMT_BGR24;\n> > > -\tcase DRM_FORMAT_BGRA8888:\n> > > -\t\treturn V4L2_PIX_FMT_ARGB32;\n> > > -\n> > > -\t/* YUV packed formats. */\n> > > -\tcase DRM_FORMAT_YUYV:\n> > > -\t\treturn V4L2_PIX_FMT_YUYV;\n> > > -\tcase DRM_FORMAT_YVYU:\n> > > -\t\treturn V4L2_PIX_FMT_YVYU;\n> > > -\tcase DRM_FORMAT_UYVY:\n> > > -\t\treturn V4L2_PIX_FMT_UYVY;\n> > > -\tcase DRM_FORMAT_VYUY:\n> > > -\t\treturn V4L2_PIX_FMT_VYUY;\n> > > -\n> > > -\t/*\n> > > -\t * YUY planar formats.\n> > > -\t * \\todo Add support for non-contiguous memory planes\n> > > -\t * \\todo Select the format variant not only based on \\a multiplanar but\n> > > -\t * also take into account the formats supported by the device.\n> > > -\t */\n> > > -\tcase DRM_FORMAT_NV16:\n> > > -\t\treturn V4L2_PIX_FMT_NV16;\n> > > -\tcase DRM_FORMAT_NV61:\n> > > -\t\treturn V4L2_PIX_FMT_NV61;\n> > > -\tcase DRM_FORMAT_NV12:\n> > > -\t\treturn V4L2_PIX_FMT_NV12;\n> > > -\tcase DRM_FORMAT_NV21:\n> > > -\t\treturn V4L2_PIX_FMT_NV21;\n> > > -\n> > > -\t/* Compressed formats. */\n> > > -\tcase DRM_FORMAT_MJPEG:\n> > > -\t\treturn V4L2_PIX_FMT_MJPEG;\n> > > -\t}\n> > > -\n> > > -\t/*\n> > > -\t * \\todo We can't use LOG() in a static method of a Loggable\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> > > -\treturn 0;\n> > > +\treturn pixelFormat.v4l2();\n> > >  }\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/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> > > index 0ebb8edd49efd1b1..d86c97dab8374d5e 100644\n> > > --- a/src/qcam/viewfinder.cpp\n> > > +++ b/src/qcam/viewfinder.cpp\n> > > @@ -14,7 +14,7 @@\n> > >  #include \"viewfinder.h\"\n> > >\n> > >  ViewFinder::ViewFinder(QWidget *parent)\n> > > -\t: QWidget(parent), format_(0), width_(0), height_(0), image_(nullptr)\n> > > +\t: QWidget(parent), format_(), width_(0), height_(0), image_(nullptr)\n> > >  {\n> > >  }\n> > >\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > > index 622520479be01f58..e9d7bfd8ee243b78 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > @@ -525,7 +525,6 @@ struct PixelFormatPlaneInfo {\n> > >  };\n> > >\n> > >  struct PixelFormatInfo {\n> > > -\tPixelFormat format;\n> > >  \tuint32_t v4l2Format;\n> > >  \tunsigned int numPlanes;\n> > >  \tstd::array<PixelFormatPlaneInfo, 3> planes;\n> > > @@ -533,24 +532,24 @@ 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> > > -\t{ DRM_FORMAT_BGRA8888,\tV4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_ARGB32,\t1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > >  \t/* YUV packed formats. */\n> > > -\t{ DRM_FORMAT_UYVY,\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_VYUY,\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_YUYV,\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_YVYU,\tV4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_YVYU,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > >  \t/* YUY planar formats. */\n> > > -\t{ DRM_FORMAT_NV12,\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_NV21,\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_NV16,\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_NV61,\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_NV24,\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > -\t{ DRM_FORMAT_NV42,\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > > -}};\n> > > +\t{ V4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > > +\t{ V4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> > > +} };\n> > \n> > Shouldn't we aim to centralize all of this in PixelFormat ?\n> \n> I thought about it and then decided against it, the information recorded \n> here is specific to the v4l2 wrapper and can be indexed directly on V4L2 \n> fourcc so I think we should leave it where it is.\n> \n> > >\n> > >  } /* namespace */\n> > >\n> > > @@ -588,24 +587,10 @@ unsigned int V4L2CameraProxy::imageSize(uint32_t format, unsigned int width,\n> > >\n> > >  PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\n> > >  {\n> > > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > > -\t\t\t\t [format](const PixelFormatInfo &info) {\n> > > -\t\t\t\t\t return info.v4l2Format == format;\n> > > -\t\t\t\t });\n> > > -\tif (info == pixelFormatInfo.end())\n> > > -\t\treturn format;\n> > > -\n> > > -\treturn info->format;\n> > > +\treturn PixelFormat(format);\n> > >  }\n> > >\n> > >  uint32_t V4L2CameraProxy::drmToV4L2(PixelFormat format)\n> > >  {\n> > > -\tauto info = std::find_if(pixelFormatInfo.begin(), pixelFormatInfo.end(),\n> > > -\t\t\t\t [format](const PixelFormatInfo &info) {\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> > > -\n> > > -\treturn info->v4l2Format;\n> > > +\treturn format.v4l2();\n> > >  }\n> > > diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> > > index a391f5cd087d3872..a904d681c1691889 100644\n> > > --- a/test/stream/stream_formats.cpp\n> > > +++ b/test/stream/stream_formats.cpp\n> > > @@ -7,6 +7,8 @@\n> > >\n> > >  #include <iostream>\n> > >\n> > > +#include <linux/drm_fourcc.h>\n> > > +\n> > >  #include <libcamera/geometry.h>\n> > >  #include <libcamera/stream.h>\n> > >\n> > > @@ -53,42 +55,49 @@ protected:\n> > >\n> > >  \tint run()\n> > >  \t{\n> > > +\t\tstd::vector<PixelFormat> pixelFormats = {\n> > > +\t\t\tPixelFormat(DRM_FORMAT_YUYV, {}),\n> > > +\t\t\tPixelFormat(DRM_FORMAT_YVYU, {}),\n> > > +\t\t\tPixelFormat(DRM_FORMAT_UYVY, {}),\n> > > +\t\t\tPixelFormat(DRM_FORMAT_VYUY, {}),\n> > > +\t\t};\n> > > +\n> > >  \t\t/* Test discrete sizes */\n> > >  \t\tStreamFormats discrete({\n> > > -\t\t\t{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },\n> > > -\t\t\t{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },\n> > > +\t\t\t{ pixelFormats[0], { SizeRange(100, 100), SizeRange(200, 200) } },\n> > > +\t\t\t{ pixelFormats[1], { SizeRange(300, 300), SizeRange(400, 400) } },\n> > >  \t\t});\n> > >\n> > > -\t\tif (testSizes(\"discrete 1\", discrete.sizes(1),\n> > > +\t\tif (testSizes(\"discrete 1\", discrete.sizes(pixelFormats[0]),\n> > >  \t\t\t      { Size(100, 100), Size(200, 200) }))\n> > >  \t\t\treturn TestFail;\n> > > -\t\tif (testSizes(\"discrete 2\", discrete.sizes(2),\n> > > +\t\tif (testSizes(\"discrete 2\", discrete.sizes(pixelFormats[1]),\n> > >  \t\t\t      { Size(300, 300), Size(400, 400) }))\n> > >  \t\t\treturn TestFail;\n> > >\n> > >  \t\t/* Test range sizes */\n> > >  \t\tStreamFormats range({\n> > > -\t\t\t{ 1, { SizeRange(640, 480, 640, 480) } },\n> > > -\t\t\t{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },\n> > > -\t\t\t{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },\n> > > -\t\t\t{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> > > +\t\t\t{ pixelFormats[0], { SizeRange(640, 480, 640, 480) } },\n> > > +\t\t\t{ pixelFormats[1], { SizeRange(640, 480, 800, 600, 8, 8) } },\n> > > +\t\t\t{ pixelFormats[2], { SizeRange(640, 480, 800, 600, 16, 16) } },\n> > > +\t\t\t{ pixelFormats[3], { SizeRange(128, 128, 4096, 4096, 128, 128) } },\n> > >  \t\t});\n> > >\n> > > -\t\tif (testSizes(\"range 1\", range.sizes(1), { Size(640, 480) }))\n> > > +\t\tif (testSizes(\"range 1\", range.sizes(pixelFormats[0]), { Size(640, 480) }))\n> > >  \t\t\treturn TestFail;\n> > >\n> > > -\t\tif (testSizes(\"range 2\", range.sizes(2), {\n> > > +\t\tif (testSizes(\"range 2\", range.sizes(pixelFormats[1]), {\n> > >  \t\t\t      Size(640, 480), Size(720, 480),\n> > >  \t\t\t      Size(720, 576), Size(768, 480),\n> > >  \t\t\t      Size(800, 600) }))\n> > >  \t\t\treturn TestFail;\n> > >\n> > > -\t\tif (testSizes(\"range 3\", range.sizes(3), {\n> > > +\t\tif (testSizes(\"range 3\", range.sizes(pixelFormats[2]), {\n> > >  \t\t\t      Size(640, 480), Size(720, 480),\n> > >  \t\t\t      Size(720, 576), Size(768, 480) }))\n> > >  \t\t\treturn TestFail;\n> > >\n> > > -\t\tif (testSizes(\"range 4\", range.sizes(4), {\n> > > +\t\tif (testSizes(\"range 4\", range.sizes(pixelFormats[3]), {\n> > >  \t\t\t      Size(1024, 768), Size(1280, 1024),\n> > >  \t\t\t      Size(2048, 1152), Size(2048, 1536),\n> > >  \t\t\t      Size(2560, 2048), Size(3200, 2048), }))","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 9321F62689\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 29 Feb 2020 13:58: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 E024C2AF;\n\tSat, 29 Feb 2020 13:58:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1582981092;\n\tbh=r3yWYYdlE2nA4q7GWMqsbSndn0r6bCKC085n/VvOR5Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M9h9jrl/9hoyaKKmD2FP2Wturj69XoXD/rpZ9qK5q83fZ5yCkk/Rx7CJVvAHdj8LG\n\tP8VvcDxgGy58i2xIOLjJvLyBmgthT/Sdun9B6s3VVTyoezCtqwHQ4tbYzVTf7zyY8q\n\tYbvws5wJn9RizYtMBsG6FN+mVne0B3dXWHtBUYI0=","Date":"Sat, 29 Feb 2020 14:57:48 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","Message-ID":"<20200229125748.GD18738@pendragon.ideasonboard.com>","References":"<20200228032913.497826-1-niklas.soderlund@ragnatech.se>\n\t<20200228032913.497826-4-niklas.soderlund@ragnatech.se>\n\t<20200228102737.6w3dw2fcm2bcjmxu@uno.localdomain>\n\t<20200228113617.GA299107@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200228113617.GA299107@oden.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [RFC 3/6] libcamera: PixelFormat: Turn into a\n\tclass","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":"Sat, 29 Feb 2020 12:58:12 -0000"}}]