[libcamera-devel] [PATCH v3 14/22] libcamera: pipeline: rkisp1: Expose self path stream

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Sep 25, 2020 at 03:41:59AM +0200, Niklas Söderlund wrote:
> Expose the self stream to applications and prefers it for the viewfinder
> and video roles as it can be extended to produce RGB. Keep preferring
> the main path for still capture as it could be extended to support RAW
> formats which makes most sense for still capture.
> 
> With this change the self path becomes available to applications and a
> camera backed by this pipeline can produce two streams simultaneously.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v2
> - Rework generation logic to grantee a stream is not picked for both
>   roles.
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 55 +++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index bd53183a034efaff..27a3c44da3805c5f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -718,17 +718,49 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> -	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> -	for (const PixelFormat &format : RKISP1_RSZ_MP_FORMATS)
> -		streamFormats[format] =
> -			{ { RKISP1_RSZ_MP_SRC_MIN, data->sensor_->resolution() } };
> +	bool mainPathAvailable = true;
> +	bool selfPathAvailable = true;
> +	for (const StreamRole role : roles) {
> +		bool useMainPath;
>  
> -	StreamFormats formats(streamFormats);
> -	StreamConfiguration cfg(formats);
> -	cfg.pixelFormat = formats::NV12;
> -	cfg.size = data->sensor_->resolution();
> +		switch (role) {
> +		case StreamRole::StillCapture: {
> +			useMainPath = mainPathAvailable;
> +			break;
> +		}
> +		case StreamRole::Viewfinder:
> +		case StreamRole::VideoRecording: {
> +			useMainPath = !selfPathAvailable;
> +			break;
> +		}
> +		default:
> +			LOG(RkISP1, Warning)
> +				<< "Requested stream role not supported: " << role;
> +			delete config;
> +			return nullptr;
> +		}
>  
> -	config->addConfiguration(cfg);
> +		std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
> +		if (useMainPath) {
> +			mainPathAvailable = false;
> +			for (const PixelFormat &format : RKISP1_RSZ_MP_FORMATS)
> +				streamFormats[format] =
> +				{ { RKISP1_RSZ_MP_SRC_MIN, data->sensor_->resolution() } };

One more indentation level is needed. You can write it as

				streamFormats[format] = { {
					RKISP1_RSZ_MP_SRC_MIN,
					data->sensor_->resolution()
				} };

for instance if you want to keep lines short.

As Jacopo pointed out, sensor_->resolution() needs to be capped to the
maximum supported size.

With this addressed,

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

> +		} else {
> +			selfPathAvailable = false;
> +			for (const PixelFormat &format : RKISP1_RSZ_SP_FORMATS)
> +				streamFormats[format] =
> +				{ { RKISP1_RSZ_SP_SRC_MIN, data->sensor_->resolution() } };
> +		}
> +
> +		StreamFormats formats(streamFormats);
> +		StreamConfiguration cfg(formats);
> +		cfg.pixelFormat = formats::NV12;
> +		cfg.size = data->sensor_->resolution();
> +		cfg.bufferCount = 4;
> +
> +		config->addConfiguration(cfg);
> +	}
>  
>  	config->validate();
>  
> @@ -1216,7 +1248,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor)
>  	if (ret)
>  		return ret;
>  
> -	std::set<Stream *> streams{ &data->mainPathStream_ };
> +	std::set<Stream *> streams{
> +		&data->mainPathStream_,
> +		&data->selfPathStream_,
> +	};
>  	std::shared_ptr<Camera> camera =
>  		Camera::create(this, data->sensor_->id(), streams);
>  	registerCamera(std::move(camera), std::move(data));

-- 
Regards,

Laurent Pinchart