[libcamera-devel,1/3] libcamera: ipu3: Improve error on multiple raw streams request

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

Commit Message

Jacopo Mondi June 28, 2020, 3:55 p.m. UTC
Improve the error message emitted when multiple raw streams are
requested.

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

Comments

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

Thanks for your work.

On 2020-06-28 17:55:37 +0200, Jacopo Mondi wrote:
> Improve the error message emitted when multiple raw streams are
> requested.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f2aa0caede7f..ef57196c32da 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -341,8 +341,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		case StreamRole::StillCaptureRaw: {
>  			if (streams.find(&data->rawStream_) == streams.end()) {
>  				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> +					<< "Multiple raw streams are not supported";
>  				break;
>  			}
>  
> -- 
> 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:40 p.m. UTC | #2
Hi Jacopo,

Thank you for the patch.

On Sun, Jun 28, 2020 at 05:55:37PM +0200, Jacopo Mondi wrote:
> Improve the error message emitted when multiple raw streams are
> requested.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f2aa0caede7f..ef57196c32da 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -341,8 +341,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		case StreamRole::StillCaptureRaw: {
>  			if (streams.find(&data->rawStream_) == streams.end()) {
>  				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> +					<< "Multiple raw streams are not supported";
>  				break;
>  			}
>
Kieran Bingham June 29, 2020, 10:21 a.m. UTC | #3
Hi Jacopo,

On 28/06/2020 16:55, Jacopo Mondi wrote:
> Improve the error message emitted when multiple raw streams are
> requested.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f2aa0caede7f..ef57196c32da 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -341,8 +341,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  		case StreamRole::StillCaptureRaw: {
>  			if (streams.find(&data->rawStream_) == streams.end()) {
>  				LOG(IPU3, Error)
> -					<< "No stream available for requested role "
> -					<< role;
> +					<< "Multiple raw streams are not supported";

I'm confused. Doesn't this conditional block look to find the rawStream
in the streams, and if it doesn't exist print this message?

I can't see where the context is that means this is adding 'another' raw
stream... unless the logic is inverted in the if statement? (or I'm just
getting it wrong).




>  				break;
>  			}
>  
>
Jacopo Mondi June 29, 2020, 10:32 a.m. UTC | #4
Hi Kieran,

On Mon, Jun 29, 2020 at 11:21:45AM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 28/06/2020 16:55, Jacopo Mondi wrote:
> > Improve the error message emitted when multiple raw streams are
> > requested.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f2aa0caede7f..ef57196c32da 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -341,8 +341,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> >  		case StreamRole::StillCaptureRaw: {
> >  			if (streams.find(&data->rawStream_) == streams.end()) {
> >  				LOG(IPU3, Error)
> > -					<< "No stream available for requested role "
> > -					<< role;
> > +					<< "Multiple raw streams are not supported";
>
> I'm confused. Doesn't this conditional block look to find the rawStream
> in the streams, and if it doesn't exist print this message?
>

Yes

> I can't see where the context is that means this is adding 'another' raw
> stream... unless the logic is inverted in the if statement? (or I'm just
> getting it wrong).


The functions declares an array of available streams
	std::set<Stream *> streams = {
		&data->outStream_,
		&data->vfStream_,
		&data->rawStream_,
	};

when one gets assigned it is removed from there

		streams.erase(stream);

So if we get here and fail to find rawStream_, it means it has been
assigned already.


>
>
>
>
> >  				break;
> >  			}
> >
> >
>
> --
> Regards
> --
> Kieran

Patch

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index f2aa0caede7f..ef57196c32da 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -341,8 +341,7 @@  CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
 		case StreamRole::StillCaptureRaw: {
 			if (streams.find(&data->rawStream_) == streams.end()) {
 				LOG(IPU3, Error)
-					<< "No stream available for requested role "
-					<< role;
+					<< "Multiple raw streams are not supported";
 				break;
 			}