[libcamera-devel,v3,14/22] libcamera: pipeline: rkisp1: Expose self path stream
diff mbox series

Message ID 20200925014207.1455796-15-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: pipeline: rkisp1: Extend to support two streams
Related show

Commit Message

Niklas Söderlund Sept. 25, 2020, 1:41 a.m. UTC 
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@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(-)

Comments

Jacopo Mondi Sept. 25, 2020, 3:07 p.m. UTC | #1 
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@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 ?

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

Same above

This apart
Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund Sept. 25, 2020, 4:50 p.m. UTC | #2 
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@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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Sept. 28, 2020, 8:39 p.m. UTC | #3 
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@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@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));

Patch
diff mbox series 

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() } };
+		} 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));