[RFC,v2,03/13] libcamera: simple: Don't use raw output formats with conversions
diff mbox series

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

Commit Message

Milan Zamazal Jan. 24, 2025, 9:57 p.m. UTC
In order to support raw streams, we need to add raw formats to software
ISP configurations.  In this preparatory patch, the raw formats are
excluded from software ISP output configurations.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

Laurent Pinchart Jan. 26, 2025, 9:02 p.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Fri, Jan 24, 2025 at 10:57:54PM +0100, Milan Zamazal wrote:
> In order to support raw streams, we need to add raw formats to software
> ISP configurations.  In this preparatory patch, the raw formats are
> excluded from software ISP output configurations.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e9bc630..06df909b 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -26,6 +26,7 @@
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
> +#include <libcamera/pixel_format.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -209,6 +210,12 @@ static const SimplePipelineInfo supportedDevices[] = {
>  	{ "sun6i-csi", {}, false },
>  };
>  
> +bool isRawFormat(const PixelFormat &format)
> +{
> +	return libcamera::PixelFormatInfo::info(format).colourEncoding ==
> +	       libcamera::PixelFormatInfo::ColourEncodingRAW;
> +}
> +
>  } /* namespace */
>  
>  class SimpleCameraData : public Camera::Private
> @@ -1284,7 +1291,8 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  
>  		cfg.setStream(&data->streams_[i]);
>  
> -		if (data->useConversion_)
> +		if (data->useConversion_ &&
> +		    (!data->swIsp_ || !isRawFormat(cfg.pixelFormat)))

That's getting confusing. As the code is hard enough to follow, I'm not
keen on making it even more obfuscated.

At the moment, useConversion_ and swIsp_ are mutually exclusive. I
understand this won't be the case anymore with this series, and
useConversion_ will be true when using the software ISP and capturing
both the raw and processed streams.

If you want to reuse some of the converter infrastructure (including
variables), I would like to see clear naming rules. We could for
instance use convert* for data related to the hardware converters,
swisp* for data related to the software ISP, and another prefix (name
TBD) for data common to both. Ideally, of course, we should abstract
both hardware converters and the software ISP with the same interface.

>  			outputCfgs.push_back(cfg);
>  	}
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 6e9bc630..06df909b 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -26,6 +26,7 @@ 
 
 #include <libcamera/camera.h>
 #include <libcamera/control_ids.h>
+#include <libcamera/pixel_format.h>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
@@ -209,6 +210,12 @@  static const SimplePipelineInfo supportedDevices[] = {
 	{ "sun6i-csi", {}, false },
 };
 
+bool isRawFormat(const PixelFormat &format)
+{
+	return libcamera::PixelFormatInfo::info(format).colourEncoding ==
+	       libcamera::PixelFormatInfo::ColourEncodingRAW;
+}
+
 } /* namespace */
 
 class SimpleCameraData : public Camera::Private
@@ -1284,7 +1291,8 @@  int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
 
 		cfg.setStream(&data->streams_[i]);
 
-		if (data->useConversion_)
+		if (data->useConversion_ &&
+		    (!data->swIsp_ || !isRawFormat(cfg.pixelFormat)))
 			outputCfgs.push_back(cfg);
 	}