[RFC PATCH v2 03/13] libcamera: simple: Don't use raw output formats with conversions

[Laurent Pinchart]

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 at 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);
>  	}
>  

-- 
Regards,

Laurent Pinchart