| Message ID | 20250824-b4-v0-5-2-gpuisp-v2-a-v2-12-96f4576c814e@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi, On 24.08.25 02:48, Bryan O'Donoghue wrote: > EGL is not thread-safe and in fact associates invisible handles with the > threadid of the calling context. > > As a result we need to make Deabyer::configure() and Debayer::process() in SoftISP execute on > the same thread. > > When we call Debayer::configure() in the egl class this will setup and egl context > for us which is associated with the calling thread context. Hence when > Debayer::process(); runs it must run in the same thread as > Debayer::configure() or the hidden Gegl context handles will not point > to the same place. > > Move start thread into configure() as a first step towards this. > > Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> > --- > src/libcamera/software_isp/software_isp.cpp | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 28e2a360e7e6d5071e7b97c2bffc60478d245a60..7bee8f0681dd5cedcd81950ed49f611b514e763d 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -159,8 +159,6 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > metadataReady.emit(frame, metadata); > }); > ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls); > - > - debayer_->moveToThread(&ispWorkerThread_); > } > > SoftwareIsp::~SoftwareIsp() > @@ -262,6 +260,9 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, > if (ret < 0) > return ret; > > + debayer_->moveToThread(&ispWorkerThread_); When testing with my OnePlus6 via Pipewire/Snapshot, I reliably hit an assert here whenever starting a camera for the second time. I.e. when switching to another camera and back, or when closing Snapshot (or some other app) and starting it again I get the following crash/bt (every time): (gdb) bt #0 __restore_sigs (set=set@entry=0xffff8f465b40) at ./arch/aarch64/syscall_arch.h:48 #1 0x0000ffff92e71e6c in raise (sig=sig@entry=6) at src/signal/raise.c:11 #2 0x0000ffff92e40628 in abort () at src/exit/abort.c:11 #3 0x0000ffff9195c244 in libcamera::LogMessage::~LogMessage (this=0xffff8f465c38) at ../src/libcamera/base/log.cpp:867 #4 0x0000ffff91952348 in libcamera::Object::assertThreadBound (this=this@entry=0xffff8f4266a0, message=message@entry=0xffff91961638 "Object can't be moved from another thread") at ../src/libcamera/base/object.cpp:253 #5 0x0000ffff91952864 in libcamera::Object::moveToThread (this=0xffff8f4266a0, thread=thread@entry=0xffff8f431750) at ../src/libcamera/base/object.cpp:308 #6 0x0000ffff91ab78d4 in libcamera::SoftwareIsp::configure (this=0xffff8f431680, inputCfg=..., outputCfgs=..., configInfo=...) at ../src/libcamera/software_isp/software_isp.cpp:280 #7 0x0000ffff91a93564 in libcamera::SimplePipelineHandler::configure (this=<optimized out>, camera=<optimized out>, c=<optimized out>) at /usr/include/c++/15.2.0/bits/unique_ptr.h:193 #8 0x0000ffff91a17620 in libcamera::BoundMethodArgs<int, libcamera::Camera*, libcamera::CameraConfiguration*>::invokePack<0ul, 1ul> (this=<optimized out>, pack=0xffff85436b20) at ../include/libcamera/base/bound_method.h:100 #9 libcamera::BoundMethodArgs<int, libcamera::Camera*, libcamera::CameraConfiguration*>::invokePack (this=<optimized out>, pack=0xffff85436b20) at ../include/libcamera/base/bound_method.h:111 #10 0x0000ffff91951ff8 in libcamera::Object::message (this=<optimized out>, msg=<optimized out>) at ../src/libcamera/base/object.cpp:211 #11 libcamera::Object::message (this=<optimized out>, msg=<optimized out>) at ../src/libcamera/base/object.cpp:201 #12 0x0000ffff9195e13c in libcamera::Thread::dispatchMessages (this=0xffff91b4fb70, type=type@entry=libcamera::Message::None, receiver=receiver@entry=0x0) at ../src/libcamera/base/thread.cpp:654 #13 0x0000ffff9195625c in libcamera::EventDispatcherPoll::processEvents (this=0xffff91e02780) at ../src/libcamera/base/event_dispatcher_poll.cpp:146 #14 0x0000ffff9195d928 in libcamera::Thread::exec (this=this@entry=0xffff91b4fb70) at ../src/libcamera/base/thread.cpp:311 #15 0x0000ffff91a1a940 in libcamera::CameraManager::Private::run (this=0xffff91b4fb60) at ../src/libcamera/camera_manager.cpp:89 #16 0x0000ffff916f557c in ?? () from /usr/lib/libstdc++.so.6 #17 0x0000ffff92e82534 in start (p=0xffff8f466dd0) at src/thread/pthread_create.c:207 #18 0x0000ffff92e8092c in __clone () at src/thread/aarch64/clone.s:28 and the logs ERROR Object object.cpp:252 Object can't be moved from another thread FATAL default object.cpp:253 assertion "false" failed in assertThreadBound() Given that this is common code this applies independently of whether LIBCAMERA_SOFTISP_MODE=gpu is set. > + ispWorkerThread_.start(); > + > return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); > } > > @@ -343,7 +344,6 @@ int SoftwareIsp::start() > if (ret) > return ret; > > - ispWorkerThread_.start(); > return 0; > } > >
2025. 08. 29. 10:50 keltezéssel, Robert Mader írta: > Hi, > > On 24.08.25 02:48, Bryan O'Donoghue wrote: >> EGL is not thread-safe and in fact associates invisible handles with the >> threadid of the calling context. >> >> As a result we need to make Deabyer::configure() and Debayer::process() in SoftISP execute on >> the same thread. >> >> When we call Debayer::configure() in the egl class this will setup and egl context >> for us which is associated with the calling thread context. Hence when >> Debayer::process(); runs it must run in the same thread as >> Debayer::configure() or the hidden Gegl context handles will not point >> to the same place. >> >> Move start thread into configure() as a first step towards this. >> >> Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> >> --- >> src/libcamera/software_isp/software_isp.cpp | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 28e2a360e7e6d5071e7b97c2bffc60478d245a60..7bee8f0681dd5cedcd81950ed49f611b514e763d 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -159,8 +159,6 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> metadataReady.emit(frame, metadata); >> }); >> ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls); >> - >> - debayer_->moveToThread(&ispWorkerThread_); >> } >> >> SoftwareIsp::~SoftwareIsp() >> @@ -262,6 +260,9 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, >> if (ret < 0) >> return ret; >> >> + debayer_->moveToThread(&ispWorkerThread_); > > When testing with my OnePlus6 via Pipewire/Snapshot, I reliably hit an assert here whenever starting a camera for the second time. I.e. when switching to another camera and back, or when closing Snapshot (or some other app) and starting it again I get the following crash/bt (every time): > > (gdb) bt > #0 __restore_sigs (set=set@entry=0xffff8f465b40) at ./arch/aarch64/syscall_arch.h:48 > #1 0x0000ffff92e71e6c in raise (sig=sig@entry=6) at src/signal/raise.c:11 > #2 0x0000ffff92e40628 in abort () at src/exit/abort.c:11 > #3 0x0000ffff9195c244 in libcamera::LogMessage::~LogMessage (this=0xffff8f465c38) at ../src/libcamera/base/log.cpp:867 > #4 0x0000ffff91952348 in libcamera::Object::assertThreadBound (this=this@entry=0xffff8f4266a0, message=message@entry=0xffff91961638 "Object can't be moved from another thread") at ../src/libcamera/base/object.cpp:253 > #5 0x0000ffff91952864 in libcamera::Object::moveToThread (this=0xffff8f4266a0, thread=thread@entry=0xffff8f431750) at ../src/libcamera/base/object.cpp:308 > #6 0x0000ffff91ab78d4 in libcamera::SoftwareIsp::configure (this=0xffff8f431680, inputCfg=..., outputCfgs=..., configInfo=...) at ../src/libcamera/software_isp/software_isp.cpp:280 > #7 0x0000ffff91a93564 in libcamera::SimplePipelineHandler::configure (this=<optimized out>, camera=<optimized out>, c=<optimized out>) at /usr/include/c++/15.2.0/bits/unique_ptr.h:193 > #8 0x0000ffff91a17620 in libcamera::BoundMethodArgs<int, libcamera::Camera*, libcamera::CameraConfiguration*>::invokePack<0ul, 1ul> (this=<optimized out>, pack=0xffff85436b20) at ../include/libcamera/base/bound_method.h:100 > #9 libcamera::BoundMethodArgs<int, libcamera::Camera*, libcamera::CameraConfiguration*>::invokePack (this=<optimized out>, pack=0xffff85436b20) at ../include/libcamera/base/bound_method.h:111 > #10 0x0000ffff91951ff8 in libcamera::Object::message (this=<optimized out>, msg=<optimized out>) at ../src/libcamera/base/object.cpp:211 > #11 libcamera::Object::message (this=<optimized out>, msg=<optimized out>) at ../src/libcamera/base/object.cpp:201 > #12 0x0000ffff9195e13c in libcamera::Thread::dispatchMessages (this=0xffff91b4fb70, type=type@entry=libcamera::Message::None, receiver=receiver@entry=0x0) at ../src/libcamera/base/thread.cpp:654 > #13 0x0000ffff9195625c in libcamera::EventDispatcherPoll::processEvents (this=0xffff91e02780) at ../src/libcamera/base/event_dispatcher_poll.cpp:146 > #14 0x0000ffff9195d928 in libcamera::Thread::exec (this=this@entry=0xffff91b4fb70) at ../src/libcamera/base/thread.cpp:311 > #15 0x0000ffff91a1a940 in libcamera::CameraManager::Private::run (this=0xffff91b4fb60) at ../src/libcamera/camera_manager.cpp:89 > #16 0x0000ffff916f557c in ?? () from /usr/lib/libstdc++.so.6 > #17 0x0000ffff92e82534 in start (p=0xffff8f466dd0) at src/thread/pthread_create.c:207 > #18 0x0000ffff92e8092c in __clone () at src/thread/aarch64/clone.s:28 > > and the logs > > ERROR Object object.cpp:252 Object can't be moved from another thread > FATAL default object.cpp:253 assertion "false" failed in assertThreadBound() A thread can only be moved from its current thread. So the when this code runs the second time, `*debayer_` object is already on `ispWorkerThread_`, which is not the current thread, hence the assertion. It's not clear to me why moving the object in the constructor is not sufficient, I think just adding `ispWorkerThread_.start()` in `SoftwareIsp::configure()` should be sufficient. Removing it from `SoftwareIsp::start()` is also not quite right because then a configure-start-stop-start-stop sequence won't work as expected as far as I can tell. Or maybe the thread can be started in the constructor and kept running. Regards, Barnabás Pőcze > > Given that this is common code this applies independently of whether > > LIBCAMERA_SOFTISP_MODE=gpu > > is set. > >> + ispWorkerThread_.start(); >> + >> return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); >> } >> >> @@ -343,7 +344,6 @@ int SoftwareIsp::start() >> if (ret) >> return ret; >> >> - ispWorkerThread_.start(); >> return 0; >> } >> >> > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718 >
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 28e2a360e7e6d5071e7b97c2bffc60478d245a60..7bee8f0681dd5cedcd81950ed49f611b514e763d 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -159,8 +159,6 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, metadataReady.emit(frame, metadata); }); ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls); - - debayer_->moveToThread(&ispWorkerThread_); } SoftwareIsp::~SoftwareIsp() @@ -262,6 +260,9 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg, if (ret < 0) return ret; + debayer_->moveToThread(&ispWorkerThread_); + ispWorkerThread_.start(); + return debayer_->configure(inputCfg, outputCfgs, ccmEnabled_); } @@ -343,7 +344,6 @@ int SoftwareIsp::start() if (ret) return ret; - ispWorkerThread_.start(); return 0; }
EGL is not thread-safe and in fact associates invisible handles with the threadid of the calling context. As a result we need to make Deabyer::configure() and Debayer::process() in SoftISP execute on the same thread. When we call Debayer::configure() in the egl class this will setup and egl context for us which is associated with the calling thread context. Hence when Debayer::process(); runs it must run in the same thread as Debayer::configure() or the hidden Gegl context handles will not point to the same place. Move start thread into configure() as a first step towards this. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/software_isp.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)