[libcamera-devel] [RFC 1/2] android: camera_worker: Introduce CameraWorker

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Oct 06, 2020 at 06:06:36PM +0200, Jacopo Mondi wrote:
> The CameraWork class creates a Worker instance and runs on an internal

s/CameraWork/CameraWorker/

> thread. The worker waits on a set of fences before queueing a capture
> requests to the the libcamera::Camera.

s/the the/the/

You may want to copy some of the text from the cover letter to explain
why this is needed.

> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_worker.cpp | 67 +++++++++++++++++++++++++++++++
>  src/android/camera_worker.h   | 75 +++++++++++++++++++++++++++++++++++
>  src/android/meson.build       |  1 +
>  3 files changed, 143 insertions(+)
>  create mode 100644 src/android/camera_worker.cpp
>  create mode 100644 src/android/camera_worker.h
> 
> diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> new file mode 100644
> index 000000000000..2bb94775a195
> --- /dev/null
> +++ b/src/android/camera_worker.cpp
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_worker.cpp - Process capture request on behalf of the Camera HAL
> + */
> +
> +#include "camera_worker.h"
> +
> +#include <errno.h>
> +#include <sys/poll.h>
> +
> +#include "camera_device.h"
> +
> +using namespace libcamera;
> +
> +CameraWorker::CameraWorker(const std::shared_ptr<Camera> &camera)
> +	: camera_(camera), worker_(camera_.get())
> +{
> +	worker_.moveToThread(&thread_);
> +	thread_.start();
> +}

You need to stop the thread before deleting it. I would actually expose
start and stop through the API, as there's no need to have a thread
running when the camera is closed.

> +
> +void CameraWorker::queueRequest(std::unique_ptr<CaptureRequest> request)
> +{
> +	CaptureRequest *req = request.release();
> +	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);

Another important reason to stop the thread manually is to make sure to
synchronize its message queue, as we want to guarantee that all requests
will be freed.

> +}
> +
> +int CameraWorker::Worker::waitFence(int fence)
> +{
> +	struct pollfd fds = { fence, POLLIN, 0 };
> +	constexpr unsigned int timeoutMs = 300;

That's rather arbitrary, I wonder how we should pick the timeout, but
for now it should be good enough.

> +	int ret;
> +
> +	do {
> +		ret = poll(&fds, 1, timeoutMs);
> +		if (ret == 0)
> +			return -ETIME;
> +
> +		if (ret > 0) {
> +			if (fds.revents & (POLLERR | POLLNVAL))
> +				return -EINVAL;
> +
> +			return 0;
> +		}
> +	} while (ret == -1 && (errno == EINTR || errno == EAGAIN));
> +
> +	return ret;
> +}
> +
> +void CameraWorker::Worker::processRequest(CaptureRequest *request)
> +{
> +	for (int fence : request->acquireFences_) {
> +		if (fence == -1)
> +			continue;
> +
> +		int ret = waitFence(fence);

It would be possible to optimize this by waiting on all the fences at
the same time to minimize syscalls when multiple fences are ready at the
same time, but that may not be worth it.

> +		if (ret < 0)
> +			return;
> +
> +		close(fence);
> +	}
> +
> +	camera_->queueRequest(request->request_);
> +	delete request;
> +}
> diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> new file mode 100644
> index 000000000000..24b176d6269c
> --- /dev/null
> +++ b/src/android/camera_worker.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Google Inc.
> + *
> + * camera_worker.h - Process capture request on behalf of the Camera HAL
> + */
> +#ifndef __ANDROID_CAMERA_WORKER_H__
> +#define __ANDROID_CAMERA_WORKER_H__
> +
> +#include <memory>
> +
> +#include <libcamera/buffer.h>
> +#include <libcamera/camera.h>
> +#include <libcamera/object.h>
> +#include <libcamera/request.h>
> +#include <libcamera/stream.h>
> +
> +#include "libcamera/internal/thread.h"
> +
> +class CameraDevice;
> +
> +struct CaptureRequest {
> +	CaptureRequest(libcamera::Request *request)
> +		: request_(request)
> +	{
> +	}
> +
> +	void addBuffer(libcamera::Stream *stream,
> +		       libcamera::FrameBuffer *buffer, int fence)
> +	{
> +		request_->addBuffer(stream, buffer);
> +		acquireFences_.push_back(fence);
> +	}

The API isn't amazing, but that's because the best solution will be to
add fences support to the FrameBuffer class. Until we do so, this
workaround is fine.

Overall I think this is totally fine for now.

> +
> +private:
> +	friend class CameraWorker;
> +
> +	std::vector<int> acquireFences_;
> +	libcamera::Request *request_;
> +};
> +
> +class CameraWorker
> +{
> +public:
> +	CameraWorker(const std::shared_ptr<libcamera::Camera> &camera);
> +
> +	void queueRequest(std::unique_ptr<CaptureRequest> request);
> +
> +private:
> +	class Worker : public libcamera::Object
> +	{
> +	public:
> +		Worker(libcamera::Camera *camera)
> +			: camera_(camera)
> +		{
> +		}
> +		~Worker()
> +		{
> +		}
> +
> +		void processRequest(CaptureRequest *request);
> +
> +	private:
> +		int waitFence(int fence);
> +
> +		libcamera::Camera *camera_;
> +	};
> +
> +	std::shared_ptr<libcamera::Camera> camera_;
> +
> +	Worker worker_;
> +	libcamera::Thread thread_;
> +};
> +
> +#endif /* __ANDROID_CAMERA_WORKER_H__ */
> diff --git a/src/android/meson.build b/src/android/meson.build
> index 802bb89afe57..b2b2293cf62d 100644
> --- a/src/android/meson.build
> +++ b/src/android/meson.build
> @@ -21,6 +21,7 @@ android_hal_sources = files([
>      'camera_metadata.cpp',
>      'camera_ops.cpp',
>      'camera_stream.cpp',
> +    'camera_worker.cpp',
>      'jpeg/encoder_libjpeg.cpp',
>      'jpeg/exif.cpp',
>  ])

-- 
Regards,

Laurent Pinchart