[libcamera-devel] [PATCH] libcamera: pixelformats: replace set of modifiers with a single value

[Kieran Bingham]

Hi Kaaira,

On 03/04/2020 15:12, Kaaira Gupta wrote:
> Pixelformat class takes a set of modifiers as an input, but all the
> values in the set are same. Hence take just one value as an input.
> 

It might be worth expanding upon what we have learned here about the
fact that DRM fourccs 'look' like they have a per-plane modifier, but in
fact each one must be the same (and thus there is only one modifer for a
DRM fourcc).


I can only see a small comment reformatting topic below, and other than
that, it compiles cleanly, passes checkstyle.py, and the tests still run
(though we didn't have any tests that use modifiers :-D)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>  include/libcamera/pixelformats.h     |  6 +++---
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
>  src/libcamera/pixelformats.cpp       | 22 +++++++++++-----------
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/include/libcamera/pixelformats.h b/include/libcamera/pixelformats.h
> index 9ce6f7f..89966e5 100644
> --- a/include/libcamera/pixelformats.h
> +++ b/include/libcamera/pixelformats.h
> @@ -19,7 +19,7 @@ class PixelFormat
>  {
>  public:
>  	PixelFormat();
> -	explicit PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers = {});
> +	explicit PixelFormat(uint32_t fourcc, uint64_t modifier = 0);
>  
>  	bool operator==(const PixelFormat &other) const;
>  	bool operator!=(const PixelFormat &other) const { return !(*this == other); }
> @@ -29,13 +29,13 @@ public:
>  
>  	operator uint32_t() const { return fourcc_; }
>  	uint32_t fourcc() const { return fourcc_; }
> -	const std::set<uint64_t> &modifiers() const { return modifiers_; }
> +	uint64_t modifier() const { return modifier_; }
>  
>  	std::string toString() const;
>  
>  private:
>  	uint32_t fourcc_;
> -	std::set<uint64_t> modifiers_;
> +	uint64_t modifier_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 1e114ca..219b90b 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -365,7 +365,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const Size size = cfg.size;
>  		const IPU3Stream *stream;
>  
> -		if (cfg.pixelFormat.modifiers().count(IPU3_FORMAT_MOD_PACKED))
> +		if (cfg.pixelFormat.modifier() == IPU3_FORMAT_MOD_PACKED)
>  			stream = &data_->rawStream_;
>  		else if (cfg.size == sensorFormat_.size)
>  			stream = &data_->outStream_;
> diff --git a/src/libcamera/pixelformats.cpp b/src/libcamera/pixelformats.cpp
> index 87557d9..a53d435 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -19,8 +19,8 @@ namespace libcamera {
>   * \brief libcamera image pixel format
>   *
>   * The PixelFormat type describes the format of images in the public libcamera
> - * API. It stores a FourCC value as a 32-bit unsigned integer and a set of
> - * modifiers. The FourCC and modifiers values are defined in the Linux kernel
> + * API. It stores a FourCC value as a 32-bit unsigned integer and a
> + * modifier. The FourCC and modifier values are defined in the Linux kernel

This can be reworked to utilise 80chars per line. (the word 'modifier'
can move up a line :-D)

>   * DRM/KMS API (see linux/drm_fourcc.h).
>   */
>  
> @@ -36,12 +36,12 @@ PixelFormat::PixelFormat()
>  }
>  
>  /**
> - * \brief Construct a PixelFormat from a DRM FourCC and a set of modifiers
> + * \brief Construct a PixelFormat from a DRM FourCC and a modifier
>   * \param[in] fourcc A DRM FourCC
> - * \param[in] modifiers A set of DRM FourCC modifiers
> + * \param[in] modifier A DRM FourCC modifier
>   */
> -PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
> -	: fourcc_(fourcc), modifiers_(modifiers)
> +PixelFormat::PixelFormat(uint32_t fourcc, uint64_t modifier)
> +	: fourcc_(fourcc), modifier_(modifier)
>  {
>  }
>  
> @@ -51,7 +51,7 @@ PixelFormat::PixelFormat(uint32_t fourcc, const std::set<uint64_t> &modifiers)
>   */
>  bool PixelFormat::operator==(const PixelFormat &other) const
>  {
> -	return fourcc_ == other.fourcc() && modifiers_ == other.modifiers_;
> +	return fourcc_ == other.fourcc() && modifier_ == other.modifier_;
>  }
>  
>  /**
> @@ -70,7 +70,7 @@ bool PixelFormat::operator<(const PixelFormat &other) const
>  		return true;
>  	if (fourcc_ > other.fourcc_)
>  		return false;
> -	return modifiers_ < modifiers_;
> +	return modifier_ < other.modifier_;
>  }
>  
>  /**
> @@ -97,9 +97,9 @@ bool PixelFormat::operator<(const PixelFormat &other) const
>   */
>  
>  /**
> - * \fn PixelFormat::modifiers() const
> - * \brief Retrieve the pixel format modifiers
> - * \return Set of DRM modifiers
> + * \fn PixelFormat::modifier() const
> + * \brief Retrieve the pixel format modifier
> + * \return DRM modifier
>   */
>  
>  /**
> 

-- 
Regards
--
Kieran