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

[Jacopo Mondi]

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 at 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 at 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
>
>