[RFC PATCH v2 04/13] libcamera: simple: Add plain output configurations to software ISP

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Fri, Jan 24, 2025 at 10:57:55PM +0100, Milan Zamazal wrote:
> When software ISP is active, only software ISP adjusted output
> configurations are considered.  We want to support raw streams together
> with software ISP and for that purpose, we need to track also unmodified
> configurations.
> 
> A flag is added to SimpleCameraData::Configuration indicating whether
> it's for software ISP.  Configurations for unmodified output sizes and
> formats are added now whether software ISP is active or not.  We later
> filter formats and output sizes for particular stream configuration
> according to the new configuration flag.  This is just preparation and
> raw output is still not supported; for this reason, we just look whether
> software ISP is active; this will be changed in followup patches.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 30 ++++++++++++++----------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 06df909b..71853bb2 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -273,6 +273,7 @@ public:
>  		Size captureSize;
>  		std::vector<PixelFormat> outputFormats;
>  		SizeRange outputSizes;
> +		bool swisp;
>  	};
>  
>  	std::vector<Stream> streams_;
> @@ -654,19 +655,24 @@ void SimpleCameraData::tryPipeline(unsigned int code, const Size &size)
>  		config.sensorSize = size;
>  		config.captureFormat = pixelFormat;
>  		config.captureSize = format.size;
> +		config.swisp = false;
>  
>  		if (converter_) {
>  			config.outputFormats = converter_->formats(pixelFormat);
>  			config.outputSizes = converter_->sizes(format.size);
> -		} else if (swIsp_) {
> -			config.outputFormats = swIsp_->formats(pixelFormat);
> -			config.outputSizes = swIsp_->sizes(pixelFormat, format.size);
> -			if (config.outputFormats.empty()) {
> -				/* Do not use swIsp for unsupported pixelFormat's. */
> -				config.outputFormats = { pixelFormat };
> -				config.outputSizes = config.captureSize;
> -			}
>  		} else {
> +			if (swIsp_) {
> +				Configuration swispConfig = config;
> +				swispConfig.outputFormats = swIsp_->formats(pixelFormat);
> +				swispConfig.outputSizes = swIsp_->sizes(pixelFormat, format.size);
> +				if (swispConfig.outputFormats.empty()) {
> +					/* Do not use swIsp for unsupported pixelFormat's. */
> +					swispConfig.outputFormats = { pixelFormat };
> +					swispConfig.outputSizes = swispConfig.captureSize;
> +				}
> +				swispConfig.swisp = true;
> +				configs_.push_back(swispConfig);
> +			}

That's really getting too confusing. I've read the rest of the series,
and it doesn't get better :-(

I think we need to first refactor the existing code to make raw handling
identifical for the converter and software ISP cases. It doesn't mean
you have to create a common interface for the two elements (it would
be nice, but it's more yak shaving than I could reasonably ask for).

>  			config.outputFormats = { pixelFormat };
>  			config.outputSizes = config.captureSize;
>  		}
> @@ -1185,10 +1191,10 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>  	/* Create the formats map. */
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
> -	for (const SimpleCameraData::Configuration &cfg : data->configs_) {
> -		for (PixelFormat format : cfg.outputFormats)
> -			formats[format].push_back(cfg.outputSizes);
> -	}
> +	for (const SimpleCameraData::Configuration &cfg : data->configs_)
> +		if (static_cast<bool>(data->swIsp_) == cfg.swisp)
> +			for (PixelFormat format : cfg.outputFormats)
> +				formats[format].push_back(cfg.outputSizes);
>  
>  	/* Sort the sizes and merge any consecutive overlapping ranges. */
>  	for (auto &[format, sizes] : formats) {

-- 
Regards,

Laurent Pinchart