[libcamera-devel,v2] libcamera: pixelformats: Replace set of modifiers with single value
diff mbox series

Message ID 20200403160623.GA6354@kaaira-HP-Pavilion-Notebook
State Accepted
Headers show
Series
  • [libcamera-devel,v2] libcamera: pixelformats: Replace set of modifiers with single value
Related show

Commit Message

Kaaira Gupta April 3, 2020, 4:06 p.m. UTC 
DRM fourccs look like they have a per-plane modifier, but in fact each
of them should be same. Hence instead of passing a set of modifiers for
each fourcc in PixelFormat class, we can pass just a single modifier.
So, replace the set with a single value.

Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
---
Changes since v1:
	- reformat the code.
	- Make commit message more articulate.

 include/libcamera/pixelformats.h     |  6 +++---
 src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
 src/libcamera/pixelformats.cpp       | 24 ++++++++++++------------
 3 files changed, 16 insertions(+), 16 deletions(-)

Comments

Kaaira Gupta April 3, 2020, 4:10 p.m. UTC | #1 
Sorry, forgot to add the 'Reviewed by' tag :/

On Fri, 3 Apr, 2020, 21:36 Kaaira Gupta, <kgupta@es.iitr.ac.in> wrote:

> DRM fourccs look like they have a per-plane modifier, but in fact each
> of them should be same. Hence instead of passing a set of modifiers for
> each fourcc in PixelFormat class, we can pass just a single modifier.
> So, replace the set with a single value.
>
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> ---
> Changes since v1:
>         - reformat the code.
>         - Make commit message more articulate.
>
>  include/libcamera/pixelformats.h     |  6 +++---
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
>  src/libcamera/pixelformats.cpp       | 24 ++++++++++++------------
>  3 files changed, 16 insertions(+), 16 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..1330dc5 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -19,9 +19,9 @@ 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
> - * DRM/KMS API (see linux/drm_fourcc.h).
> + * 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 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
>   */
>
>  /**
> --
> 2.17.1
>
>
Laurent Pinchart April 3, 2020, 9:17 p.m. UTC | #2 
Hi Kaaira,

Thank you for the patch.

On Fri, Apr 03, 2020 at 09:36:23PM +0530, Kaaira Gupta wrote:
> DRM fourccs look like they have a per-plane modifier, but in fact each
> of them should be same. Hence instead of passing a set of modifiers for
> each fourcc in PixelFormat class, we can pass just a single modifier.
> So, replace the set with a single value.
> 
> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Changes since v1:
> 	- reformat the code.
> 	- Make commit message more articulate.
> 
>  include/libcamera/pixelformats.h     |  6 +++---
>  src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
>  src/libcamera/pixelformats.cpp       | 24 ++++++++++++------------
>  3 files changed, 16 insertions(+), 16 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..1330dc5 100644
> --- a/src/libcamera/pixelformats.cpp
> +++ b/src/libcamera/pixelformats.cpp
> @@ -19,9 +19,9 @@ 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
> - * DRM/KMS API (see linux/drm_fourcc.h).
> + * 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 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
>   */
>  
>  /**
Laurent Pinchart April 3, 2020, 9:50 p.m. UTC | #3 
Kaaira,

On Sat, Apr 04, 2020 at 12:17:12AM +0300, Laurent Pinchart wrote:
> On Fri, Apr 03, 2020 at 09:36:23PM +0530, Kaaira Gupta wrote:
> > DRM fourccs look like they have a per-plane modifier, but in fact each
> > of them should be same. Hence instead of passing a set of modifiers for
> > each fourcc in PixelFormat class, we can pass just a single modifier.
> > So, replace the set with a single value.
> > 
> > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'm actually getting compilation errors with clang :-S

../../src/libcamera/pipeline/ipu3/ipu3.cpp:37:64: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]
        { MEDIA_BUS_FMT_SBGGR10_1X10, PixelFormat(DRM_FORMAT_SBGGR10, { IPU3_FORMAT_MOD_PACKED }) },
                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/libcamera/pipeline/ipu3/ipu3.cpp:38:64: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]
        { MEDIA_BUS_FMT_SGBRG10_1X10, PixelFormat(DRM_FORMAT_SGBRG10, { IPU3_FORMAT_MOD_PACKED }) },
                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/libcamera/pipeline/ipu3/ipu3.cpp:39:64: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]
        { MEDIA_BUS_FMT_SGRBG10_1X10, PixelFormat(DRM_FORMAT_SGRBG10, { IPU3_FORMAT_MOD_PACKED }) },
                                                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~
../../src/libcamera/pipeline/ipu3/ipu3.cpp:40:64: error: braces around scalar initializer [-Werror,-Wbraced-scalar-init]
        { MEDIA_BUS_FMT_SRGGB10_1X10, PixelFormat(DRM_FORMAT_SRGGB10, { IPU3_FORMAT_MOD_PACKED }) },

That's easy enough to fix by removing the braces. I've done so and
pushed the patch.

Please record it in your contributions.

> > ---
> > Changes since v1:
> > 	- reformat the code.
> > 	- Make commit message more articulate.
> > 
> >  include/libcamera/pixelformats.h     |  6 +++---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp |  2 +-
> >  src/libcamera/pixelformats.cpp       | 24 ++++++++++++------------
> >  3 files changed, 16 insertions(+), 16 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..1330dc5 100644
> > --- a/src/libcamera/pixelformats.cpp
> > +++ b/src/libcamera/pixelformats.cpp
> > @@ -19,9 +19,9 @@ 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
> > - * DRM/KMS API (see linux/drm_fourcc.h).
> > + * 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 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
> >   */
> >  
> >  /**

Patch
diff mbox series 

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..1330dc5 100644
--- a/src/libcamera/pixelformats.cpp
+++ b/src/libcamera/pixelformats.cpp
@@ -19,9 +19,9 @@  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
- * DRM/KMS API (see linux/drm_fourcc.h).
+ * 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 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
  */
 
 /**