[{"id":4041,"web_url":"https://patchwork.libcamera.org/comment/4041/","msgid":"<20200317091603.GC4864@pendragon.ideasonboard.com>","date":"2020-03-17T09:16:03","subject":"Re: [libcamera-devel] [PATCH v2 7/8] libcamera: PixelFormat: Make\n\tconstructor explicit","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Tue, Mar 17, 2020 at 04:52:38AM +0100, Niklas Söderlund wrote:\n> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> To achieve the goal of preventing unwanted conversion between a DRM and\n> a V4L2 FourCC, make the PixelFormat constructor that takes an integer\n> value explicit. All users of V4L2 pixel formats flagged by the compiler\n> are fixed.\n> \n> While at it make the compare operations part of PixelFormat class.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> [Niklas: Make the compare operations part of PixelFormat]\n\nI'd move this to the patch that introduces the operators. When I\nmentioned they can be made member functions because the constructor is\nexplicit, it wasn't that they couldn't with an implicit constructor, but\nthat it's a good practice to have non-member operators in that case to\navoid the two following operations to have different behaviours:\n\n\tPixelFormat format;\n\tuint32_t fourcc;\n\n\tif (format == fourcc) { ... } /* Compiles */\n\tif (fourcc == format) { ... } /* Doesn't compile */\n\nUnless we have occurrences of the second in the code, I would make the\noperators member functions in patch 6/8 to avoid the churn.\n\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/pixelformats.h         | 10 ++---\n>  src/gstreamer/gstlibcamera-utils.cpp     |  4 +-\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +--\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++-----\n>  src/libcamera/pipeline/vimc.cpp          | 12 ++---\n>  src/libcamera/pixelformats.cpp           | 57 ++++++++++++------------\n>  src/libcamera/v4l2_videodevice.cpp       | 26 +++++------\n>  src/v4l2/v4l2_camera_proxy.cpp           | 28 ++++++------\n>  test/stream/stream_formats.cpp           | 24 +++++-----\n>  test/v4l2_videodevice/buffer_cache.cpp   |  2 +-\n>  10 files changed, 94 insertions(+), 95 deletions(-)\n> \n> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> index 5ba1ba1b324272b9..3285941382ccdd96 100644\n> --- a/include/libcamera/pixelformats.h\n> +++ b/include/libcamera/pixelformats.h\n> @@ -17,7 +17,11 @@ class PixelFormat\n>  {\n>  public:\n>  \tPixelFormat();\n> -\tPixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});\n> +\texplicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});\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 fourcc() const { return fourcc_; }\n>  \tconst std::set<uint64_t> &modifiers() const { return modifiers_; }\n> @@ -28,10 +32,6 @@ private:\n>  \tstd::set<uint64_t> modifiers_;\n>  };\n>  \n> -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);\n> -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);\n> -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);\n> -\n>  } /* namespace libcamera */\n>  \n>  #endif /* __LIBCAMERA_PIXEL_FORMATS_H__ */\n> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> index f21e94c3eef92737..c13b0ca245386168 100644\n> --- a/src/gstreamer/gstlibcamera-utils.cpp\n> +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> @@ -154,9 +154,9 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n>  \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n>  \t\tconst gchar *format = gst_structure_get_string(s, \"format\");\n>  \t\tgst_format = gst_video_format_from_string(format);\n> -\t\tstream_cfg.pixelFormat = gst_format_to_drm(gst_format);\n> +\t\tstream_cfg.pixelFormat = PixelFormat(gst_format_to_drm(gst_format));\n>  \t} else if (gst_structure_has_name(s, \"image/jpeg\")) {\n> -\t\tstream_cfg.pixelFormat = DRM_FORMAT_MJPEG;\n> +\t\tstream_cfg.pixelFormat = PixelFormat(DRM_FORMAT_MJPEG);\n>  \t} else {\n>  \t\tg_critical(\"Unsupported media type: %s\", gst_structure_get_name(s));\n>  \t}\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 0c2a217c9ea8f6ba..52b6d48aca4394c6 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -246,7 +246,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> @@ -401,7 +401,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> @@ -1079,7 +1079,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 = dev->toV4L2Fourcc(PixelFormat(DRM_FORMAT_NV12));\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 8223b82c4a9c773c..3bbe73c3abd9d75e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -431,14 +431,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> @@ -460,7 +460,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> @@ -539,7 +539,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> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 72924bf2f55d0021..8dad2b40ed4fbb2c 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -104,10 +104,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> +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> @@ -136,7 +136,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> @@ -185,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>  \n>  \tStreamConfiguration cfg(formats);\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 fe9a6a2576978647..1c559fe46d406826 100644\n> --- a/src/libcamera/pixelformats.cpp\n> +++ b/src/libcamera/pixelformats.cpp\n> @@ -41,6 +41,34 @@ PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)\n>  {\n>  }\n>  \n> +/**\n> + * \\brief Compare pixel formats for equality\n> + * \\return True if the two pixel formats are equal, false otherwise\n> + */\n> +bool PixelFormat::operator==(const PixelFormat &other) const\n> +{\n> +\treturn fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;\n> +}\n> +\n> +/**\n> + * \\brief Compare pixel formats for inequality\n> + * \\return True if the two pixel formats are not equal, false otherwise\n> + */\n> +bool PixelFormat::operator!=(const PixelFormat &other) const\n> +{\n> +\treturn !(*this == other);\n> +}\n> +\n> +/**\n> + * \\brief Compare pixel formats for smaller than order\n> + * \\todo Take modifiers into account if FourCC are equal\n> + * \\return True if \\a this is smaller than \\a other, false otherwise\n> + */\n> +bool PixelFormat::operator<(const PixelFormat &other) const\n> +{\n> +\treturn fourcc_ < other.fourcc_;\n> +}\n> +\n>  /**\n>   * \\fn PixelFormat::fourcc() const\n>   * \\brief Retrieve the pixel format FourCC\n> @@ -64,33 +92,4 @@ std::string PixelFormat::toString() const\n>  \treturn str;\n>  }\n>  \n> -/**\n> - * \\brief Compare pixel formats for equality\n> - * \\return True if the two pixel formats are equal, false otherwise\n> - */\n> -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)\n> -{\n> -\treturn lhs.fourcc() == rhs.fourcc() &&\n> -\t       lhs.modifiers() == rhs.modifiers();\n> -}\n> -\n> -/**\n> - * \\brief Compare pixel formats for inequality\n> - * \\return True if the two pixel formats are not equal, false otherwise\n> - */\n> -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)\n> -{\n> -\treturn !(lhs == rhs);\n> -}\n> -\n> -/**\n> - * \\brief Compare pixel formats for smaller than order\n> - * \\todo Take modifiers into account if \\a lhs == \\a rhs.\n> - * \\return True if \\a lhs is smaller than \\a rhs, false otherwise\n> - */\n> -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)\n> -{\n> -\treturn lhs.fourcc() < rhs.fourcc();\n> -}\n> -\n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index b5762a7eabcf4e25..d8d711a951d666e9 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1563,39 +1563,39 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n>  \tswitch (v4l2Fourcc) {\n>  \t/* RGB formats. */\n>  \tcase V4L2_PIX_FMT_RGB24:\n> -\t\treturn DRM_FORMAT_BGR888;\n> +\t\treturn PixelFormat(DRM_FORMAT_BGR888);\n>  \tcase V4L2_PIX_FMT_BGR24:\n> -\t\treturn DRM_FORMAT_RGB888;\n> +\t\treturn PixelFormat(DRM_FORMAT_RGB888);\n>  \tcase V4L2_PIX_FMT_ARGB32:\n> -\t\treturn DRM_FORMAT_BGRA8888;\n> +\t\treturn PixelFormat(DRM_FORMAT_BGRA8888);\n>  \n>  \t/* YUV packed formats. */\n>  \tcase V4L2_PIX_FMT_YUYV:\n> -\t\treturn DRM_FORMAT_YUYV;\n> +\t\treturn PixelFormat(DRM_FORMAT_YUYV);\n>  \tcase V4L2_PIX_FMT_YVYU:\n> -\t\treturn DRM_FORMAT_YVYU;\n> +\t\treturn PixelFormat(DRM_FORMAT_YVYU);\n>  \tcase V4L2_PIX_FMT_UYVY:\n> -\t\treturn DRM_FORMAT_UYVY;\n> +\t\treturn PixelFormat(DRM_FORMAT_UYVY);\n>  \tcase V4L2_PIX_FMT_VYUY:\n> -\t\treturn DRM_FORMAT_VYUY;\n> +\t\treturn PixelFormat(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> +\t\treturn PixelFormat(DRM_FORMAT_NV16);\n>  \tcase V4L2_PIX_FMT_NV61:\n>  \tcase V4L2_PIX_FMT_NV61M:\n> -\t\treturn DRM_FORMAT_NV61;\n> +\t\treturn PixelFormat(DRM_FORMAT_NV61);\n>  \tcase V4L2_PIX_FMT_NV12:\n>  \tcase V4L2_PIX_FMT_NV12M:\n> -\t\treturn DRM_FORMAT_NV12;\n> +\t\treturn PixelFormat(DRM_FORMAT_NV12);\n>  \tcase V4L2_PIX_FMT_NV21:\n>  \tcase V4L2_PIX_FMT_NV21M:\n> -\t\treturn DRM_FORMAT_NV21;\n> +\t\treturn PixelFormat(DRM_FORMAT_NV21);\n>  \n>  \t/* Compressed formats. */\n>  \tcase V4L2_PIX_FMT_MJPEG:\n> -\t\treturn DRM_FORMAT_MJPEG;\n> +\t\treturn PixelFormat(DRM_FORMAT_MJPEG);\n>  \n>  \t/* V4L2 formats not yet supported by DRM. */\n>  \tcase V4L2_PIX_FMT_GREY:\n> @@ -1608,7 +1608,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\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\treturn PixelFormat();\n>  \t}\n>  }\n>  \n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index 3bbbbf79cdb475db..55dd69d37bd65897 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -536,21 +536,21 @@ namespace {\n>  \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{ PixelFormat(DRM_FORMAT_RGB888),\tV4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_BGR888),\tV4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_BGRA8888),\tV4L2_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{ PixelFormat(DRM_FORMAT_UYVY),\t\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_VYUY),\t\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_YUYV),\t\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_YVYU),\t\tV4L2_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> +\t{ PixelFormat(DRM_FORMAT_NV12),\t\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_NV21),\t\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_NV16),\t\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_NV61),\t\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_NV24),\t\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> +\t{ PixelFormat(DRM_FORMAT_NV42),\t\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n>  }};\n>  \n>  } /* namespace */\n> @@ -594,7 +594,7 @@ PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\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> +\t\treturn PixelFormat();\n>  \n>  \treturn info->format;\n>  }\n> diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> index a391f5cd087d3872..92f1574b8a0b315c 100644\n> --- a/test/stream/stream_formats.cpp\n> +++ b/test/stream/stream_formats.cpp\n> @@ -55,40 +55,40 @@ protected:\n>  \t{\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{ PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },\n> +\t\t\t{ PixelFormat(2), { 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(PixelFormat(1)),\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(PixelFormat(2)),\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{ PixelFormat(1), { SizeRange(640, 480, 640, 480) } },\n> +\t\t\t{ PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },\n> +\t\t\t{ PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },\n> +\t\t\t{ PixelFormat(4), { 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(PixelFormat(1)), { 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(PixelFormat(2)), {\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(PixelFormat(3)), {\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(PixelFormat(4)), {\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> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> index c951bc9650dc4e0e..8921605030cfdefb 100644\n> --- a/test/v4l2_videodevice/buffer_cache.cpp\n> +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> @@ -144,7 +144,7 @@ public:\n>  \t\tconst unsigned int numBuffers = 8;\n>  \n>  \t\tStreamConfiguration cfg;\n> -\t\tcfg.pixelFormat = DRM_FORMAT_YUYV;\n> +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_YUYV);\n>  \t\tcfg.size = Size(600, 800);\n>  \t\tcfg.bufferCount = numBuffers;\n>","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73F6760418\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Mar 2020 10:16:09 +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 D32CEF9;\n\tTue, 17 Mar 2020 10:16:08 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584436569;\n\tbh=RHsDSPj/DMWVtqwIkGIrwT4DKYF9Snee2zxqBisyLd4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nD9i71P+vDU3UFTHX3xxQWJvyzYdwMTFgcbxQSJXUzJ8lXkExHLUmpn5aAlA+59sx\n\tL+SqwtTWCMQan+xj8CvMG08J5P6FueO9i73CFF17BfMEfFhkAWr0djwiC4LpewazIf\n\tFLrJdUYycki+G3OfFiOQiH8NTVaE1BQyoS3IwKcs=","Date":"Tue, 17 Mar 2020 11:16:03 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200317091603.GC4864@pendragon.ideasonboard.com>","References":"<20200317035239.2697679-1-niklas.soderlund@ragnatech.se>\n\t<20200317035239.2697679-8-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200317035239.2697679-8-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 7/8] libcamera: PixelFormat: Make\n\tconstructor explicit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 17 Mar 2020 09:16:09 -0000"}},{"id":4042,"web_url":"https://patchwork.libcamera.org/comment/4042/","msgid":"<20200317101514.GA2496015@oden.dyn.berto.se>","date":"2020-03-17T10:15:14","subject":"Re: [libcamera-devel] [PATCH v2 7/8] libcamera: PixelFormat: Make\n\tconstructor explicit","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-03-17 11:16:03 +0200, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 17, 2020 at 04:52:38AM +0100, Niklas Söderlund wrote:\n> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > \n> > To achieve the goal of preventing unwanted conversion between a DRM and\n> > a V4L2 FourCC, make the PixelFormat constructor that takes an integer\n> > value explicit. All users of V4L2 pixel formats flagged by the compiler\n> > are fixed.\n> > \n> > While at it make the compare operations part of PixelFormat class.\n> > \n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > [Niklas: Make the compare operations part of PixelFormat]\n> \n> I'd move this to the patch that introduces the operators. When I\n> mentioned they can be made member functions because the constructor is\n> explicit, it wasn't that they couldn't with an implicit constructor, but\n> that it's a good practice to have non-member operators in that case to\n> avoid the two following operations to have different behaviours:\n> \n> \tPixelFormat format;\n> \tuint32_t fourcc;\n> \n> \tif (format == fourcc) { ... } /* Compiles */\n> \tif (fourcc == format) { ... } /* Doesn't compile */\n> \n> Unless we have occurrences of the second in the code, I would make the\n> operators member functions in patch 6/8 to avoid the churn.\n\nI know, and unfortunately we do, in the rkisp1 and vimc pipeline \nhandlers :-(\n\nBut they really highlight an issue for s/unsigned int/PixelFormat/ which \ncan be made earlier in the series and the comparison operators can be \nfolded into the patch that makes a class out of PixelFormat :-)\n\n> \n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/pixelformats.h         | 10 ++---\n> >  src/gstreamer/gstlibcamera-utils.cpp     |  4 +-\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +--\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++-----\n> >  src/libcamera/pipeline/vimc.cpp          | 12 ++---\n> >  src/libcamera/pixelformats.cpp           | 57 ++++++++++++------------\n> >  src/libcamera/v4l2_videodevice.cpp       | 26 +++++------\n> >  src/v4l2/v4l2_camera_proxy.cpp           | 28 ++++++------\n> >  test/stream/stream_formats.cpp           | 24 +++++-----\n> >  test/v4l2_videodevice/buffer_cache.cpp   |  2 +-\n> >  10 files changed, 94 insertions(+), 95 deletions(-)\n> > \n> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h\n> > index 5ba1ba1b324272b9..3285941382ccdd96 100644\n> > --- a/include/libcamera/pixelformats.h\n> > +++ b/include/libcamera/pixelformats.h\n> > @@ -17,7 +17,11 @@ class PixelFormat\n> >  {\n> >  public:\n> >  \tPixelFormat();\n> > -\tPixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});\n> > +\texplicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});\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 fourcc() const { return fourcc_; }\n> >  \tconst std::set<uint64_t> &modifiers() const { return modifiers_; }\n> > @@ -28,10 +32,6 @@ private:\n> >  \tstd::set<uint64_t> modifiers_;\n> >  };\n> >  \n> > -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);\n> > -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);\n> > -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);\n> > -\n> >  } /* namespace libcamera */\n> >  \n> >  #endif /* __LIBCAMERA_PIXEL_FORMATS_H__ */\n> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp\n> > index f21e94c3eef92737..c13b0ca245386168 100644\n> > --- a/src/gstreamer/gstlibcamera-utils.cpp\n> > +++ b/src/gstreamer/gstlibcamera-utils.cpp\n> > @@ -154,9 +154,9 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,\n> >  \tif (gst_structure_has_name(s, \"video/x-raw\")) {\n> >  \t\tconst gchar *format = gst_structure_get_string(s, \"format\");\n> >  \t\tgst_format = gst_video_format_from_string(format);\n> > -\t\tstream_cfg.pixelFormat = gst_format_to_drm(gst_format);\n> > +\t\tstream_cfg.pixelFormat = PixelFormat(gst_format_to_drm(gst_format));\n> >  \t} else if (gst_structure_has_name(s, \"image/jpeg\")) {\n> > -\t\tstream_cfg.pixelFormat = DRM_FORMAT_MJPEG;\n> > +\t\tstream_cfg.pixelFormat = PixelFormat(DRM_FORMAT_MJPEG);\n> >  \t} else {\n> >  \t\tg_critical(\"Unsupported media type: %s\", gst_structure_get_name(s));\n> >  \t}\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 0c2a217c9ea8f6ba..52b6d48aca4394c6 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -246,7 +246,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> > @@ -401,7 +401,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> > @@ -1079,7 +1079,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 = dev->toV4L2Fourcc(PixelFormat(DRM_FORMAT_NV12));\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 8223b82c4a9c773c..3bbe73c3abd9d75e 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -431,14 +431,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> > @@ -460,7 +460,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> > @@ -539,7 +539,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> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 72924bf2f55d0021..8dad2b40ed4fbb2c 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -104,10 +104,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> > +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> > @@ -136,7 +136,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> > @@ -185,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >  \n> >  \tStreamConfiguration cfg(formats);\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 fe9a6a2576978647..1c559fe46d406826 100644\n> > --- a/src/libcamera/pixelformats.cpp\n> > +++ b/src/libcamera/pixelformats.cpp\n> > @@ -41,6 +41,34 @@ PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)\n> >  {\n> >  }\n> >  \n> > +/**\n> > + * \\brief Compare pixel formats for equality\n> > + * \\return True if the two pixel formats are equal, false otherwise\n> > + */\n> > +bool PixelFormat::operator==(const PixelFormat &other) const\n> > +{\n> > +\treturn fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compare pixel formats for inequality\n> > + * \\return True if the two pixel formats are not equal, false otherwise\n> > + */\n> > +bool PixelFormat::operator!=(const PixelFormat &other) const\n> > +{\n> > +\treturn !(*this == other);\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compare pixel formats for smaller than order\n> > + * \\todo Take modifiers into account if FourCC are equal\n> > + * \\return True if \\a this is smaller than \\a other, false otherwise\n> > + */\n> > +bool PixelFormat::operator<(const PixelFormat &other) const\n> > +{\n> > +\treturn fourcc_ < other.fourcc_;\n> > +}\n> > +\n> >  /**\n> >   * \\fn PixelFormat::fourcc() const\n> >   * \\brief Retrieve the pixel format FourCC\n> > @@ -64,33 +92,4 @@ std::string PixelFormat::toString() const\n> >  \treturn str;\n> >  }\n> >  \n> > -/**\n> > - * \\brief Compare pixel formats for equality\n> > - * \\return True if the two pixel formats are equal, false otherwise\n> > - */\n> > -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)\n> > -{\n> > -\treturn lhs.fourcc() == rhs.fourcc() &&\n> > -\t       lhs.modifiers() == rhs.modifiers();\n> > -}\n> > -\n> > -/**\n> > - * \\brief Compare pixel formats for inequality\n> > - * \\return True if the two pixel formats are not equal, false otherwise\n> > - */\n> > -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)\n> > -{\n> > -\treturn !(lhs == rhs);\n> > -}\n> > -\n> > -/**\n> > - * \\brief Compare pixel formats for smaller than order\n> > - * \\todo Take modifiers into account if \\a lhs == \\a rhs.\n> > - * \\return True if \\a lhs is smaller than \\a rhs, false otherwise\n> > - */\n> > -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)\n> > -{\n> > -\treturn lhs.fourcc() < rhs.fourcc();\n> > -}\n> > -\n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index b5762a7eabcf4e25..d8d711a951d666e9 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1563,39 +1563,39 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\n> >  \tswitch (v4l2Fourcc) {\n> >  \t/* RGB formats. */\n> >  \tcase V4L2_PIX_FMT_RGB24:\n> > -\t\treturn DRM_FORMAT_BGR888;\n> > +\t\treturn PixelFormat(DRM_FORMAT_BGR888);\n> >  \tcase V4L2_PIX_FMT_BGR24:\n> > -\t\treturn DRM_FORMAT_RGB888;\n> > +\t\treturn PixelFormat(DRM_FORMAT_RGB888);\n> >  \tcase V4L2_PIX_FMT_ARGB32:\n> > -\t\treturn DRM_FORMAT_BGRA8888;\n> > +\t\treturn PixelFormat(DRM_FORMAT_BGRA8888);\n> >  \n> >  \t/* YUV packed formats. */\n> >  \tcase V4L2_PIX_FMT_YUYV:\n> > -\t\treturn DRM_FORMAT_YUYV;\n> > +\t\treturn PixelFormat(DRM_FORMAT_YUYV);\n> >  \tcase V4L2_PIX_FMT_YVYU:\n> > -\t\treturn DRM_FORMAT_YVYU;\n> > +\t\treturn PixelFormat(DRM_FORMAT_YVYU);\n> >  \tcase V4L2_PIX_FMT_UYVY:\n> > -\t\treturn DRM_FORMAT_UYVY;\n> > +\t\treturn PixelFormat(DRM_FORMAT_UYVY);\n> >  \tcase V4L2_PIX_FMT_VYUY:\n> > -\t\treturn DRM_FORMAT_VYUY;\n> > +\t\treturn PixelFormat(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> > +\t\treturn PixelFormat(DRM_FORMAT_NV16);\n> >  \tcase V4L2_PIX_FMT_NV61:\n> >  \tcase V4L2_PIX_FMT_NV61M:\n> > -\t\treturn DRM_FORMAT_NV61;\n> > +\t\treturn PixelFormat(DRM_FORMAT_NV61);\n> >  \tcase V4L2_PIX_FMT_NV12:\n> >  \tcase V4L2_PIX_FMT_NV12M:\n> > -\t\treturn DRM_FORMAT_NV12;\n> > +\t\treturn PixelFormat(DRM_FORMAT_NV12);\n> >  \tcase V4L2_PIX_FMT_NV21:\n> >  \tcase V4L2_PIX_FMT_NV21M:\n> > -\t\treturn DRM_FORMAT_NV21;\n> > +\t\treturn PixelFormat(DRM_FORMAT_NV21);\n> >  \n> >  \t/* Compressed formats. */\n> >  \tcase V4L2_PIX_FMT_MJPEG:\n> > -\t\treturn DRM_FORMAT_MJPEG;\n> > +\t\treturn PixelFormat(DRM_FORMAT_MJPEG);\n> >  \n> >  \t/* V4L2 formats not yet supported by DRM. */\n> >  \tcase V4L2_PIX_FMT_GREY:\n> > @@ -1608,7 +1608,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)\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\treturn PixelFormat();\n> >  \t}\n> >  }\n> >  \n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index 3bbbbf79cdb475db..55dd69d37bd65897 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -536,21 +536,21 @@ namespace {\n> >  \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{ PixelFormat(DRM_FORMAT_RGB888),\tV4L2_PIX_FMT_BGR24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_BGR888),\tV4L2_PIX_FMT_RGB24,\t1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_BGRA8888),\tV4L2_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{ PixelFormat(DRM_FORMAT_UYVY),\t\tV4L2_PIX_FMT_UYVY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_VYUY),\t\tV4L2_PIX_FMT_VYUY,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_YUYV),\t\tV4L2_PIX_FMT_YUYV,\t1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_YVYU),\t\tV4L2_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> > +\t{ PixelFormat(DRM_FORMAT_NV12),\t\tV4L2_PIX_FMT_NV12,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_NV21),\t\tV4L2_PIX_FMT_NV21,\t2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_NV16),\t\tV4L2_PIX_FMT_NV16,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_NV61),\t\tV4L2_PIX_FMT_NV61,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_NV24),\t\tV4L2_PIX_FMT_NV24,\t2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },\n> > +\t{ PixelFormat(DRM_FORMAT_NV42),\t\tV4L2_PIX_FMT_NV42,\t2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },\n> >  }};\n> >  \n> >  } /* namespace */\n> > @@ -594,7 +594,7 @@ PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)\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> > +\t\treturn PixelFormat();\n> >  \n> >  \treturn info->format;\n> >  }\n> > diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp\n> > index a391f5cd087d3872..92f1574b8a0b315c 100644\n> > --- a/test/stream/stream_formats.cpp\n> > +++ b/test/stream/stream_formats.cpp\n> > @@ -55,40 +55,40 @@ protected:\n> >  \t{\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{ PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },\n> > +\t\t\t{ PixelFormat(2), { 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(PixelFormat(1)),\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(PixelFormat(2)),\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{ PixelFormat(1), { SizeRange(640, 480, 640, 480) } },\n> > +\t\t\t{ PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },\n> > +\t\t\t{ PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },\n> > +\t\t\t{ PixelFormat(4), { 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(PixelFormat(1)), { 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(PixelFormat(2)), {\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(PixelFormat(3)), {\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(PixelFormat(4)), {\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> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp\n> > index c951bc9650dc4e0e..8921605030cfdefb 100644\n> > --- a/test/v4l2_videodevice/buffer_cache.cpp\n> > +++ b/test/v4l2_videodevice/buffer_cache.cpp\n> > @@ -144,7 +144,7 @@ public:\n> >  \t\tconst unsigned int numBuffers = 8;\n> >  \n> >  \t\tStreamConfiguration cfg;\n> > -\t\tcfg.pixelFormat = DRM_FORMAT_YUYV;\n> > +\t\tcfg.pixelFormat = PixelFormat(DRM_FORMAT_YUYV);\n> >  \t\tcfg.size = Size(600, 800);\n> >  \t\tcfg.bufferCount = numBuffers;\n> >  \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D119660418\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Mar 2020 11:15:16 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id r7so22105299ljp.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 17 Mar 2020 03:15:16 -0700 (PDT)","from localhost (h-200-138.A463.priv.bahnhof.se. [176.10.200.138])\n\tby smtp.gmail.com with ESMTPSA id\n\to13sm2349280lfg.90.2020.03.17.03.15.15\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 17 Mar 2020 03:15:15 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=JngVjoi6fNwtaOF0xE9F9IZchgSHX6Yt+y9JBGoPA2c=;\n\tb=RQSmSTDbseffkVq6XY400AYO5d51o7u8i8TOwcfPdJY8tMW5VQNMUpb2yXcHDQPF0Z\n\tev7Oski6L59pXhz2rv4pksLF1mIdZlMNgtgoRcO7VXW22vzDR5xqMhiEe/4NHW8osA5X\n\tTil3j2XpPJhxb7jrRUfzawWWUXyWkIqvi6U2U8pENpgT7q0SvQTmy3Iuec+Bai0OWJfj\n\tZYaiNoOG4ohT0FhL2IvzNGfiCvplGQM+PBWLLkoZlSozwGAlKKy2tbMHPnBHca8VLvzs\n\t9KRnYov9mlK+uKyWDhmUOAZB5uT96913A3e1bAsvIKkp+HVGyoQkUdNa/VMsjRUjsE0d\n\tmEEg==","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=JngVjoi6fNwtaOF0xE9F9IZchgSHX6Yt+y9JBGoPA2c=;\n\tb=Kx0Ai0uPAJjbX3Gij0i3MVS7RJcZ+RlKdGTfh2uOE6lnKOVR/+ifeaUa0LbBjCmS+d\n\twOoj06CbMDe1WKMNY3xbeY7cfuZJtyD/OD45fFBdK2dklSk1RNburPUtqUk53B0uOjrc\n\tc5hegCuAZQpxFArUc7+dUmaBYGtySPBbXF+TElYeO3pPgVjU1zt+ngeZGCsnTJzGS60m\n\tWdnFsT9DMINvw0DA2mvudz2muTAz0+W6YbBFnV0bRAjxCVMDuF7F/FpPA2r/pvZPMaq1\n\t8HG4eid6sodHyYCthR6tVV/FsDIAMhlH+oQs++Q4YQ6VFVP/HtYeFG4LbWYCu9iYXs3Y\n\t6M/w==","X-Gm-Message-State":"ANhLgQ1tm0EZXOgbHErtBANMEYVA9C6P49lphvncEtsZbSqfugP4FPW9\n\tXSL2w+OXFosKSRg/ScUGpD/J0w==","X-Google-Smtp-Source":"ADFU+vvo8/cUUknC1t9kcnZSiqnQf/owQ9/3UWiPI6hz0GLzmxiPcWT+UHOAakQ8Q0fnh4OKZhugdQ==","X-Received":"by 2002:a2e:98da:: with SMTP id\n\ts26mr2402315ljj.155.1584440115789; \n\tTue, 17 Mar 2020 03:15:15 -0700 (PDT)","Date":"Tue, 17 Mar 2020 11:15:14 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200317101514.GA2496015@oden.dyn.berto.se>","References":"<20200317035239.2697679-1-niklas.soderlund@ragnatech.se>\n\t<20200317035239.2697679-8-niklas.soderlund@ragnatech.se>\n\t<20200317091603.GC4864@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200317091603.GC4864@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 7/8] libcamera: PixelFormat: Make\n\tconstructor explicit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Tue, 17 Mar 2020 10:15:17 -0000"}}]