[libcamera-devel,3/3] libcamera: pipelines: Fail if more than one role is requested
diff mbox series

Message ID 20200628161723.30625-4-jacopo@jmondi.org
State RFC
Delegated to: Jacopo Mondi
Headers show
Series
  • libcamera: pipeline handlers: Fail if roles are not supported
Related show

Commit Message

Jacopo Mondi June 28, 2020, 4:17 p.m. UTC 
The rkisp1, vimc, uvc and simple pipeline handlers only support a single
stream.

Their generateConfiguration() implementations does not check how many
roles have been requested, silently ignoring requests for more than one
role that cannot be satisfied.

Fix this and comply to the documented Camera API by failing if more than one
role is requested by applications.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 5 +++++
 src/libcamera/pipeline/simple/simple.cpp     | 5 +++++
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++++
 src/libcamera/pipeline/vimc/vimc.cpp         | 5 +++++
 4 files changed, 20 insertions(+)

Comments

Niklas Söderlund June 28, 2020, 6:38 p.m. UTC | #1 
Hello Jacopo,

Thanks for your patch.

On 2020-06-28 18:17:23 +0200, Jacopo Mondi wrote:
> The rkisp1, vimc, uvc and simple pipeline handlers only support a single
> stream.
> 
> Their generateConfiguration() implementations does not check how many
> roles have been requested, silently ignoring requests for more than one
> role that cannot be satisfied.
> 
> Fix this and comply to the documented Camera API by failing if more than one
> role is requested by applications.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 5 +++++
>  src/libcamera/pipeline/simple/simple.cpp     | 5 +++++
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++++
>  src/libcamera/pipeline/vimc/vimc.cpp         | 5 +++++
>  4 files changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c01821135f8..6d5c7c39edf1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -566,6 +566,11 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	StreamConfiguration cfg{};
>  	cfg.pixelFormat = formats::NV12;
>  	cfg.size = data->sensor_->resolution();
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1ec8d0f7de03..20675de568a8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -484,6 +484,11 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	/* Create the formats map. */
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  	std::transform(data->formats_.begin(), data->formats_.end(),
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 80a0e77ba3fc..773480e0a821 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -158,6 +158,11 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	std::map<V4L2PixelFormat, std::vector<SizeRange>> v4l2Formats =
>  		data->video_->formats();
>  	std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index b6530662a9ba..e0980fc4a64c 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -177,6 +177,11 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
>  	for (const auto &pixelformat : pixelformats) {
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart June 28, 2020, 8:59 p.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Sun, Jun 28, 2020 at 06:17:23PM +0200, Jacopo Mondi wrote:
> The rkisp1, vimc, uvc and simple pipeline handlers only support a single
> stream.
> 
> Their generateConfiguration() implementations does not check how many
> roles have been requested, silently ignoring requests for more than one
> role that cannot be satisfied.
> 
> Fix this and comply to the documented Camera API by failing if more than one
> role is requested by applications.

I don't think this is needed, Camera::generateConfiguration() already
has

	if (roles.size() > streams().size())
		return nullptr;

> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 5 +++++
>  src/libcamera/pipeline/simple/simple.cpp     | 5 +++++
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 5 +++++
>  src/libcamera/pipeline/vimc/vimc.cpp         | 5 +++++
>  4 files changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 3c01821135f8..6d5c7c39edf1 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -566,6 +566,11 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	StreamConfiguration cfg{};
>  	cfg.pixelFormat = formats::NV12;
>  	cfg.size = data->sensor_->resolution();
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1ec8d0f7de03..20675de568a8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -484,6 +484,11 @@ CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	/* Create the formats map. */
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  	std::transform(data->formats_.begin(), data->formats_.end(),
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 80a0e77ba3fc..773480e0a821 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -158,6 +158,11 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	std::map<V4L2PixelFormat, std::vector<SizeRange>> v4l2Formats =
>  		data->video_->formats();
>  	std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index b6530662a9ba..e0980fc4a64c 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -177,6 +177,11 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
>  	if (roles.empty())
>  		return config;
>  
> +	if (roles.size() > 1) {
> +		delete config;
> +		return nullptr;
> +	}
> +
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
>  	for (const auto &pixelformat : pixelformats) {

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 3c01821135f8..6d5c7c39edf1 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -566,6 +566,11 @@  CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
 	if (roles.empty())
 		return config;
 
+	if (roles.size() > 1) {
+		delete config;
+		return nullptr;
+	}
+
 	StreamConfiguration cfg{};
 	cfg.pixelFormat = formats::NV12;
 	cfg.size = data->sensor_->resolution();
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 1ec8d0f7de03..20675de568a8 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -484,6 +484,11 @@  CameraConfiguration *SimplePipelineHandler::generateConfiguration(Camera *camera
 	if (roles.empty())
 		return config;
 
+	if (roles.size() > 1) {
+		delete config;
+		return nullptr;
+	}
+
 	/* Create the formats map. */
 	std::map<PixelFormat, std::vector<SizeRange>> formats;
 	std::transform(data->formats_.begin(), data->formats_.end(),
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 80a0e77ba3fc..773480e0a821 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -158,6 +158,11 @@  CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
 	if (roles.empty())
 		return config;
 
+	if (roles.size() > 1) {
+		delete config;
+		return nullptr;
+	}
+
 	std::map<V4L2PixelFormat, std::vector<SizeRange>> v4l2Formats =
 		data->video_->formats();
 	std::map<PixelFormat, std::vector<SizeRange>> deviceFormats;
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index b6530662a9ba..e0980fc4a64c 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -177,6 +177,11 @@  CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
 	if (roles.empty())
 		return config;
 
+	if (roles.size() > 1) {
+		delete config;
+		return nullptr;
+	}
+
 	std::map<PixelFormat, std::vector<SizeRange>> formats;
 
 	for (const auto &pixelformat : pixelformats) {