[libcamera-devel] [PATCH 3/4] libcamera: pipeline: simple: return from match() on error

[Laurent Pinchart]

Hello,

On Thu, Oct 22, 2020 at 10:53:52AM +0100, Kieran Bingham wrote:
> On 22/10/2020 09:17, Tomi Valkeinen wrote:
> > If converter_->open() fails, the code deletes the converter_ but then
> > happily goes on, and at the very next lines will use converter_.
> > 
> > I assume the intention is to return from the function with false.
> > 
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen at iki.fi>
> 
> I think in this instance, the failure to open convertor should simply
> disable conversion on the pipeline (as indicated by the warning message
> printed in that code block).
> 
> Returning false here would completely disable the whole pipeline ...

Correct, that was the intent.

> Perhaps this should only connect the convertor bufferReady signal in an
> else statement?

Indeed, that's a bug :-) Tomi, I assume you'll submit a v2 as you've
spotted this, but if you'd like me to fix the bug instead, I'm happy to
do so.

> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 0d48e1b6..df93bc8d 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -774,6 +774,7 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> >  				<< "Failed to open converter, disabling format conversion";
> >  			delete converter_;
> >  			converter_ = nullptr;
> > +			return false;
> >  		}
> >  
> >  		converter_->bufferReady.connect(this, &SimplePipelineHandler::converterDone);

-- 
Regards,

Laurent Pinchart