[libcamera-devel,2/3] libcamera: ipu3: Accept an empty roles list

Message ID 20200628155539.29498-3-jacopo@jmondi.org
State Accepted
Commit 5267ca8e0209c08ba2b022543f291985c4cfeaf5
Headers show
Series
  • ipu3: Accept empty roles list in generateConfiguration()
Related show

Commit Message

Jacopo Mondi June 28, 2020, 3:55 p.m. UTC
The IPU3 pipeline handler that does not support receiving an empty list
of roles at generateConfiguration() time. This contradicts the camera
API which allows application to generate empty CameraConfiguration to
be later manually filled.

Fix this by returning an empty CameraConfiguration if the list of
requested roles is empty. While at it, align the style with the other
pipeline handlers.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

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

Thanks for your work.

On 2020-06-28 17:55:38 +0200, Jacopo Mondi wrote:
> The IPU3 pipeline handler that does not support receiving an empty list
> of roles at generateConfiguration() time. This contradicts the camera
> API which allows application to generate empty CameraConfiguration to
> be later manually filled.
> 
> Fix this by returning an empty CameraConfiguration if the list of
> requested roles is empty. While at it, align the style with the other
> pipeline handlers.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

I'm not sure this is the way we wish this API to work in the end. But a 
good start is that all pipelines do the same thing :-)

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

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ef57196c32da..cbf19793c43e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -292,14 +292,15 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	IPU3CameraConfiguration *config;
> +	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
>  	std::set<Stream *> streams = {
>  		&data->outStream_,
>  		&data->vfStream_,
>  		&data->rawStream_,
>  	};
>  
> -	config = new IPU3CameraConfiguration(camera, data);
> +	if (roles.empty())
> +		return config;
>  
>  	for (const StreamRole role : roles) {
>  		StreamConfiguration cfg = {};
> -- 
> 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:41 p.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Sun, Jun 28, 2020 at 05:55:38PM +0200, Jacopo Mondi wrote:
> The IPU3 pipeline handler that does not support receiving an empty list
> of roles at generateConfiguration() time. This contradicts the camera
> API which allows application to generate empty CameraConfiguration to
> be later manually filled.
> 
> Fix this by returning an empty CameraConfiguration if the list of
> requested roles is empty. While at it, align the style with the other
> pipeline handlers.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ef57196c32da..cbf19793c43e 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -292,14 +292,15 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  	const StreamRoles &roles)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	IPU3CameraConfiguration *config;
> +	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);

I wouldn't have changed this, up to you.

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

>  	std::set<Stream *> streams = {
>  		&data->outStream_,
>  		&data->vfStream_,
>  		&data->rawStream_,
>  	};
>  
> -	config = new IPU3CameraConfiguration(camera, data);
> +	if (roles.empty())
> +		return config;
>  
>  	for (const StreamRole role : roles) {
>  		StreamConfiguration cfg = {};

Patch

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index ef57196c32da..cbf19793c43e 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -292,14 +292,15 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 	const StreamRoles &roles)
 {
 	IPU3CameraData *data = cameraData(camera);
-	IPU3CameraConfiguration *config;
+	IPU3CameraConfiguration *config = new IPU3CameraConfiguration(camera, data);
 	std::set<Stream *> streams = {
 		&data->outStream_,
 		&data->vfStream_,
 		&data->rawStream_,
 	};
 
-	config = new IPU3CameraConfiguration(camera, data);
+	if (roles.empty())
+		return config;
 
 	for (const StreamRole role : roles) {
 		StreamConfiguration cfg = {};