[v7,03/12] libcamera: formats: Add a helper to check for a raw pixel format
diff mbox series

Message ID 20250627113449.23106-4-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Enable raw streams with software ISP
Related show

Commit Message

Milan Zamazal June 27, 2025, 11:34 a.m. UTC 
There are several places with the same pattern to check whether a given
pixel format is a raw format:

  return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
         libcamera::PixelFormatInfo::ColourEncodingRAW;

Let's move the corresponding isFormatRaw helper from mali-c55.cpp to
formats.cpp and use it where applicable.  This also avoids a need to
introduce the same helper (or the same pattern) in the followup patches.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 include/libcamera/internal/formats.h         |  2 ++
 src/libcamera/formats.cpp                    | 11 +++++++++++
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  4 ++--
 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 10 ----------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp     |  7 ++-----
 5 files changed, 17 insertions(+), 17 deletions(-)

Comments

Umang Jain July 1, 2025, 8:06 a.m. UTC | #1 
Hi Milan,

On Fri, Jun 27, 2025 at 01:34:38PM +0200, Milan Zamazal wrote:
> There are several places with the same pattern to check whether a given
> pixel format is a raw format:
> 
>   return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>          libcamera::PixelFormatInfo::ColourEncodingRAW;
> 
> Let's move the corresponding isFormatRaw helper from mali-c55.cpp to
> formats.cpp and use it where applicable.  This also avoids a need to
> introduce the same helper (or the same pattern) in the followup patches.
> 

I think I had tried this previously. See the discussion:
https://patchwork.libcamera.org/project/libcamera/list/?series=4640&state=*

> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  include/libcamera/internal/formats.h         |  2 ++
>  src/libcamera/formats.cpp                    | 11 +++++++++++
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  4 ++--
>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 10 ----------
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     |  7 ++-----
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 6a3e9c16a..bc4417d05 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -62,4 +62,6 @@ public:
>  	std::array<Plane, 3> planes;
>  };
>  
> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt);
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index bfcdfc089..c6e0a2620 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -1215,4 +1215,15 @@ unsigned int PixelFormatInfo::numPlanes() const
>  	return count;
>  }
>  
> +/**
> + * \brief Return whether the given pixel format is a raw format
> + * \param[in] pixFmt The pixel format to examine
> + * \return True iff the given format is a raw format
> + */
> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> +{
> +	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
> +	       libcamera::PixelFormatInfo::ColourEncodingRAW;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index f4014b95d..c9f56af02 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -24,6 +24,7 @@
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_sensor.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/v4l2_subdevice.h"
> @@ -312,8 +313,7 @@ unsigned int ISICameraData::getYuvMediaBusFormat(const PixelFormat &pixelFormat)
>  
>  unsigned int ISICameraData::getMediaBusFormat(PixelFormat *pixelFormat) const
>  {
> -	if (PixelFormatInfo::info(*pixelFormat).colourEncoding ==
> -	    PixelFormatInfo::ColourEncodingRAW)
> +	if (isFormatRaw(*pixelFormat))
>  		return getRawMediaBusFormat(pixelFormat);
>  
>  	return getYuvMediaBusFormat(*pixelFormat);
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 4acc091bd..8d9b1e380 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -42,16 +42,6 @@
>  #include "libcamera/internal/v4l2_subdevice.h"
>  #include "libcamera/internal/v4l2_videodevice.h"
>  
> -namespace {
> -
> -bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> -{
> -	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
> -	       libcamera::PixelFormatInfo::ColourEncodingRAW;
> -}
> -
> -} /* namespace */
> -
>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(MaliC55)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 675f0a749..50c83fb72 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -538,8 +538,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  	 */
>  	if (config_.size() > 1) {
>  		for (const auto &cfg : config_) {
> -			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
> -			    PixelFormatInfo::ColourEncodingRAW) {
> +			if (isFormatRaw(cfg.pixelFormat)) {
>  				config_.resize(1);
>  				status = Adjusted;
>  				break;
> @@ -553,9 +552,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  		 * Platforms with dewarper support, such as i.MX8MP, support
>  		 * only a single stream. We can inspect config_[0] only here.
>  		 */
> -		bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
> -			     PixelFormatInfo::ColourEncodingRAW;
> -		if (!isRaw)
> +		if (!isFormatRaw(config_[0].pixelFormat))
>  			useDewarper = true;
>  	}
>  
> -- 
> 2.50.0
>
Milan Zamazal July 1, 2025, 1:53 p.m. UTC | #2 
Hi Umang,

Umang Jain <uajain@igalia.com> writes:

> Hi Milan,
>
> On Fri, Jun 27, 2025 at 01:34:38PM +0200, Milan Zamazal wrote:
>> There are several places with the same pattern to check whether a given
>> pixel format is a raw format:
>> 
>>   return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>>          libcamera::PixelFormatInfo::ColourEncodingRAW;
>> 
>> Let's move the corresponding isFormatRaw helper from mali-c55.cpp to
>> formats.cpp and use it where applicable.  This also avoids a need to
>> introduce the same helper (or the same pattern) in the followup patches.
>> 
>
> I think I had tried this previously. See the discussion:
> https://patchwork.libcamera.org/project/libcamera/list/?series=4640&state=*

Thank you for the pointer.  It looks like attempts to unify those
patterns are consistently difficult and there'll be no shame if I simply
drop this and use another private helper, for the simple pipeline.  The
series doesn't move forward, let's push problematic parts out of it.

>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  include/libcamera/internal/formats.h         |  2 ++
>>  src/libcamera/formats.cpp                    | 11 +++++++++++
>>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp |  4 ++--
>>  src/libcamera/pipeline/mali-c55/mali-c55.cpp | 10 ----------
>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     |  7 ++-----
>>  5 files changed, 17 insertions(+), 17 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
>> index 6a3e9c16a..bc4417d05 100644
>> --- a/include/libcamera/internal/formats.h
>> +++ b/include/libcamera/internal/formats.h
>> @@ -62,4 +62,6 @@ public:
>>  	std::array<Plane, 3> planes;
>>  };
>>  
>> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt);
>> +
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
>> index bfcdfc089..c6e0a2620 100644
>> --- a/src/libcamera/formats.cpp
>> +++ b/src/libcamera/formats.cpp
>> @@ -1215,4 +1215,15 @@ unsigned int PixelFormatInfo::numPlanes() const
>>  	return count;
>>  }
>>  
>> +/**
>> + * \brief Return whether the given pixel format is a raw format
>> + * \param[in] pixFmt The pixel format to examine
>> + * \return True iff the given format is a raw format
>> + */
>> +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
>> +{
>> +	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>> +	       libcamera::PixelFormatInfo::ColourEncodingRAW;
>> +}
>> +
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> index f4014b95d..c9f56af02 100644
>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> @@ -24,6 +24,7 @@
>>  #include "libcamera/internal/camera.h"
>>  #include "libcamera/internal/camera_sensor.h"
>>  #include "libcamera/internal/device_enumerator.h"
>> +#include "libcamera/internal/formats.h"
>>  #include "libcamera/internal/media_device.h"
>>  #include "libcamera/internal/pipeline_handler.h"
>>  #include "libcamera/internal/v4l2_subdevice.h"
>> @@ -312,8 +313,7 @@ unsigned int ISICameraData::getYuvMediaBusFormat(const PixelFormat &pixelFormat)
>>  
>>  unsigned int ISICameraData::getMediaBusFormat(PixelFormat *pixelFormat) const
>>  {
>> -	if (PixelFormatInfo::info(*pixelFormat).colourEncoding ==
>> -	    PixelFormatInfo::ColourEncodingRAW)
>> +	if (isFormatRaw(*pixelFormat))
>>  		return getRawMediaBusFormat(pixelFormat);
>>  
>>  	return getYuvMediaBusFormat(*pixelFormat);
>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> index 4acc091bd..8d9b1e380 100644
>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>> @@ -42,16 +42,6 @@
>>  #include "libcamera/internal/v4l2_subdevice.h"
>>  #include "libcamera/internal/v4l2_videodevice.h"
>>  
>> -namespace {
>> -
>> -bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
>> -{
>> -	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
>> -	       libcamera::PixelFormatInfo::ColourEncodingRAW;
>> -}
>> -
>> -} /* namespace */
>> -
>>  namespace libcamera {
>>  
>>  LOG_DEFINE_CATEGORY(MaliC55)
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index 675f0a749..50c83fb72 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -538,8 +538,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>  	 */
>>  	if (config_.size() > 1) {
>>  		for (const auto &cfg : config_) {
>> -			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
>> -			    PixelFormatInfo::ColourEncodingRAW) {
>> +			if (isFormatRaw(cfg.pixelFormat)) {
>>  				config_.resize(1);
>>  				status = Adjusted;
>>  				break;
>> @@ -553,9 +552,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>>  		 * Platforms with dewarper support, such as i.MX8MP, support
>>  		 * only a single stream. We can inspect config_[0] only here.
>>  		 */
>> -		bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
>> -			     PixelFormatInfo::ColourEncodingRAW;
>> -		if (!isRaw)
>> +		if (!isFormatRaw(config_[0].pixelFormat))
>>  			useDewarper = true;
>>  	}
>>  
>> -- 
>> 2.50.0
>>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
index 6a3e9c16a..bc4417d05 100644
--- a/include/libcamera/internal/formats.h
+++ b/include/libcamera/internal/formats.h
@@ -62,4 +62,6 @@  public:
 	std::array<Plane, 3> planes;
 };
 
+bool isFormatRaw(const libcamera::PixelFormat &pixFmt);
+
 } /* namespace libcamera */
diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
index bfcdfc089..c6e0a2620 100644
--- a/src/libcamera/formats.cpp
+++ b/src/libcamera/formats.cpp
@@ -1215,4 +1215,15 @@  unsigned int PixelFormatInfo::numPlanes() const
 	return count;
 }
 
+/**
+ * \brief Return whether the given pixel format is a raw format
+ * \param[in] pixFmt The pixel format to examine
+ * \return True iff the given format is a raw format
+ */
+bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
+{
+	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
+	       libcamera::PixelFormatInfo::ColourEncodingRAW;
+}
+
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index f4014b95d..c9f56af02 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -24,6 +24,7 @@ 
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/camera_sensor.h"
 #include "libcamera/internal/device_enumerator.h"
+#include "libcamera/internal/formats.h"
 #include "libcamera/internal/media_device.h"
 #include "libcamera/internal/pipeline_handler.h"
 #include "libcamera/internal/v4l2_subdevice.h"
@@ -312,8 +313,7 @@  unsigned int ISICameraData::getYuvMediaBusFormat(const PixelFormat &pixelFormat)
 
 unsigned int ISICameraData::getMediaBusFormat(PixelFormat *pixelFormat) const
 {
-	if (PixelFormatInfo::info(*pixelFormat).colourEncoding ==
-	    PixelFormatInfo::ColourEncodingRAW)
+	if (isFormatRaw(*pixelFormat))
 		return getRawMediaBusFormat(pixelFormat);
 
 	return getYuvMediaBusFormat(*pixelFormat);
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index 4acc091bd..8d9b1e380 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -42,16 +42,6 @@ 
 #include "libcamera/internal/v4l2_subdevice.h"
 #include "libcamera/internal/v4l2_videodevice.h"
 
-namespace {
-
-bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
-{
-	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
-	       libcamera::PixelFormatInfo::ColourEncodingRAW;
-}
-
-} /* namespace */
-
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(MaliC55)
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 675f0a749..50c83fb72 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -538,8 +538,7 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 	 */
 	if (config_.size() > 1) {
 		for (const auto &cfg : config_) {
-			if (PixelFormatInfo::info(cfg.pixelFormat).colourEncoding ==
-			    PixelFormatInfo::ColourEncodingRAW) {
+			if (isFormatRaw(cfg.pixelFormat)) {
 				config_.resize(1);
 				status = Adjusted;
 				break;
@@ -553,9 +552,7 @@  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
 		 * Platforms with dewarper support, such as i.MX8MP, support
 		 * only a single stream. We can inspect config_[0] only here.
 		 */
-		bool isRaw = PixelFormatInfo::info(config_[0].pixelFormat).colourEncoding ==
-			     PixelFormatInfo::ColourEncodingRAW;
-		if (!isRaw)
+		if (!isFormatRaw(config_[0].pixelFormat))
 			useDewarper = true;
 	}