| Message ID | 20250131191838.47661-1-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Fri, Jan 31, 2025 at 08:18:38PM +0100, Milan Zamazal wrote: > inputBufferReady ready signal in the simple pipeline is handled in the > pipeline handler thread. outputBufferReady and ispStatsReady signals > should be handled there too. > > Rather than relying on the user of the SoftwareIsp instance, let > SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the > signals above are emitted from signal handlers. This means that if > SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp > thread. Which is the camera manager thread because the SoftwareIsp > instance is created there. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../internal/software_isp/software_isp.h | 3 ++- > src/libcamera/pipeline/simple/simple.cpp | 19 ++++++------------- > 2 files changed, 8 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index d51b03fd..440a296d 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -18,6 +18,7 @@ > > #include <libcamera/base/class.h> > #include <libcamera/base/log.h> > +#include <libcamera/base/object.h> > #include <libcamera/base/signal.h> > #include <libcamera/base/thread.h> > > @@ -43,7 +44,7 @@ struct StreamConfiguration; > > LOG_DECLARE_CATEGORY(SoftwareIsp) > > -class SoftwareIsp > +class SoftwareIsp : public Object > { > public: > SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 8ac24e6e..fade8fda 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -538,20 +538,13 @@ int SimpleCameraData::init() > swIsp_.reset(); > } else { > /* > - * The inputBufferReady signal is emitted from the soft ISP thread, > - * and needs to be handled in the pipeline handler thread. Signals > - * implement queued delivery, but this works transparently only if > - * the receiver is bound to the target thread. As the > - * SimpleCameraData class doesn't inherit from the Object class, it > - * is not bound to any thread, and the signal would be delivered > - * synchronously. Instead, connect the signal to a lambda function > - * bound explicitly to the pipe, which is bound to the pipeline > - * handler thread. The function then simply forwards the call to > - * conversionInputDone(). > + * The connected signals should be handled by the camera manager > + * thread. This method is called in the camera manager thread and > + * instantiates the SoftwareIsp instance, which inherits Object and > + * emits the signals from the instance's own signal handlers; thus > + * the slots here are invoked in the camera manager thread. > */ I think you can drop this comment complete. With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > - swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { > - this->conversionInputDone(buffer); > - }); > + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); > swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); > swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); > swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Fri, Jan 31, 2025 at 08:18:38PM +0100, Milan Zamazal wrote: >> inputBufferReady ready signal in the simple pipeline is handled in the >> pipeline handler thread. outputBufferReady and ispStatsReady signals >> should be handled there too. >> >> Rather than relying on the user of the SoftwareIsp instance, let >> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the >> signals above are emitted from signal handlers. This means that if >> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp >> thread. Which is the camera manager thread because the SoftwareIsp >> instance is created there. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../internal/software_isp/software_isp.h | 3 ++- >> src/libcamera/pipeline/simple/simple.cpp | 19 ++++++------------- >> 2 files changed, 8 insertions(+), 14 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index d51b03fd..440a296d 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -18,6 +18,7 @@ >> >> #include <libcamera/base/class.h> >> #include <libcamera/base/log.h> >> +#include <libcamera/base/object.h> >> #include <libcamera/base/signal.h> >> #include <libcamera/base/thread.h> >> >> @@ -43,7 +44,7 @@ struct StreamConfiguration; >> >> LOG_DECLARE_CATEGORY(SoftwareIsp) >> >> -class SoftwareIsp >> +class SoftwareIsp : public Object >> { >> public: >> SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 8ac24e6e..fade8fda 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -538,20 +538,13 @@ int SimpleCameraData::init() >> swIsp_.reset(); >> } else { >> /* >> - * The inputBufferReady signal is emitted from the soft ISP thread, >> - * and needs to be handled in the pipeline handler thread. Signals >> - * implement queued delivery, but this works transparently only if >> - * the receiver is bound to the target thread. As the >> - * SimpleCameraData class doesn't inherit from the Object class, it >> - * is not bound to any thread, and the signal would be delivered >> - * synchronously. Instead, connect the signal to a lambda function >> - * bound explicitly to the pipe, which is bound to the pipeline >> - * handler thread. The function then simply forwards the call to >> - * conversionInputDone(). >> + * The connected signals should be handled by the camera manager >> + * thread. This method is called in the camera manager thread and >> + * instantiates the SoftwareIsp instance, which inherits Object and >> + * emits the signals from the instance's own signal handlers; thus >> + * the slots here are invoked in the camera manager thread. >> */ > > I think you can drop this comment complete. With that, OK, posted v3 with this change. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> - swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { >> - this->conversionInputDone(buffer); >> - }); >> + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); >> swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); >> swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); >> swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index d51b03fd..440a296d 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -18,6 +18,7 @@ #include <libcamera/base/class.h> #include <libcamera/base/log.h> +#include <libcamera/base/object.h> #include <libcamera/base/signal.h> #include <libcamera/base/thread.h> @@ -43,7 +44,7 @@ struct StreamConfiguration; LOG_DECLARE_CATEGORY(SoftwareIsp) -class SoftwareIsp +class SoftwareIsp : public Object { public: SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 8ac24e6e..fade8fda 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -538,20 +538,13 @@ int SimpleCameraData::init() swIsp_.reset(); } else { /* - * The inputBufferReady signal is emitted from the soft ISP thread, - * and needs to be handled in the pipeline handler thread. Signals - * implement queued delivery, but this works transparently only if - * the receiver is bound to the target thread. As the - * SimpleCameraData class doesn't inherit from the Object class, it - * is not bound to any thread, and the signal would be delivered - * synchronously. Instead, connect the signal to a lambda function - * bound explicitly to the pipe, which is bound to the pipeline - * handler thread. The function then simply forwards the call to - * conversionInputDone(). + * The connected signals should be handled by the camera manager + * thread. This method is called in the camera manager thread and + * instantiates the SoftwareIsp instance, which inherits Object and + * emits the signals from the instance's own signal handlers; thus + * the slots here are invoked in the camera manager thread. */ - swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) { - this->conversionInputDone(buffer); - }); + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone); swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone); swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady); swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);
inputBufferReady ready signal in the simple pipeline is handled in the pipeline handler thread. outputBufferReady and ispStatsReady signals should be handled there too. Rather than relying on the user of the SoftwareIsp instance, let SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the signals above are emitted from signal handlers. This means that if SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp thread. Which is the camera manager thread because the SoftwareIsp instance is created there. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../internal/software_isp/software_isp.h | 3 ++- src/libcamera/pipeline/simple/simple.cpp | 19 ++++++------------- 2 files changed, 8 insertions(+), 14 deletions(-)