| Message ID | 20241219103108.244243-1-mzamazal@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. december 19., csütörtök 11:31 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > inputBufferReady ready signal in the simple pipeline is handled in the > pipeline handler thread. Similarly, outputBufferReady and ispStatsReady > signals should be handled there too, everything should be called from > the right thread and not block the callers. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8ac24e6e..280112f4 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -548,12 +548,17 @@ int SimpleCameraData::init() > * bound explicitly to the pipe, which is bound to the pipeline > * handler thread. The function then simply forwards the call to > * conversionInputDone(). > + * Similarly for outputBufferReady and ispStatsReady signals. > */ > swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { > this->conversionInputDone(buffer); > }); > - swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > - swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > + swIsp_->outputBufferReady.connect(this, [this](FrameBuffer *buffer) { > + this->conversionOutputDone(buffer); > + }); > + swIsp_->ispStatsReady.connect(this, [this](uint32_t frame, uint32_t bufferId) { > + this->ispStatsReady(frame, bufferId); > + }); > [...] The object is still `this`, so I wouldn't expect any difference in behaviour. Is there? Regards, Barnabás Pőcze
Hi Barnabás, thank you for review. Barnabás Pőcze <pobrn@protonmail.com> writes: > Hi > > > 2024. december 19., csütörtök 11:31 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > >> inputBufferReady ready signal in the simple pipeline is handled in the >> pipeline handler thread. Similarly, outputBufferReady and ispStatsReady >> signals should be handled there too, everything should be called from >> the right thread and not block the callers. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 8ac24e6e..280112f4 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -548,12 +548,17 @@ int SimpleCameraData::init() >> * bound explicitly to the pipe, which is bound to the pipeline >> * handler thread. The function then simply forwards the call to >> * conversionInputDone(). >> + * Similarly for outputBufferReady and ispStatsReady signals. >> */ >> swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { >> this->conversionInputDone(buffer); >> }); >> - swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); >> - swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); >> + swIsp_->outputBufferReady.connect(this, [this](FrameBuffer *buffer) { >> + this->conversionOutputDone(buffer); >> + }); >> + swIsp_->ispStatsReady.connect(this, [this](uint32_t frame, uint32_t bufferId) { >> + this->ispStatsReady(frame, bufferId); >> + }); >> [...] > > The object is still `this`, so I wouldn't expect any difference in behaviour. Is there? Ah, right, should be `pipe', will fix it in v2. > Regards, > Barnabás Pőcze
Hi Milan, On Thu, Dec 19, 2024 at 10:13:17PM +0100, Milan Zamazal wrote: > Barnabás Pőcze writes: > > 2024. december 19., csütörtök 11:31 keltezéssel, Milan Zamazal írta: > > > >> inputBufferReady ready signal in the simple pipeline is handled in the > >> pipeline handler thread. Similarly, outputBufferReady and ispStatsReady > >> signals should be handled there too, everything should be called from > >> the right thread and not block the callers. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> src/libcamera/pipeline/simple/simple.cpp | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index 8ac24e6e..280112f4 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -548,12 +548,17 @@ int SimpleCameraData::init() > >> * bound explicitly to the pipe, which is bound to the pipeline > >> * handler thread. The function then simply forwards the call to > >> * conversionInputDone(). > >> + * Similarly for outputBufferReady and ispStatsReady signals. > >> */ > >> swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { > >> this->conversionInputDone(buffer); > >> }); > >> - swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > >> - swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > >> + swIsp_->outputBufferReady.connect(this, [this](FrameBuffer *buffer) { > >> + this->conversionOutputDone(buffer); > >> + }); > >> + swIsp_->ispStatsReady.connect(this, [this](uint32_t frame, uint32_t bufferId) { > >> + this->ispStatsReady(frame, bufferId); > >> + }); > >> [...] > > > > The object is still `this`, so I wouldn't expect any difference in behaviour. Is there? > > Ah, right, should be `pipe', will fix it in v2. I think it would be better to emit the signals from the camera manager thread, instead of relying on the user of the soft ISP to perform cross-thread synchronization. That will be less error-prone.
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 8ac24e6e..280112f4 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -548,12 +548,17 @@ int SimpleCameraData::init() * bound explicitly to the pipe, which is bound to the pipeline * handler thread. The function then simply forwards the call to * conversionInputDone(). + * Similarly for outputBufferReady and ispStatsReady signals. */ swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { this->conversionInputDone(buffer); }); - swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); - swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); + swIsp_->outputBufferReady.connect(this, [this](FrameBuffer *buffer) { + this->conversionOutputDone(buffer); + }); + swIsp_->ispStatsReady.connect(this, [this](uint32_t frame, uint32_t bufferId) { + this->ispStatsReady(frame, bufferId); + }); swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls); } }
inputBufferReady ready signal in the simple pipeline is handled in the pipeline handler thread. Similarly, outputBufferReady and ispStatsReady signals should be handled there too, everything should be called from the right thread and not block the callers. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/libcamera/pipeline/simple/simple.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)