[RFC,v3,19/21] apps: lc-compliance: Run request completion handler in "main" thread
diff mbox series

Message ID 20250130115001.1129305-20-pobrn@protonmail.com
State New
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 30, 2025, 11:51 a.m. UTC
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(-)

Comments

Jacopo Mondi Feb. 6, 2025, 5:33 p.m. UTC | #1
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
>
>
Barnabás Pőcze Feb. 10, 2025, 8:05 a.m. UTC | #2
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
> >
> >
>

Patch
diff mbox series

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_;