Message ID | 20250114182143.1773762-17-pobrn@protonmail.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Tue, Jan 14, 2025 at 06:23:05PM +0000, Barnabás Pőcze wrote: > Currently, `Capture::requestCompleted()` runs in the `CameraManager`'s > thread. This makes it a bit more complicated to use googletest and > report errors in those callbacks since lc-compliance sets up > googletest to throw exceptions on fatal errors / test skip, but > those exceptions are only caught on the "main" thread, the one > running the test suite. > > To minimize the burden of dealing with synchronization in tests, > execute `Capture::requestCompleted()` in the event loop's thread > by utilizing `EventLoop::callLater()`. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 21 ++++++++++++--------- > src/apps/lc-compliance/helpers/capture.h | 2 +- > 2 files changed, 13 insertions(+), 10 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index dbaff4138..528a3a13a 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -129,16 +129,13 @@ void Capture::prepareRequests(std::optional<unsigned int> queueLimit) > } > } > > -int Capture::queueRequest(libcamera::Request *request) > +void Capture::queueRequest(libcamera::Request *request) > { > if (queueLimit_ && queueCount_ >= *queueLimit_) > - return 0; > - > - if (int ret = camera_->queueRequest(request); ret < 0) > - return ret; > + return; > > + ASSERT_EQ(camera_->queueRequest(request), 0); > queueCount_ += 1; > - return 0; > } > > void Capture::requestComplete(Request *request) > @@ -153,8 +150,7 @@ void Capture::requestComplete(Request *request) > << "Request didn't complete successfully"; > > request->reuse(Request::ReuseBuffers); > - if (queueRequest(request)) > - loop_->exit(-EINVAL); Per my understanding, this interrupts the loop_->exec() call and goes through the cleanup procedure in Capture::run(). If we ASSERT_EQ here, will the loop_ be interrupted and stop() be called ? Thanks j > + queueRequest(request); > } > > void Capture::start() > @@ -173,7 +169,14 @@ void Capture::start() > > ASSERT_TRUE(allocator_.allocated()); > > - camera_->requestCompleted.connect(this, &Capture::requestComplete); > + camera_->requestCompleted.connect(this, [this](libcamera::Request *request) { > + /* Runs in the CameraManager thread. */ > + > + loop_->callLater([this, request] { > + /* Run handler in the context of the event loop. */ > + requestComplete(request); > + }); > + }); > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > } > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > index 391184ad6..9003a0934 100644 > --- a/src/apps/lc-compliance/helpers/capture.h > +++ b/src/apps/lc-compliance/helpers/capture.h > @@ -30,7 +30,7 @@ private: > void stop(); > > void prepareRequests(std::optional<unsigned int> queueLimit = {}); > - int queueRequest(libcamera::Request *request); > + void queueRequest(libcamera::Request *request); > void requestComplete(libcamera::Request *request); > > std::shared_ptr<libcamera::Camera> camera_; > -- > 2.48.0 > >
2025. január 22., szerda 13:54 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > Hi Barnabás > > On Tue, Jan 14, 2025 at 06:23:05PM +0000, Barnabás Pőcze wrote: > > Currently, `Capture::requestCompleted()` runs in the `CameraManager`'s > > thread. This makes it a bit more complicated to use googletest and > > report errors in those callbacks since lc-compliance sets up > > googletest to throw exceptions on fatal errors / test skip, but > > those exceptions are only caught on the "main" thread, the one > > running the test suite. > > > > To minimize the burden of dealing with synchronization in tests, > > execute `Capture::requestCompleted()` in the event loop's thread > > by utilizing `EventLoop::callLater()`. > > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/apps/lc-compliance/helpers/capture.cpp | 21 ++++++++++++--------- > > src/apps/lc-compliance/helpers/capture.h | 2 +- > > 2 files changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > index dbaff4138..528a3a13a 100644 > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > @@ -129,16 +129,13 @@ void Capture::prepareRequests(std::optional<unsigned int> queueLimit) > > } > > } > > > > -int Capture::queueRequest(libcamera::Request *request) > > +void Capture::queueRequest(libcamera::Request *request) > > { > > if (queueLimit_ && queueCount_ >= *queueLimit_) > > - return 0; > > - > > - if (int ret = camera_->queueRequest(request); ret < 0) > > - return ret; > > + return; > > > > + ASSERT_EQ(camera_->queueRequest(request), 0); > > queueCount_ += 1; > > - return 0; > > } > > > > void Capture::requestComplete(Request *request) > > @@ -153,8 +150,7 @@ void Capture::requestComplete(Request *request) > > << "Request didn't complete successfully"; > > > > request->reuse(Request::ReuseBuffers); > > - if (queueRequest(request)) > > - loop_->exit(-EINVAL); > > Per my understanding, this interrupts the loop_->exec() call > and goes through the cleanup procedure in Capture::run(). > > If we ASSERT_EQ here, will the loop_ be interrupted and stop() be > called ? Yes, that will happen, although there was an issue in the interaction with libevent, but that is now fixed in the third version. > > Thanks > j > > > + queueRequest(request); > > } > > > > void Capture::start() > > @@ -173,7 +169,14 @@ void Capture::start() > > > > ASSERT_TRUE(allocator_.allocated()); > > > > - camera_->requestCompleted.connect(this, &Capture::requestComplete); > > + camera_->requestCompleted.connect(this, [this](libcamera::Request *request) { > > + /* Runs in the CameraManager thread. */ > > + > > + loop_->callLater([this, request] { > > + /* Run handler in the context of the event loop. */ > > + requestComplete(request); > > + }); > > + }); > > > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > > } > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > index 391184ad6..9003a0934 100644 > > --- a/src/apps/lc-compliance/helpers/capture.h > > +++ b/src/apps/lc-compliance/helpers/capture.h > > @@ -30,7 +30,7 @@ private: > > void stop(); > > > > void prepareRequests(std::optional<unsigned int> queueLimit = {}); > > - int queueRequest(libcamera::Request *request); > > + void queueRequest(libcamera::Request *request); > > void requestComplete(libcamera::Request *request); > > > > std::shared_ptr<libcamera::Camera> camera_; > > -- > > 2.48.0 > > > > >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index dbaff4138..528a3a13a 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -129,16 +129,13 @@ void Capture::prepareRequests(std::optional<unsigned int> queueLimit) } } -int Capture::queueRequest(libcamera::Request *request) +void Capture::queueRequest(libcamera::Request *request) { if (queueLimit_ && queueCount_ >= *queueLimit_) - return 0; - - if (int ret = camera_->queueRequest(request); ret < 0) - return ret; + return; + ASSERT_EQ(camera_->queueRequest(request), 0); queueCount_ += 1; - return 0; } void Capture::requestComplete(Request *request) @@ -153,8 +150,7 @@ void Capture::requestComplete(Request *request) << "Request didn't complete successfully"; request->reuse(Request::ReuseBuffers); - if (queueRequest(request)) - loop_->exit(-EINVAL); + queueRequest(request); } void Capture::start() @@ -173,7 +169,14 @@ void Capture::start() ASSERT_TRUE(allocator_.allocated()); - camera_->requestCompleted.connect(this, &Capture::requestComplete); + camera_->requestCompleted.connect(this, [this](libcamera::Request *request) { + /* Runs in the CameraManager thread. */ + + loop_->callLater([this, request] { + /* Run handler in the context of the event loop. */ + requestComplete(request); + }); + }); ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; } diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h index 391184ad6..9003a0934 100644 --- a/src/apps/lc-compliance/helpers/capture.h +++ b/src/apps/lc-compliance/helpers/capture.h @@ -30,7 +30,7 @@ private: void stop(); void prepareRequests(std::optional<unsigned int> queueLimit = {}); - int queueRequest(libcamera::Request *request); + void queueRequest(libcamera::Request *request); void requestComplete(libcamera::Request *request); std::shared_ptr<libcamera::Camera> camera_;
Currently, `Capture::requestCompleted()` runs in the `CameraManager`'s thread. This makes it a bit more complicated to use googletest and report errors in those callbacks since lc-compliance sets up googletest to throw exceptions on fatal errors / test skip, but those exceptions are only caught on the "main" thread, the one running the test suite. To minimize the burden of dealing with synchronization in tests, execute `Capture::requestCompleted()` in the event loop's thread by utilizing `EventLoop::callLater()`. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/apps/lc-compliance/helpers/capture.cpp | 21 ++++++++++++--------- src/apps/lc-compliance/helpers/capture.h | 2 +- 2 files changed, 13 insertions(+), 10 deletions(-)