| 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 >
On 29.08.25 11:03, Barnabás Pőcze wrote: > 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. I had another look and can confirm that moving debayer_->moveToThread(&ispWorkerThread_); back to the constructor (leaving `ispWorkerThread_.start();` and the later added `debayer_->invokeMethod()` in place) solves the issue here and makes the branch run pretty robust for me. Regarding the described sequence: I guess another option could be to call `ispWorkerThread_.start();` in both `configure()` and `start()` as repeated calls get ignored. IIUC that should only have negative consequences - in the form of minor wasted resources - if a successfull `configure()` is never followed up with a `start()`. P.S.: I added a fixup commit to https://gitlab.freedesktop.org/rmader/libcamera/-/commits/gpuisp-v2-buffer-import in case anybody wants to try this with PW. P.S.S.: on a Librem5 - when combined with upcoming PW, GTK, Snapshot, Mesa and forcing on GLES 3.0 - this can almost rival Millipixels performance wise already - really really cool! Regards, Robert > > 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(-)