| Message ID | 20241219103108.244243-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| 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.
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > 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. Maybe a stupid question, but how to do it? The signals are emitted from the corresponding place/thread asynchronously and there is little choice of the thread to use. At least without passing around some Object bound to the camera manager thread, which wouldn't be nice either I suppose. What do I miss?
Hi Milan, On Fri, Jan 03, 2025 at 07:31:54PM +0100, Milan Zamazal wrote: > Laurent Pinchart writes: > > 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. > > Maybe a stupid question, but how to do it? The signals are emitted from > the corresponding place/thread asynchronously and there is little choice > of the thread to use. At least without passing around some Object bound > to the camera manager thread, which wouldn't be nice either I suppose. > What do I miss? Not a stupid question at all, I should have made this clearer. Signals are emitted from the thread running when the emit() function is called. If a signal is connected to an object that does not inherit from the Object class, the connected slot will be called synchronously, in the same thread. If the connected object inherits from the Object class, the behaviour differs and depends on the connection type, specified when calling the connect() function on the signal: - If the connection type is ConnectionTypeDirect, the slot is called synchronously, in the same thread as the emit() calls. - If the connection type is ConnectionTypeQueued, the slot is called asynchronously, in the thread of the connected object. The emit() call returns immediately, before the slot is called. - If the connection type is ConnectionTypeBlocking, the slot is called synchronously, in the thread of the connected object. The emit() call waits until the synchronous call completes in the other thread before returning. If the emitter and receiver are in the same thread, this behaves like ConnectionTypeDirect. - If the connection type is ConnectionTypeAuto (the default), ConnectionTypeDirect is used if the emitter and receiver are in the same thread, and ConnectionTypeQueued is used otherwise. The SimpleCameraData class does not inherit from Object, so slots are called synchronously, in the thread of the emitter. This is why the inputBufferReady signal is handled with a lambda function: swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { this->conversionInputDone(buffer); }); The first argument to the connect() function is the receiver object, which here inherits from the Object class. As a result, the lambda function is called asynchronusly in the pipeline handler thread, not the soft ISP worker thread. This construct is fragile, as shown by this patch: the outputBufferReady and ispStatsReady signals were mistakenly connected to the camera data object, resulting in the slots being called from the wrong thread. I think it would be better to handle this issue in the soft ISP class, to avoid depending on the implementation of the receiver. One easy way to do so is to make the SoftwareIsp class inherit from the Object class. That way, SoftwareIsp::inputReady() will be called in the thread in which the SoftwareIsp instance lives, which is the pipeline handler thread. The inputBufferReady signal that is emitted from there will always come from the pipeline handler thread.
Hi Laurent, Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > On Fri, Jan 03, 2025 at 07:31:54PM +0100, Milan Zamazal wrote: >> Laurent Pinchart writes: >> > 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. >> >> Maybe a stupid question, but how to do it? The signals are emitted from >> the corresponding place/thread asynchronously and there is little choice >> of the thread to use. At least without passing around some Object bound >> to the camera manager thread, which wouldn't be nice either I suppose. >> What do I miss? > > Not a stupid question at all, I should have made this clearer. > > Signals are emitted from the thread running when the emit() function is > called. If a signal is connected to an object that does not inherit from > the Object class, the connected slot will be called synchronously, in > the same thread. > > If the connected object inherits from the Object class, the behaviour > differs and depends on the connection type, specified when calling the > connect() function on the signal: > > - If the connection type is ConnectionTypeDirect, the slot is called > synchronously, in the same thread as the emit() calls. > > - If the connection type is ConnectionTypeQueued, the slot is called > asynchronously, in the thread of the connected object. The emit() call > returns immediately, before the slot is called. > > - If the connection type is ConnectionTypeBlocking, the slot is called > synchronously, in the thread of the connected object. The emit() call > waits until the synchronous call completes in the other thread before > returning. If the emitter and receiver are in the same thread, this > behaves like ConnectionTypeDirect. > > - If the connection type is ConnectionTypeAuto (the default), > ConnectionTypeDirect is used if the emitter and receiver are in the > same thread, and ConnectionTypeQueued is used otherwise. > > The SimpleCameraData class does not inherit from Object, so slots are > called synchronously, in the thread of the emitter. This is why the > inputBufferReady signal is handled with a lambda function: > > swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { > this->conversionInputDone(buffer); > }); > > The first argument to the connect() function is the receiver object, > which here inherits from the Object class. As a result, the lambda > function is called asynchronusly in the pipeline handler thread, not the > soft ISP worker thread. > > This construct is fragile, as shown by this patch: the outputBufferReady > and ispStatsReady signals were mistakenly connected to the camera data > object, resulting in the slots being called from the wrong thread. I > think it would be better to handle this issue in the soft ISP class, to > avoid depending on the implementation of the receiver. One easy way to > do so is to make the SoftwareIsp class inherit from the Object class. > That way, SoftwareIsp::inputReady() will be called in the thread in > which the SoftwareIsp instance lives, which is the pipeline handler > thread. The inputBufferReady signal that is emitted from there will > always come from the pipeline handler thread. Thank you for thorough explanation. The idea of making SoftwareIsp to inherit Object looks fine and working, I'll post v2 with it.
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(-)