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

Message ID 20250114182143.1773762-17-pobrn@protonmail.com
State Superseded
Headers show
Series
  • apps: lc-compliance: Multi-stream tests
Related show

Commit Message

Barnabás Pőcze Jan. 14, 2025, 6:23 p.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 | 21 ++++++++++++---------
 src/apps/lc-compliance/helpers/capture.h   |  2 +-
 2 files changed, 13 insertions(+), 10 deletions(-)

Comments

Jacopo Mondi Jan. 22, 2025, 12:54 p.m. UTC | #1
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
>
>
Barnabás Pőcze Jan. 30, 2025, 2:46 p.m. UTC | #2
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
> >
> >
>

Patch
diff mbox series

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