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

[Niklas Söderlund]

Hi Jacopo,

On 2020-09-25 17:07:23 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> 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;
> 
> Should roles be capped to 2 ?

This is handled in Camera::generateConfiguration().

> 
> >
> > -	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;
> > +		}
> 
> Do you need to restrict the scope ?
> 
> > +		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() } };
> > +		} else {
> > +			selfPathAvailable = false;
> > +			for (const PixelFormat &format : RKISP1_RSZ_SP_FORMATS)
> > +				streamFormats[format] =
> > +				{ { RKISP1_RSZ_SP_SRC_MIN, data->sensor_->resolution() } };
> 
> Shouldn't this be
> 
> 				{ { RKISP1_RSZ_SP_SRC_MIN,
>                                     std:min(data->sensor_->resolution(),
>                                             RKISP1_RSZ_SP_SRC_MAX} };

You are correct, I do this when I refactor way this to RkISP1Path but 
not here. Will fix.

> 
> Same above
> 
> This apart
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> Thanks
>   j
> 
> > +		}
> > +
> > +		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));
> > --
> > 2.28.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund