Message ID | 20250130115001.1129305-20-pobrn@protonmail.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
Hi Barnabás On Thu, Jan 30, 2025 at 11:51:41AM +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> You mentioned an issue with libevent interaction in v2 ? What is it and how has it been fixed ? The patch itself looks good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > src/apps/lc-compliance/helpers/capture.cpp | 24 +++++++++++++--------- > src/apps/lc-compliance/helpers/capture.h | 2 +- > 2 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 4a8627662..5032470d9 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -127,17 +127,13 @@ void Capture::prepareRequests() > } > } > > -int Capture::queueRequest(libcamera::Request *request) > +void Capture::queueRequest(libcamera::Request *request) > { > if (queueLimit_ && queueCount_ >= *queueLimit_) > - return 0; > - > - int ret = camera_->queueRequest(request); > - if (ret < 0) > - return ret; > + return; > > + ASSERT_EQ(camera_->queueRequest(request), 0); > queueCount_ += 1; > - return 0; > } > > void Capture::requestComplete(Request *request) > @@ -152,8 +148,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() > @@ -172,7 +167,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); > + }, reinterpret_cast<std::uintptr_t>(this)); > + }); > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > } > @@ -188,6 +190,8 @@ void Capture::stop() > > requests_.clear(); > > + loop_->cancelLater(reinterpret_cast<std::uintptr_t>(this)); > + > for (const auto &cfg : *config_) { > int ret = allocator_.free(cfg.stream()); > EXPECT_EQ(ret, 0) << "Failed to free buffers associated with stream"; > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > index 48a8dadcb..dacce1fe2 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(); > - int queueRequest(libcamera::Request *request); > + void queueRequest(libcamera::Request *request); > void requestComplete(libcamera::Request *request); > > std::shared_ptr<libcamera::Camera> camera_; > -- > 2.48.1 > >
Hi 2025. február 6., csütörtök 18:33 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > Hi Barnabás > > On Thu, Jan 30, 2025 at 11:51:41AM +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> > > You mentioned an issue with libevent interaction in v2 ? What is it > and how has it been fixed ? An exception cannot leave an event callback otherwise libevent will enter an inconsistent state. This was addressed by catching exceptions from deferred calls, breaking the event loop, and then rethrowing the exception from `EventLoop::exec()`. > > The patch itself looks good to me > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > --- > > src/apps/lc-compliance/helpers/capture.cpp | 24 +++++++++++++--------- > > src/apps/lc-compliance/helpers/capture.h | 2 +- > > 2 files changed, 15 insertions(+), 11 deletions(-) > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > index 4a8627662..5032470d9 100644 > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > @@ -127,17 +127,13 @@ void Capture::prepareRequests() > > } > > } > > > > -int Capture::queueRequest(libcamera::Request *request) > > +void Capture::queueRequest(libcamera::Request *request) > > { > > if (queueLimit_ && queueCount_ >= *queueLimit_) > > - return 0; > > - > > - int ret = camera_->queueRequest(request); > > - if (ret < 0) > > - return ret; > > + return; > > > > + ASSERT_EQ(camera_->queueRequest(request), 0); > > queueCount_ += 1; > > - return 0; > > } > > > > void Capture::requestComplete(Request *request) > > @@ -152,8 +148,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() > > @@ -172,7 +167,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); > > + }, reinterpret_cast<std::uintptr_t>(this)); > > + }); > > > > ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; > > } > > @@ -188,6 +190,8 @@ void Capture::stop() > > > > requests_.clear(); > > > > + loop_->cancelLater(reinterpret_cast<std::uintptr_t>(this)); > > + > > for (const auto &cfg : *config_) { > > int ret = allocator_.free(cfg.stream()); > > EXPECT_EQ(ret, 0) << "Failed to free buffers associated with stream"; > > diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h > > index 48a8dadcb..dacce1fe2 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(); > > - int queueRequest(libcamera::Request *request); > > + void queueRequest(libcamera::Request *request); > > void requestComplete(libcamera::Request *request); > > > > std::shared_ptr<libcamera::Camera> camera_; > > -- > > 2.48.1 > > > > >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index 4a8627662..5032470d9 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -127,17 +127,13 @@ void Capture::prepareRequests() } } -int Capture::queueRequest(libcamera::Request *request) +void Capture::queueRequest(libcamera::Request *request) { if (queueLimit_ && queueCount_ >= *queueLimit_) - return 0; - - int ret = camera_->queueRequest(request); - if (ret < 0) - return ret; + return; + ASSERT_EQ(camera_->queueRequest(request), 0); queueCount_ += 1; - return 0; } void Capture::requestComplete(Request *request) @@ -152,8 +148,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() @@ -172,7 +167,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); + }, reinterpret_cast<std::uintptr_t>(this)); + }); ASSERT_EQ(camera_->start(), 0) << "Failed to start camera"; } @@ -188,6 +190,8 @@ void Capture::stop() requests_.clear(); + loop_->cancelLater(reinterpret_cast<std::uintptr_t>(this)); + for (const auto &cfg : *config_) { int ret = allocator_.free(cfg.stream()); EXPECT_EQ(ret, 0) << "Failed to free buffers associated with stream"; diff --git a/src/apps/lc-compliance/helpers/capture.h b/src/apps/lc-compliance/helpers/capture.h index 48a8dadcb..dacce1fe2 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(); - 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 | 24 +++++++++++++--------- src/apps/lc-compliance/helpers/capture.h | 2 +- 2 files changed, 15 insertions(+), 11 deletions(-)