| Message ID | 20201022081731.36217-3-tomi.valkeinen@iki.fi |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, 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@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 ... Perhaps this should only connect the convertor bufferReady signal in an else statement? -- Kieran > --- > 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); >
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@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);
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);
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@iki.fi> --- src/libcamera/pipeline/simple/simple.cpp | 1 + 1 file changed, 1 insertion(+)