[libcamera-devel] lc-compliance: simple_capture: Handle unsupported roles

11930 diff mbox series

It's valid for a camera to return a nullptr if the requested set of
roles can not be satisfied. This is not correctly handled by
lc-compliance which instead crashes, fix this.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/lc-compliance/simple_capture.cpp | 3 +++
 1 file changed, 3 insertions(+)

Hi Niklas,

On 14/04/2021 15:10, Niklas Söderlund wrote:
> It's valid for a camera to return a nullptr if the requested set of
> roles can not be satisfied. This is not correctly handled by
> lc-compliance which instead crashes, fix this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Ohhh I bet we haven't considered this in all the places it's used.

But indeed this is true.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/lc-compliance/simple_capture.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> index 811a6220009654be..64e862a08e3a8221 100644
> --- a/src/lc-compliance/simple_capture.cpp
> +++ b/src/lc-compliance/simple_capture.cpp
> @@ -23,6 +23,9 @@ Results::Result SimpleCapture::configure(StreamRole role)
>  {
>  	config_ = camera_->generateConfiguration({ role });
>  
> +	if (!config_)
> +		return { Results::Skip, "Role not supported by camera" };
> +
>  	if (config_->validate() != CameraConfiguration::Valid) {
>  		config_.reset();
>  		return { Results::Fail, "Configuration not valid" };
>

Hi Kieran,

On 2021-04-14 15:19:54 +0100, Kieran Bingham wrote:
> Hi Niklas,
> 
> On 14/04/2021 15:10, Niklas Söderlund wrote:
> > It's valid for a camera to return a nullptr if the requested set of
> > roles can not be satisfied. This is not correctly handled by
> > lc-compliance which instead crashes, fix this.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> Ohhh I bet we haven't considered this in all the places it's used.

Yea I think it's bordering on a flaw in our organic API growth. It's not 
super clear at a first look what the difference errors signaled between

    !conifg

and

    config->validate() != CameraConfiguration::Valid

are. I think we somehow should consolidate this in the Camera so that 
applications always get a configuration but if the pipeline can't 
satisfy the roles it could maybe also be signaled thru 
config->validate()?

> 
> But indeed this is true.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > ---
> >  src/lc-compliance/simple_capture.cpp | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/lc-compliance/simple_capture.cpp b/src/lc-compliance/simple_capture.cpp
> > index 811a6220009654be..64e862a08e3a8221 100644
> > --- a/src/lc-compliance/simple_capture.cpp
> > +++ b/src/lc-compliance/simple_capture.cpp
> > @@ -23,6 +23,9 @@ Results::Result SimpleCapture::configure(StreamRole role)
> >  {
> >  	config_ = camera_->generateConfiguration({ role });
> >  
> > +	if (!config_)
> > +		return { Results::Skip, "Role not supported by camera" };
> > +
> >  	if (config_->validate() != CameraConfiguration::Valid) {
> >  		config_.reset();
> >  		return { Results::Fail, "Configuration not valid" };
> > 
> 
> -- 
> Regards
> --
> Kieran

Em 2021-04-14 11:10, Niklas Söderlund escreveu:

> It's valid for a camera to return a nullptr if the requested set of
> roles can not be satisfied. This is not correctly handled by
> lc-compliance which instead crashes, fix this.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

Glad to soon have lc-compliance working on rkisp1 without needing any additional
patches :).

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas