[libcamera-devel,v2,7/8] libcamera: PixelFormat: Make constructor explicit
diff mbox series

Message ID 20200317035239.2697679-8-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: PixelFormat: Turn into a class
Related show

Commit Message

Niklas Söderlund March 17, 2020, 3:52 a.m. UTC 
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

To achieve the goal of preventing unwanted conversion between a DRM and
a V4L2 FourCC, make the PixelFormat constructor that takes an integer
value explicit. All users of V4L2 pixel formats flagged by the compiler
are fixed.

While at it make the compare operations part of PixelFormat class.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Niklas: Make the compare operations part of PixelFormat]
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 include/libcamera/pixelformats.h         | 10 ++---
 src/gstreamer/gstlibcamera-utils.cpp     |  4 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++-----
 src/libcamera/pipeline/vimc.cpp          | 12 ++---
 src/libcamera/pixelformats.cpp           | 57 ++++++++++++------------
 src/libcamera/v4l2_videodevice.cpp       | 26 +++++------
 src/v4l2/v4l2_camera_proxy.cpp           | 28 ++++++------
 test/stream/stream_formats.cpp           | 24 +++++-----
 test/v4l2_videodevice/buffer_cache.cpp   |  2 +-
 10 files changed, 94 insertions(+), 95 deletions(-)

Comments

Laurent Pinchart March 17, 2020, 9:16 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Tue, Mar 17, 2020 at 04:52:38AM +0100, Niklas Söderlund wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> To achieve the goal of preventing unwanted conversion between a DRM and
> a V4L2 FourCC, make the PixelFormat constructor that takes an integer
> value explicit. All users of V4L2 pixel formats flagged by the compiler
> are fixed.
> 
> While at it make the compare operations part of PixelFormat class.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> [Niklas: Make the compare operations part of PixelFormat]

I'd move this to the patch that introduces the operators. When I
mentioned they can be made member functions because the constructor is
explicit, it wasn't that they couldn't with an implicit constructor, but
that it's a good practice to have non-member operators in that case to
avoid the two following operations to have different behaviours:

	PixelFormat format;
	uint32_t fourcc;

	if (format == fourcc) { ... } /* Compiles */
	if (fourcc == format) { ... } /* Doesn't compile */

Unless we have occurrences of the second in the code, I would make the
operators member functions in patch 6/8 to avoid the churn.

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  include/libcamera/pixelformats.h         | 10 ++---
>  src/gstreamer/gstlibcamera-utils.cpp     |  4 +-
>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++-----
>  src/libcamera/pipeline/vimc.cpp          | 12 ++---
>  src/libcamera/pixelformats.cpp           | 57 ++++++++++++------------
>  src/libcamera/v4l2_videodevice.cpp       | 26 +++++------
>  src/v4l2/v4l2_camera_proxy.cpp           | 28 ++++++------
>  test/stream/stream_formats.cpp           | 24 +++++-----
>  test/v4l2_videodevice/buffer_cache.cpp   |  2 +-
>  10 files changed, 94 insertions(+), 95 deletions(-)
> 
> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> index 5ba1ba1b324272b9..3285941382ccdd96 100644
> --- a/include/libcamera/pixelformats.h
> +++ b/include/libcamera/pixelformats.h
> @@ -17,7 +17,11 @@ class PixelFormat
>  {
>  public:
>  	PixelFormat();
> -	PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> +	explicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> +
> +	bool operator==(const PixelFormat &other) const;
> +	bool operator!=(const PixelFormat &other) const;
> +	bool operator<(const PixelFormat &other) const;
>  
>  	uint32_t fourcc() const { return fourcc_; }
>  	const std::set<uint64_t> &modifiers() const { return modifiers_; }
> @@ -28,10 +32,6 @@ private:
>  	std::set<uint64_t> modifiers_;
>  };
>  
> -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);
> -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);
> -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);
> -
>  } /* namespace libcamera */
>  
>  #endif /* __LIBCAMERA_PIXEL_FORMATS_H__ */
> diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> index f21e94c3eef92737..c13b0ca245386168 100644
> --- a/src/gstreamer/gstlibcamera-utils.cpp
> +++ b/src/gstreamer/gstlibcamera-utils.cpp
> @@ -154,9 +154,9 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
>  	if (gst_structure_has_name(s, "video/x-raw")) {
>  		const gchar *format = gst_structure_get_string(s, "format");
>  		gst_format = gst_video_format_from_string(format);
> -		stream_cfg.pixelFormat = gst_format_to_drm(gst_format);
> +		stream_cfg.pixelFormat = PixelFormat(gst_format_to_drm(gst_format));
>  	} else if (gst_structure_has_name(s, "image/jpeg")) {
> -		stream_cfg.pixelFormat = DRM_FORMAT_MJPEG;
> +		stream_cfg.pixelFormat = PixelFormat(DRM_FORMAT_MJPEG);
>  	} else {
>  		g_critical("Unsupported media type: %s", gst_structure_get_name(s));
>  	}
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0c2a217c9ea8f6ba..52b6d48aca4394c6 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -246,7 +246,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
>  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
>  {
>  	/* The only pixel format the driver supports is NV12. */
> -	cfg.pixelFormat = DRM_FORMAT_NV12;
> +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>  
>  	if (scale) {
>  		/*
> @@ -401,7 +401,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		StreamConfiguration cfg = {};
>  		IPU3Stream *stream = nullptr;
>  
> -		cfg.pixelFormat = DRM_FORMAT_NV12;
> +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>  
>  		switch (role) {
>  		case StreamRole::StillCapture:
> @@ -1079,7 +1079,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
>  		return 0;
>  
>  	V4L2DeviceFormat outputFormat = {};
> -	outputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);
> +	outputFormat.fourcc = dev->toV4L2Fourcc(PixelFormat(DRM_FORMAT_NV12));
>  	outputFormat.size = cfg.size;
>  	outputFormat.planesCount = 2;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8223b82c4a9c773c..3bbe73c3abd9d75e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -431,14 +431,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
>  
>  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
> -	static const std::array<unsigned int, 8> formats{
> -		DRM_FORMAT_YUYV,
> -		DRM_FORMAT_YVYU,
> -		DRM_FORMAT_VYUY,
> -		DRM_FORMAT_NV16,
> -		DRM_FORMAT_NV61,
> -		DRM_FORMAT_NV21,
> -		DRM_FORMAT_NV12,
> +	static const std::array<PixelFormat, 8> formats{
> +		PixelFormat(DRM_FORMAT_YUYV),
> +		PixelFormat(DRM_FORMAT_YVYU),
> +		PixelFormat(DRM_FORMAT_VYUY),
> +		PixelFormat(DRM_FORMAT_NV16),
> +		PixelFormat(DRM_FORMAT_NV61),
> +		PixelFormat(DRM_FORMAT_NV21),
> +		PixelFormat(DRM_FORMAT_NV12),
>  		/* \todo Add support for 8-bit greyscale to DRM formats */
>  	};
>  
> @@ -460,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
>  	    formats.end()) {
>  		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> -		cfg.pixelFormat = DRM_FORMAT_NV12,
> +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),
>  		status = Adjusted;
>  	}
>  
> @@ -539,7 +539,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  		return config;
>  
>  	StreamConfiguration cfg{};
> -	cfg.pixelFormat = DRM_FORMAT_NV12;
> +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
>  	cfg.size = data->sensor_->resolution();
>  
>  	config->addConfiguration(cfg);
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 72924bf2f55d0021..8dad2b40ed4fbb2c 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -104,10 +104,10 @@ private:
>  
>  namespace {
>  
> -constexpr std::array<unsigned int, 3> pixelformats{
> -	DRM_FORMAT_RGB888,
> -	DRM_FORMAT_BGR888,
> -	DRM_FORMAT_BGRA8888,
> +const std::array<PixelFormat, 3> pixelformats{
> +	PixelFormat(DRM_FORMAT_RGB888),
> +	PixelFormat(DRM_FORMAT_BGR888),
> +	PixelFormat(DRM_FORMAT_BGRA8888),
>  };
>  
>  } /* namespace */
> @@ -136,7 +136,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
>  	if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==
>  	    pixelformats.end()) {
>  		LOG(VIMC, Debug) << "Adjusting format to RGB24";
> -		cfg.pixelFormat = DRM_FORMAT_BGR888;
> +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
>  		status = Adjusted;
>  	}
>  
> @@ -185,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  
>  	StreamConfiguration cfg(formats);
>  
> -	cfg.pixelFormat = DRM_FORMAT_BGR888;
> +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
>  	cfg.size = { 1920, 1080 };
>  	cfg.bufferCount = 4;
>  
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index fe9a6a2576978647..1c559fe46d406826 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -41,6 +41,34 @@ PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
>  {
>  }
>  
> +/**
> + * \brief Compare pixel formats for equality
> + * \return True if the two pixel formats are equal, false otherwise
> + */
> +bool PixelFormat::operator==(const PixelFormat &other) const
> +{
> +	return fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;
> +}
> +
> +/**
> + * \brief Compare pixel formats for inequality
> + * \return True if the two pixel formats are not equal, false otherwise
> + */
> +bool PixelFormat::operator!=(const PixelFormat &other) const
> +{
> +	return !(*this == other);
> +}
> +
> +/**
> + * \brief Compare pixel formats for smaller than order
> + * \todo Take modifiers into account if FourCC are equal
> + * \return True if \a this is smaller than \a other, false otherwise
> + */
> +bool PixelFormat::operator<(const PixelFormat &other) const
> +{
> +	return fourcc_ < other.fourcc_;
> +}
> +
>  /**
>   * \fn PixelFormat::fourcc() const
>   * \brief Retrieve the pixel format FourCC
> @@ -64,33 +92,4 @@ std::string PixelFormat::toString() const
>  	return str;
>  }
>  
> -/**
> - * \brief Compare pixel formats for equality
> - * \return True if the two pixel formats are equal, false otherwise
> - */
> -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)
> -{
> -	return lhs.fourcc() == rhs.fourcc() &&
> -	       lhs.modifiers() == rhs.modifiers();
> -}
> -
> -/**
> - * \brief Compare pixel formats for inequality
> - * \return True if the two pixel formats are not equal, false otherwise
> - */
> -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)
> -{
> -	return !(lhs == rhs);
> -}
> -
> -/**
> - * \brief Compare pixel formats for smaller than order
> - * \todo Take modifiers into account if \a lhs == \a rhs.
> - * \return True if \a lhs is smaller than \a rhs, false otherwise
> - */
> -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)
> -{
> -	return lhs.fourcc() < rhs.fourcc();
> -}
> -
>  } /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index b5762a7eabcf4e25..d8d711a951d666e9 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1563,39 +1563,39 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
>  	switch (v4l2Fourcc) {
>  	/* RGB formats. */
>  	case V4L2_PIX_FMT_RGB24:
> -		return DRM_FORMAT_BGR888;
> +		return PixelFormat(DRM_FORMAT_BGR888);
>  	case V4L2_PIX_FMT_BGR24:
> -		return DRM_FORMAT_RGB888;
> +		return PixelFormat(DRM_FORMAT_RGB888);
>  	case V4L2_PIX_FMT_ARGB32:
> -		return DRM_FORMAT_BGRA8888;
> +		return PixelFormat(DRM_FORMAT_BGRA8888);
>  
>  	/* YUV packed formats. */
>  	case V4L2_PIX_FMT_YUYV:
> -		return DRM_FORMAT_YUYV;
> +		return PixelFormat(DRM_FORMAT_YUYV);
>  	case V4L2_PIX_FMT_YVYU:
> -		return DRM_FORMAT_YVYU;
> +		return PixelFormat(DRM_FORMAT_YVYU);
>  	case V4L2_PIX_FMT_UYVY:
> -		return DRM_FORMAT_UYVY;
> +		return PixelFormat(DRM_FORMAT_UYVY);
>  	case V4L2_PIX_FMT_VYUY:
> -		return DRM_FORMAT_VYUY;
> +		return PixelFormat(DRM_FORMAT_VYUY);
>  
>  	/* YUY planar formats. */
>  	case V4L2_PIX_FMT_NV16:
>  	case V4L2_PIX_FMT_NV16M:
> -		return DRM_FORMAT_NV16;
> +		return PixelFormat(DRM_FORMAT_NV16);
>  	case V4L2_PIX_FMT_NV61:
>  	case V4L2_PIX_FMT_NV61M:
> -		return DRM_FORMAT_NV61;
> +		return PixelFormat(DRM_FORMAT_NV61);
>  	case V4L2_PIX_FMT_NV12:
>  	case V4L2_PIX_FMT_NV12M:
> -		return DRM_FORMAT_NV12;
> +		return PixelFormat(DRM_FORMAT_NV12);
>  	case V4L2_PIX_FMT_NV21:
>  	case V4L2_PIX_FMT_NV21M:
> -		return DRM_FORMAT_NV21;
> +		return PixelFormat(DRM_FORMAT_NV21);
>  
>  	/* Compressed formats. */
>  	case V4L2_PIX_FMT_MJPEG:
> -		return DRM_FORMAT_MJPEG;
> +		return PixelFormat(DRM_FORMAT_MJPEG);
>  
>  	/* V4L2 formats not yet supported by DRM. */
>  	case V4L2_PIX_FMT_GREY:
> @@ -1608,7 +1608,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
>  				LogError).stream()
>  			<< "Unsupported V4L2 pixel format "
>  			<< utils::hex(v4l2Fourcc);
> -		return 0;
> +		return PixelFormat();
>  	}
>  }
>  
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 3bbbbf79cdb475db..55dd69d37bd65897 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -536,21 +536,21 @@ namespace {
>  
>  static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
>  	/* RGB formats. */
> -	{ DRM_FORMAT_RGB888,	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_BGR888,	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_BGRA8888,	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_RGB888),	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_BGR888),	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_BGRA8888),	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
>  	/* YUV packed formats. */
> -	{ DRM_FORMAT_UYVY,	V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_VYUY,	V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_YUYV,	V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_YVYU,	V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_UYVY),		V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_VYUY),		V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_YUYV),		V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_YVYU),		V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
>  	/* YUY planar formats. */
> -	{ DRM_FORMAT_NV12,	V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_NV21,	V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_NV16,	V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_NV61,	V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_NV24,	V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> -	{ DRM_FORMAT_NV42,	V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV12),		V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV21),		V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV16),		V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV61),		V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV24),		V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> +	{ PixelFormat(DRM_FORMAT_NV42),		V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
>  }};
>  
>  } /* namespace */
> @@ -594,7 +594,7 @@ PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)
>  					 return info.v4l2Format == format;
>  				 });
>  	if (info == pixelFormatInfo.end())
> -		return format;
> +		return PixelFormat();
>  
>  	return info->format;
>  }
> diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp
> index a391f5cd087d3872..92f1574b8a0b315c 100644
> --- a/test/stream/stream_formats.cpp
> +++ b/test/stream/stream_formats.cpp
> @@ -55,40 +55,40 @@ protected:
>  	{
>  		/* Test discrete sizes */
>  		StreamFormats discrete({
> -			{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },
> -			{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },
> +			{ PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },
> +			{ PixelFormat(2), { SizeRange(300, 300), SizeRange(400, 400) } },
>  		});
>  
> -		if (testSizes("discrete 1", discrete.sizes(1),
> +		if (testSizes("discrete 1", discrete.sizes(PixelFormat(1)),
>  			      { Size(100, 100), Size(200, 200) }))
>  			return TestFail;
> -		if (testSizes("discrete 2", discrete.sizes(2),
> +		if (testSizes("discrete 2", discrete.sizes(PixelFormat(2)),
>  			      { Size(300, 300), Size(400, 400) }))
>  			return TestFail;
>  
>  		/* Test range sizes */
>  		StreamFormats range({
> -			{ 1, { SizeRange(640, 480, 640, 480) } },
> -			{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },
> -			{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },
> -			{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },
> +			{ PixelFormat(1), { SizeRange(640, 480, 640, 480) } },
> +			{ PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },
> +			{ PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },
> +			{ PixelFormat(4), { SizeRange(128, 128, 4096, 4096, 128, 128) } },
>  		});
>  
> -		if (testSizes("range 1", range.sizes(1), { Size(640, 480) }))
> +		if (testSizes("range 1", range.sizes(PixelFormat(1)), { Size(640, 480) }))
>  			return TestFail;
>  
> -		if (testSizes("range 2", range.sizes(2), {
> +		if (testSizes("range 2", range.sizes(PixelFormat(2)), {
>  			      Size(640, 480), Size(720, 480),
>  			      Size(720, 576), Size(768, 480),
>  			      Size(800, 600) }))
>  			return TestFail;
>  
> -		if (testSizes("range 3", range.sizes(3), {
> +		if (testSizes("range 3", range.sizes(PixelFormat(3)), {
>  			      Size(640, 480), Size(720, 480),
>  			      Size(720, 576), Size(768, 480) }))
>  			return TestFail;
>  
> -		if (testSizes("range 4", range.sizes(4), {
> +		if (testSizes("range 4", range.sizes(PixelFormat(4)), {
>  			      Size(1024, 768), Size(1280, 1024),
>  			      Size(2048, 1152), Size(2048, 1536),
>  			      Size(2560, 2048), Size(3200, 2048), }))
> diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> index c951bc9650dc4e0e..8921605030cfdefb 100644
> --- a/test/v4l2_videodevice/buffer_cache.cpp
> +++ b/test/v4l2_videodevice/buffer_cache.cpp
> @@ -144,7 +144,7 @@ public:
>  		const unsigned int numBuffers = 8;
>  
>  		StreamConfiguration cfg;
> -		cfg.pixelFormat = DRM_FORMAT_YUYV;
> +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_YUYV);
>  		cfg.size = Size(600, 800);
>  		cfg.bufferCount = numBuffers;
>
Niklas Söderlund March 17, 2020, 10:15 a.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-03-17 11:16:03 +0200, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Mar 17, 2020 at 04:52:38AM +0100, Niklas Söderlund wrote:
> > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > To achieve the goal of preventing unwanted conversion between a DRM and
> > a V4L2 FourCC, make the PixelFormat constructor that takes an integer
> > value explicit. All users of V4L2 pixel formats flagged by the compiler
> > are fixed.
> > 
> > While at it make the compare operations part of PixelFormat class.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > [Niklas: Make the compare operations part of PixelFormat]
> 
> I'd move this to the patch that introduces the operators. When I
> mentioned they can be made member functions because the constructor is
> explicit, it wasn't that they couldn't with an implicit constructor, but
> that it's a good practice to have non-member operators in that case to
> avoid the two following operations to have different behaviours:
> 
> 	PixelFormat format;
> 	uint32_t fourcc;
> 
> 	if (format == fourcc) { ... } /* Compiles */
> 	if (fourcc == format) { ... } /* Doesn't compile */
> 
> Unless we have occurrences of the second in the code, I would make the
> operators member functions in patch 6/8 to avoid the churn.

I know, and unfortunately we do, in the rkisp1 and vimc pipeline 
handlers :-(

But they really highlight an issue for s/unsigned int/PixelFormat/ which 
can be made earlier in the series and the comparison operators can be 
folded into the patch that makes a class out of PixelFormat :-)

> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  include/libcamera/pixelformats.h         | 10 ++---
> >  src/gstreamer/gstlibcamera-utils.cpp     |  4 +-
> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |  6 +--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 20 ++++-----
> >  src/libcamera/pipeline/vimc.cpp          | 12 ++---
> >  src/libcamera/pixelformats.cpp           | 57 ++++++++++++------------
> >  src/libcamera/v4l2_videodevice.cpp       | 26 +++++------
> >  src/v4l2/v4l2_camera_proxy.cpp           | 28 ++++++------
> >  test/stream/stream_formats.cpp           | 24 +++++-----
> >  test/v4l2_videodevice/buffer_cache.cpp   |  2 +-
> >  10 files changed, 94 insertions(+), 95 deletions(-)
> > 
> > diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> > index 5ba1ba1b324272b9..3285941382ccdd96 100644
> > --- a/include/libcamera/pixelformats.h
> > +++ b/include/libcamera/pixelformats.h
> > @@ -17,7 +17,11 @@ class PixelFormat
> >  {
> >  public:
> >  	PixelFormat();
> > -	PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> > +	explicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> > +
> > +	bool operator==(const PixelFormat &other) const;
> > +	bool operator!=(const PixelFormat &other) const;
> > +	bool operator<(const PixelFormat &other) const;
> >  
> >  	uint32_t fourcc() const { return fourcc_; }
> >  	const std::set<uint64_t> &modifiers() const { return modifiers_; }
> > @@ -28,10 +32,6 @@ private:
> >  	std::set<uint64_t> modifiers_;
> >  };
> >  
> > -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);
> > -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);
> > -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);
> > -
> >  } /* namespace libcamera */
> >  
> >  #endif /* __LIBCAMERA_PIXEL_FORMATS_H__ */
> > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > index f21e94c3eef92737..c13b0ca245386168 100644
> > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > @@ -154,9 +154,9 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> >  	if (gst_structure_has_name(s, "video/x-raw")) {
> >  		const gchar *format = gst_structure_get_string(s, "format");
> >  		gst_format = gst_video_format_from_string(format);
> > -		stream_cfg.pixelFormat = gst_format_to_drm(gst_format);
> > +		stream_cfg.pixelFormat = PixelFormat(gst_format_to_drm(gst_format));
> >  	} else if (gst_structure_has_name(s, "image/jpeg")) {
> > -		stream_cfg.pixelFormat = DRM_FORMAT_MJPEG;
> > +		stream_cfg.pixelFormat = PixelFormat(DRM_FORMAT_MJPEG);
> >  	} else {
> >  		g_critical("Unsupported media type: %s", gst_structure_get_name(s));
> >  	}
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 0c2a217c9ea8f6ba..52b6d48aca4394c6 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -246,7 +246,7 @@ IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
> >  void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
> >  {
> >  	/* The only pixel format the driver supports is NV12. */
> > -	cfg.pixelFormat = DRM_FORMAT_NV12;
> > +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> >  
> >  	if (scale) {
> >  		/*
> > @@ -401,7 +401,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  		StreamConfiguration cfg = {};
> >  		IPU3Stream *stream = nullptr;
> >  
> > -		cfg.pixelFormat = DRM_FORMAT_NV12;
> > +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> >  
> >  		switch (role) {
> >  		case StreamRole::StillCapture:
> > @@ -1079,7 +1079,7 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> >  		return 0;
> >  
> >  	V4L2DeviceFormat outputFormat = {};
> > -	outputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);
> > +	outputFormat.fourcc = dev->toV4L2Fourcc(PixelFormat(DRM_FORMAT_NV12));
> >  	outputFormat.size = cfg.size;
> >  	outputFormat.planesCount = 2;
> >  
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 8223b82c4a9c773c..3bbe73c3abd9d75e 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -431,14 +431,14 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> >  
> >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  {
> > -	static const std::array<unsigned int, 8> formats{
> > -		DRM_FORMAT_YUYV,
> > -		DRM_FORMAT_YVYU,
> > -		DRM_FORMAT_VYUY,
> > -		DRM_FORMAT_NV16,
> > -		DRM_FORMAT_NV61,
> > -		DRM_FORMAT_NV21,
> > -		DRM_FORMAT_NV12,
> > +	static const std::array<PixelFormat, 8> formats{
> > +		PixelFormat(DRM_FORMAT_YUYV),
> > +		PixelFormat(DRM_FORMAT_YVYU),
> > +		PixelFormat(DRM_FORMAT_VYUY),
> > +		PixelFormat(DRM_FORMAT_NV16),
> > +		PixelFormat(DRM_FORMAT_NV61),
> > +		PixelFormat(DRM_FORMAT_NV21),
> > +		PixelFormat(DRM_FORMAT_NV12),
> >  		/* \todo Add support for 8-bit greyscale to DRM formats */
> >  	};
> >  
> > @@ -460,7 +460,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> >  	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
> >  	    formats.end()) {
> >  		LOG(RkISP1, Debug) << "Adjusting format to NV12";
> > -		cfg.pixelFormat = DRM_FORMAT_NV12,
> > +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),
> >  		status = Adjusted;
> >  	}
> >  
> > @@ -539,7 +539,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> >  		return config;
> >  
> >  	StreamConfiguration cfg{};
> > -	cfg.pixelFormat = DRM_FORMAT_NV12;
> > +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> >  	cfg.size = data->sensor_->resolution();
> >  
> >  	config->addConfiguration(cfg);
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 72924bf2f55d0021..8dad2b40ed4fbb2c 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -104,10 +104,10 @@ private:
> >  
> >  namespace {
> >  
> > -constexpr std::array<unsigned int, 3> pixelformats{
> > -	DRM_FORMAT_RGB888,
> > -	DRM_FORMAT_BGR888,
> > -	DRM_FORMAT_BGRA8888,
> > +const std::array<PixelFormat, 3> pixelformats{
> > +	PixelFormat(DRM_FORMAT_RGB888),
> > +	PixelFormat(DRM_FORMAT_BGR888),
> > +	PixelFormat(DRM_FORMAT_BGRA8888),
> >  };
> >  
> >  } /* namespace */
> > @@ -136,7 +136,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> >  	if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==
> >  	    pixelformats.end()) {
> >  		LOG(VIMC, Debug) << "Adjusting format to RGB24";
> > -		cfg.pixelFormat = DRM_FORMAT_BGR888;
> > +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
> >  		status = Adjusted;
> >  	}
> >  
> > @@ -185,7 +185,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> >  
> >  	StreamConfiguration cfg(formats);
> >  
> > -	cfg.pixelFormat = DRM_FORMAT_BGR888;
> > +	cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
> >  	cfg.size = { 1920, 1080 };
> >  	cfg.bufferCount = 4;
> >  
> > diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> > index fe9a6a2576978647..1c559fe46d406826 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -41,6 +41,34 @@ PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
> >  {
> >  }
> >  
> > +/**
> > + * \brief Compare pixel formats for equality
> > + * \return True if the two pixel formats are equal, false otherwise
> > + */
> > +bool PixelFormat::operator==(const PixelFormat &other) const
> > +{
> > +	return fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;
> > +}
> > +
> > +/**
> > + * \brief Compare pixel formats for inequality
> > + * \return True if the two pixel formats are not equal, false otherwise
> > + */
> > +bool PixelFormat::operator!=(const PixelFormat &other) const
> > +{
> > +	return !(*this == other);
> > +}
> > +
> > +/**
> > + * \brief Compare pixel formats for smaller than order
> > + * \todo Take modifiers into account if FourCC are equal
> > + * \return True if \a this is smaller than \a other, false otherwise
> > + */
> > +bool PixelFormat::operator<(const PixelFormat &other) const
> > +{
> > +	return fourcc_ < other.fourcc_;
> > +}
> > +
> >  /**
> >   * \fn PixelFormat::fourcc() const
> >   * \brief Retrieve the pixel format FourCC
> > @@ -64,33 +92,4 @@ std::string PixelFormat::toString() const
> >  	return str;
> >  }
> >  
> > -/**
> > - * \brief Compare pixel formats for equality
> > - * \return True if the two pixel formats are equal, false otherwise
> > - */
> > -bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)
> > -{
> > -	return lhs.fourcc() == rhs.fourcc() &&
> > -	       lhs.modifiers() == rhs.modifiers();
> > -}
> > -
> > -/**
> > - * \brief Compare pixel formats for inequality
> > - * \return True if the two pixel formats are not equal, false otherwise
> > - */
> > -bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)
> > -{
> > -	return !(lhs == rhs);
> > -}
> > -
> > -/**
> > - * \brief Compare pixel formats for smaller than order
> > - * \todo Take modifiers into account if \a lhs == \a rhs.
> > - * \return True if \a lhs is smaller than \a rhs, false otherwise
> > - */
> > -bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)
> > -{
> > -	return lhs.fourcc() < rhs.fourcc();
> > -}
> > -
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index b5762a7eabcf4e25..d8d711a951d666e9 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1563,39 +1563,39 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
> >  	switch (v4l2Fourcc) {
> >  	/* RGB formats. */
> >  	case V4L2_PIX_FMT_RGB24:
> > -		return DRM_FORMAT_BGR888;
> > +		return PixelFormat(DRM_FORMAT_BGR888);
> >  	case V4L2_PIX_FMT_BGR24:
> > -		return DRM_FORMAT_RGB888;
> > +		return PixelFormat(DRM_FORMAT_RGB888);
> >  	case V4L2_PIX_FMT_ARGB32:
> > -		return DRM_FORMAT_BGRA8888;
> > +		return PixelFormat(DRM_FORMAT_BGRA8888);
> >  
> >  	/* YUV packed formats. */
> >  	case V4L2_PIX_FMT_YUYV:
> > -		return DRM_FORMAT_YUYV;
> > +		return PixelFormat(DRM_FORMAT_YUYV);
> >  	case V4L2_PIX_FMT_YVYU:
> > -		return DRM_FORMAT_YVYU;
> > +		return PixelFormat(DRM_FORMAT_YVYU);
> >  	case V4L2_PIX_FMT_UYVY:
> > -		return DRM_FORMAT_UYVY;
> > +		return PixelFormat(DRM_FORMAT_UYVY);
> >  	case V4L2_PIX_FMT_VYUY:
> > -		return DRM_FORMAT_VYUY;
> > +		return PixelFormat(DRM_FORMAT_VYUY);
> >  
> >  	/* YUY planar formats. */
> >  	case V4L2_PIX_FMT_NV16:
> >  	case V4L2_PIX_FMT_NV16M:
> > -		return DRM_FORMAT_NV16;
> > +		return PixelFormat(DRM_FORMAT_NV16);
> >  	case V4L2_PIX_FMT_NV61:
> >  	case V4L2_PIX_FMT_NV61M:
> > -		return DRM_FORMAT_NV61;
> > +		return PixelFormat(DRM_FORMAT_NV61);
> >  	case V4L2_PIX_FMT_NV12:
> >  	case V4L2_PIX_FMT_NV12M:
> > -		return DRM_FORMAT_NV12;
> > +		return PixelFormat(DRM_FORMAT_NV12);
> >  	case V4L2_PIX_FMT_NV21:
> >  	case V4L2_PIX_FMT_NV21M:
> > -		return DRM_FORMAT_NV21;
> > +		return PixelFormat(DRM_FORMAT_NV21);
> >  
> >  	/* Compressed formats. */
> >  	case V4L2_PIX_FMT_MJPEG:
> > -		return DRM_FORMAT_MJPEG;
> > +		return PixelFormat(DRM_FORMAT_MJPEG);
> >  
> >  	/* V4L2 formats not yet supported by DRM. */
> >  	case V4L2_PIX_FMT_GREY:
> > @@ -1608,7 +1608,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
> >  				LogError).stream()
> >  			<< "Unsupported V4L2 pixel format "
> >  			<< utils::hex(v4l2Fourcc);
> > -		return 0;
> > +		return PixelFormat();
> >  	}
> >  }
> >  
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 3bbbbf79cdb475db..55dd69d37bd65897 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -536,21 +536,21 @@ namespace {
> >  
> >  static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
> >  	/* RGB formats. */
> > -	{ DRM_FORMAT_RGB888,	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_BGR888,	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_BGRA8888,	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_RGB888),	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_BGR888),	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_BGRA8888),	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> >  	/* YUV packed formats. */
> > -	{ DRM_FORMAT_UYVY,	V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_VYUY,	V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_YUYV,	V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_YVYU,	V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_UYVY),		V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_VYUY),		V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_YUYV),		V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_YVYU),		V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
> >  	/* YUY planar formats. */
> > -	{ DRM_FORMAT_NV12,	V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_NV21,	V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_NV16,	V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_NV61,	V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_NV24,	V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > -	{ DRM_FORMAT_NV42,	V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV12),		V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV21),		V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV16),		V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV61),		V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV24),		V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
> > +	{ PixelFormat(DRM_FORMAT_NV42),		V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
> >  }};
> >  
> >  } /* namespace */
> > @@ -594,7 +594,7 @@ PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)
> >  					 return info.v4l2Format == format;
> >  				 });
> >  	if (info == pixelFormatInfo.end())
> > -		return format;
> > +		return PixelFormat();
> >  
> >  	return info->format;
> >  }
> > diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp
> > index a391f5cd087d3872..92f1574b8a0b315c 100644
> > --- a/test/stream/stream_formats.cpp
> > +++ b/test/stream/stream_formats.cpp
> > @@ -55,40 +55,40 @@ protected:
> >  	{
> >  		/* Test discrete sizes */
> >  		StreamFormats discrete({
> > -			{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },
> > -			{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },
> > +			{ PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },
> > +			{ PixelFormat(2), { SizeRange(300, 300), SizeRange(400, 400) } },
> >  		});
> >  
> > -		if (testSizes("discrete 1", discrete.sizes(1),
> > +		if (testSizes("discrete 1", discrete.sizes(PixelFormat(1)),
> >  			      { Size(100, 100), Size(200, 200) }))
> >  			return TestFail;
> > -		if (testSizes("discrete 2", discrete.sizes(2),
> > +		if (testSizes("discrete 2", discrete.sizes(PixelFormat(2)),
> >  			      { Size(300, 300), Size(400, 400) }))
> >  			return TestFail;
> >  
> >  		/* Test range sizes */
> >  		StreamFormats range({
> > -			{ 1, { SizeRange(640, 480, 640, 480) } },
> > -			{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },
> > -			{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },
> > -			{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },
> > +			{ PixelFormat(1), { SizeRange(640, 480, 640, 480) } },
> > +			{ PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },
> > +			{ PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },
> > +			{ PixelFormat(4), { SizeRange(128, 128, 4096, 4096, 128, 128) } },
> >  		});
> >  
> > -		if (testSizes("range 1", range.sizes(1), { Size(640, 480) }))
> > +		if (testSizes("range 1", range.sizes(PixelFormat(1)), { Size(640, 480) }))
> >  			return TestFail;
> >  
> > -		if (testSizes("range 2", range.sizes(2), {
> > +		if (testSizes("range 2", range.sizes(PixelFormat(2)), {
> >  			      Size(640, 480), Size(720, 480),
> >  			      Size(720, 576), Size(768, 480),
> >  			      Size(800, 600) }))
> >  			return TestFail;
> >  
> > -		if (testSizes("range 3", range.sizes(3), {
> > +		if (testSizes("range 3", range.sizes(PixelFormat(3)), {
> >  			      Size(640, 480), Size(720, 480),
> >  			      Size(720, 576), Size(768, 480) }))
> >  			return TestFail;
> >  
> > -		if (testSizes("range 4", range.sizes(4), {
> > +		if (testSizes("range 4", range.sizes(PixelFormat(4)), {
> >  			      Size(1024, 768), Size(1280, 1024),
> >  			      Size(2048, 1152), Size(2048, 1536),
> >  			      Size(2560, 2048), Size(3200, 2048), }))
> > diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
> > index c951bc9650dc4e0e..8921605030cfdefb 100644
> > --- a/test/v4l2_videodevice/buffer_cache.cpp
> > +++ b/test/v4l2_videodevice/buffer_cache.cpp
> > @@ -144,7 +144,7 @@ public:
> >  		const unsigned int numBuffers = 8;
> >  
> >  		StreamConfiguration cfg;
> > -		cfg.pixelFormat = DRM_FORMAT_YUYV;
> > +		cfg.pixelFormat = PixelFormat(DRM_FORMAT_YUYV);
> >  		cfg.size = Size(600, 800);
> >  		cfg.bufferCount = numBuffers;
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
index 5ba1ba1b324272b9..3285941382ccdd96 100644
--- a/include/libcamera/pixelformats.h
+++ b/include/libcamera/pixelformats.h
@@ -17,7 +17,11 @@  class PixelFormat
 {
 public:
 	PixelFormat();
-	PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
+	explicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
+
+	bool operator==(const PixelFormat &other) const;
+	bool operator!=(const PixelFormat &other) const;
+	bool operator<(const PixelFormat &other) const;
 
 	uint32_t fourcc() const { return fourcc_; }
 	const std::set<uint64_t> &modifiers() const { return modifiers_; }
@@ -28,10 +32,6 @@  private:
 	std::set<uint64_t> modifiers_;
 };
 
-bool operator==(const PixelFormat &lhs, const PixelFormat &rhs);
-bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs);
-bool operator<(const PixelFormat &lhs, const PixelFormat &rhs);
-
 } /* namespace libcamera */
 
 #endif /* __LIBCAMERA_PIXEL_FORMATS_H__ */
diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
index f21e94c3eef92737..c13b0ca245386168 100644
--- a/src/gstreamer/gstlibcamera-utils.cpp
+++ b/src/gstreamer/gstlibcamera-utils.cpp
@@ -154,9 +154,9 @@  gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
 	if (gst_structure_has_name(s, "video/x-raw")) {
 		const gchar *format = gst_structure_get_string(s, "format");
 		gst_format = gst_video_format_from_string(format);
-		stream_cfg.pixelFormat = gst_format_to_drm(gst_format);
+		stream_cfg.pixelFormat = PixelFormat(gst_format_to_drm(gst_format));
 	} else if (gst_structure_has_name(s, "image/jpeg")) {
-		stream_cfg.pixelFormat = DRM_FORMAT_MJPEG;
+		stream_cfg.pixelFormat = PixelFormat(DRM_FORMAT_MJPEG);
 	} else {
 		g_critical("Unsupported media type: %s", gst_structure_get_name(s));
 	}
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 0c2a217c9ea8f6ba..52b6d48aca4394c6 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -246,7 +246,7 @@  IPU3CameraConfiguration::IPU3CameraConfiguration(Camera *camera,
 void IPU3CameraConfiguration::adjustStream(StreamConfiguration &cfg, bool scale)
 {
 	/* The only pixel format the driver supports is NV12. */
-	cfg.pixelFormat = DRM_FORMAT_NV12;
+	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
 
 	if (scale) {
 		/*
@@ -401,7 +401,7 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 		StreamConfiguration cfg = {};
 		IPU3Stream *stream = nullptr;
 
-		cfg.pixelFormat = DRM_FORMAT_NV12;
+		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
 
 		switch (role) {
 		case StreamRole::StillCapture:
@@ -1079,7 +1079,7 @@  int ImgUDevice::configureOutput(ImgUOutput *output,
 		return 0;
 
 	V4L2DeviceFormat outputFormat = {};
-	outputFormat.fourcc = dev->toV4L2Fourcc(DRM_FORMAT_NV12);
+	outputFormat.fourcc = dev->toV4L2Fourcc(PixelFormat(DRM_FORMAT_NV12));
 	outputFormat.size = cfg.size;
 	outputFormat.planesCount = 2;
 
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 8223b82c4a9c773c..3bbe73c3abd9d75e 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -431,14 +431,14 @@  RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
 
 CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 {
-	static const std::array<unsigned int, 8> formats{
-		DRM_FORMAT_YUYV,
-		DRM_FORMAT_YVYU,
-		DRM_FORMAT_VYUY,
-		DRM_FORMAT_NV16,
-		DRM_FORMAT_NV61,
-		DRM_FORMAT_NV21,
-		DRM_FORMAT_NV12,
+	static const std::array<PixelFormat, 8> formats{
+		PixelFormat(DRM_FORMAT_YUYV),
+		PixelFormat(DRM_FORMAT_YVYU),
+		PixelFormat(DRM_FORMAT_VYUY),
+		PixelFormat(DRM_FORMAT_NV16),
+		PixelFormat(DRM_FORMAT_NV61),
+		PixelFormat(DRM_FORMAT_NV21),
+		PixelFormat(DRM_FORMAT_NV12),
 		/* \todo Add support for 8-bit greyscale to DRM formats */
 	};
 
@@ -460,7 +460,7 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 	if (std::find(formats.begin(), formats.end(), cfg.pixelFormat) ==
 	    formats.end()) {
 		LOG(RkISP1, Debug) << "Adjusting format to NV12";
-		cfg.pixelFormat = DRM_FORMAT_NV12,
+		cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12),
 		status = Adjusted;
 	}
 
@@ -539,7 +539,7 @@  CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
 		return config;
 
 	StreamConfiguration cfg{};
-	cfg.pixelFormat = DRM_FORMAT_NV12;
+	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
 	cfg.size = data->sensor_->resolution();
 
 	config->addConfiguration(cfg);
diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
index 72924bf2f55d0021..8dad2b40ed4fbb2c 100644
--- a/src/libcamera/pipeline/vimc.cpp
+++ b/src/libcamera/pipeline/vimc.cpp
@@ -104,10 +104,10 @@  private:
 
 namespace {
 
-constexpr std::array<unsigned int, 3> pixelformats{
-	DRM_FORMAT_RGB888,
-	DRM_FORMAT_BGR888,
-	DRM_FORMAT_BGRA8888,
+const std::array<PixelFormat, 3> pixelformats{
+	PixelFormat(DRM_FORMAT_RGB888),
+	PixelFormat(DRM_FORMAT_BGR888),
+	PixelFormat(DRM_FORMAT_BGRA8888),
 };
 
 } /* namespace */
@@ -136,7 +136,7 @@  CameraConfiguration::Status VimcCameraConfiguration::validate()
 	if (std::find(pixelformats.begin(), pixelformats.end(), cfg.pixelFormat) ==
 	    pixelformats.end()) {
 		LOG(VIMC, Debug) << "Adjusting format to RGB24";
-		cfg.pixelFormat = DRM_FORMAT_BGR888;
+		cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
 		status = Adjusted;
 	}
 
@@ -185,7 +185,7 @@  CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
 
 	StreamConfiguration cfg(formats);
 
-	cfg.pixelFormat = DRM_FORMAT_BGR888;
+	cfg.pixelFormat = PixelFormat(DRM_FORMAT_BGR888);
 	cfg.size = { 1920, 1080 };
 	cfg.bufferCount = 4;
 
diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
index fe9a6a2576978647..1c559fe46d406826 100644
--- a/src/libcamera/pixelformats.cpp
+++ b/src/libcamera/pixelformats.cpp
@@ -41,6 +41,34 @@  PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
 {
 }
 
+/**
+ * \brief Compare pixel formats for equality
+ * \return True if the two pixel formats are equal, false otherwise
+ */
+bool PixelFormat::operator==(const PixelFormat &other) const
+{
+	return fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;
+}
+
+/**
+ * \brief Compare pixel formats for inequality
+ * \return True if the two pixel formats are not equal, false otherwise
+ */
+bool PixelFormat::operator!=(const PixelFormat &other) const
+{
+	return !(*this == other);
+}
+
+/**
+ * \brief Compare pixel formats for smaller than order
+ * \todo Take modifiers into account if FourCC are equal
+ * \return True if \a this is smaller than \a other, false otherwise
+ */
+bool PixelFormat::operator<(const PixelFormat &other) const
+{
+	return fourcc_ < other.fourcc_;
+}
+
 /**
  * \fn PixelFormat::fourcc() const
  * \brief Retrieve the pixel format FourCC
@@ -64,33 +92,4 @@  std::string PixelFormat::toString() const
 	return str;
 }
 
-/**
- * \brief Compare pixel formats for equality
- * \return True if the two pixel formats are equal, false otherwise
- */
-bool operator==(const PixelFormat &lhs, const PixelFormat &rhs)
-{
-	return lhs.fourcc() == rhs.fourcc() &&
-	       lhs.modifiers() == rhs.modifiers();
-}
-
-/**
- * \brief Compare pixel formats for inequality
- * \return True if the two pixel formats are not equal, false otherwise
- */
-bool operator!=(const PixelFormat &lhs, const PixelFormat &rhs)
-{
-	return !(lhs == rhs);
-}
-
-/**
- * \brief Compare pixel formats for smaller than order
- * \todo Take modifiers into account if \a lhs == \a rhs.
- * \return True if \a lhs is smaller than \a rhs, false otherwise
- */
-bool operator<(const PixelFormat &lhs, const PixelFormat &rhs)
-{
-	return lhs.fourcc() < rhs.fourcc();
-}
-
 } /* namespace libcamera */
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index b5762a7eabcf4e25..d8d711a951d666e9 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1563,39 +1563,39 @@  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
 	switch (v4l2Fourcc) {
 	/* RGB formats. */
 	case V4L2_PIX_FMT_RGB24:
-		return DRM_FORMAT_BGR888;
+		return PixelFormat(DRM_FORMAT_BGR888);
 	case V4L2_PIX_FMT_BGR24:
-		return DRM_FORMAT_RGB888;
+		return PixelFormat(DRM_FORMAT_RGB888);
 	case V4L2_PIX_FMT_ARGB32:
-		return DRM_FORMAT_BGRA8888;
+		return PixelFormat(DRM_FORMAT_BGRA8888);
 
 	/* YUV packed formats. */
 	case V4L2_PIX_FMT_YUYV:
-		return DRM_FORMAT_YUYV;
+		return PixelFormat(DRM_FORMAT_YUYV);
 	case V4L2_PIX_FMT_YVYU:
-		return DRM_FORMAT_YVYU;
+		return PixelFormat(DRM_FORMAT_YVYU);
 	case V4L2_PIX_FMT_UYVY:
-		return DRM_FORMAT_UYVY;
+		return PixelFormat(DRM_FORMAT_UYVY);
 	case V4L2_PIX_FMT_VYUY:
-		return DRM_FORMAT_VYUY;
+		return PixelFormat(DRM_FORMAT_VYUY);
 
 	/* YUY planar formats. */
 	case V4L2_PIX_FMT_NV16:
 	case V4L2_PIX_FMT_NV16M:
-		return DRM_FORMAT_NV16;
+		return PixelFormat(DRM_FORMAT_NV16);
 	case V4L2_PIX_FMT_NV61:
 	case V4L2_PIX_FMT_NV61M:
-		return DRM_FORMAT_NV61;
+		return PixelFormat(DRM_FORMAT_NV61);
 	case V4L2_PIX_FMT_NV12:
 	case V4L2_PIX_FMT_NV12M:
-		return DRM_FORMAT_NV12;
+		return PixelFormat(DRM_FORMAT_NV12);
 	case V4L2_PIX_FMT_NV21:
 	case V4L2_PIX_FMT_NV21M:
-		return DRM_FORMAT_NV21;
+		return PixelFormat(DRM_FORMAT_NV21);
 
 	/* Compressed formats. */
 	case V4L2_PIX_FMT_MJPEG:
-		return DRM_FORMAT_MJPEG;
+		return PixelFormat(DRM_FORMAT_MJPEG);
 
 	/* V4L2 formats not yet supported by DRM. */
 	case V4L2_PIX_FMT_GREY:
@@ -1608,7 +1608,7 @@  PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc)
 				LogError).stream()
 			<< "Unsupported V4L2 pixel format "
 			<< utils::hex(v4l2Fourcc);
-		return 0;
+		return PixelFormat();
 	}
 }
 
diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
index 3bbbbf79cdb475db..55dd69d37bd65897 100644
--- a/src/v4l2/v4l2_camera_proxy.cpp
+++ b/src/v4l2/v4l2_camera_proxy.cpp
@@ -536,21 +536,21 @@  namespace {
 
 static const std::array<PixelFormatInfo, 13> pixelFormatInfo = {{
 	/* RGB formats. */
-	{ DRM_FORMAT_RGB888,	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_BGR888,	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_BGRA8888,	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_RGB888),	V4L2_PIX_FMT_BGR24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_BGR888),	V4L2_PIX_FMT_RGB24,	1, {{ { 24, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_BGRA8888),	V4L2_PIX_FMT_ARGB32,	1, {{ { 32, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
 	/* YUV packed formats. */
-	{ DRM_FORMAT_UYVY,	V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_VYUY,	V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_YUYV,	V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_YVYU,	V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_UYVY),		V4L2_PIX_FMT_UYVY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_VYUY),		V4L2_PIX_FMT_VYUY,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_YUYV),		V4L2_PIX_FMT_YUYV,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_YVYU),		V4L2_PIX_FMT_YVYU,	1, {{ { 16, 1, 1 }, {  0, 0, 0 }, {  0, 0, 0 } }} },
 	/* YUY planar formats. */
-	{ DRM_FORMAT_NV12,	V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_NV21,	V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_NV16,	V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_NV61,	V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_NV24,	V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
-	{ DRM_FORMAT_NV42,	V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV12),		V4L2_PIX_FMT_NV12,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV21),		V4L2_PIX_FMT_NV21,	2, {{ {  8, 1, 1 }, { 16, 2, 2 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV16),		V4L2_PIX_FMT_NV16,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV61),		V4L2_PIX_FMT_NV61,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV24),		V4L2_PIX_FMT_NV24,	2, {{ {  8, 1, 1 }, { 16, 2, 1 }, {  0, 0, 0 } }} },
+	{ PixelFormat(DRM_FORMAT_NV42),		V4L2_PIX_FMT_NV42,	2, {{ {  8, 1, 1 }, { 16, 1, 1 }, {  0, 0, 0 } }} },
 }};
 
 } /* namespace */
@@ -594,7 +594,7 @@  PixelFormat V4L2CameraProxy::v4l2ToDrm(uint32_t format)
 					 return info.v4l2Format == format;
 				 });
 	if (info == pixelFormatInfo.end())
-		return format;
+		return PixelFormat();
 
 	return info->format;
 }
diff --git a/test/stream/stream_formats.cpp b/test/stream/stream_formats.cpp
index a391f5cd087d3872..92f1574b8a0b315c 100644
--- a/test/stream/stream_formats.cpp
+++ b/test/stream/stream_formats.cpp
@@ -55,40 +55,40 @@  protected:
 	{
 		/* Test discrete sizes */
 		StreamFormats discrete({
-			{ 1, { SizeRange(100, 100), SizeRange(200, 200) } },
-			{ 2, { SizeRange(300, 300), SizeRange(400, 400) } },
+			{ PixelFormat(1), { SizeRange(100, 100), SizeRange(200, 200) } },
+			{ PixelFormat(2), { SizeRange(300, 300), SizeRange(400, 400) } },
 		});
 
-		if (testSizes("discrete 1", discrete.sizes(1),
+		if (testSizes("discrete 1", discrete.sizes(PixelFormat(1)),
 			      { Size(100, 100), Size(200, 200) }))
 			return TestFail;
-		if (testSizes("discrete 2", discrete.sizes(2),
+		if (testSizes("discrete 2", discrete.sizes(PixelFormat(2)),
 			      { Size(300, 300), Size(400, 400) }))
 			return TestFail;
 
 		/* Test range sizes */
 		StreamFormats range({
-			{ 1, { SizeRange(640, 480, 640, 480) } },
-			{ 2, { SizeRange(640, 480, 800, 600, 8, 8) } },
-			{ 3, { SizeRange(640, 480, 800, 600, 16, 16) } },
-			{ 4, { SizeRange(128, 128, 4096, 4096, 128, 128) } },
+			{ PixelFormat(1), { SizeRange(640, 480, 640, 480) } },
+			{ PixelFormat(2), { SizeRange(640, 480, 800, 600, 8, 8) } },
+			{ PixelFormat(3), { SizeRange(640, 480, 800, 600, 16, 16) } },
+			{ PixelFormat(4), { SizeRange(128, 128, 4096, 4096, 128, 128) } },
 		});
 
-		if (testSizes("range 1", range.sizes(1), { Size(640, 480) }))
+		if (testSizes("range 1", range.sizes(PixelFormat(1)), { Size(640, 480) }))
 			return TestFail;
 
-		if (testSizes("range 2", range.sizes(2), {
+		if (testSizes("range 2", range.sizes(PixelFormat(2)), {
 			      Size(640, 480), Size(720, 480),
 			      Size(720, 576), Size(768, 480),
 			      Size(800, 600) }))
 			return TestFail;
 
-		if (testSizes("range 3", range.sizes(3), {
+		if (testSizes("range 3", range.sizes(PixelFormat(3)), {
 			      Size(640, 480), Size(720, 480),
 			      Size(720, 576), Size(768, 480) }))
 			return TestFail;
 
-		if (testSizes("range 4", range.sizes(4), {
+		if (testSizes("range 4", range.sizes(PixelFormat(4)), {
 			      Size(1024, 768), Size(1280, 1024),
 			      Size(2048, 1152), Size(2048, 1536),
 			      Size(2560, 2048), Size(3200, 2048), }))
diff --git a/test/v4l2_videodevice/buffer_cache.cpp b/test/v4l2_videodevice/buffer_cache.cpp
index c951bc9650dc4e0e..8921605030cfdefb 100644
--- a/test/v4l2_videodevice/buffer_cache.cpp
+++ b/test/v4l2_videodevice/buffer_cache.cpp
@@ -144,7 +144,7 @@  public:
 		const unsigned int numBuffers = 8;
 
 		StreamConfiguration cfg;
-		cfg.pixelFormat = DRM_FORMAT_YUYV;
+		cfg.pixelFormat = PixelFormat(DRM_FORMAT_YUYV);
 		cfg.size = Size(600, 800);
 		cfg.bufferCount = numBuffers;