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

[Jacopo Mondi]

Hi Laurent,

On Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:
> 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.
>

Good idea!
I'll add start() and stop()

> > +
> > +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.
>

mmm, so Thread exposes a start() and an exit() functions.

start() runs the underlying thread and make the connected event dispatcher
process events. exit() interrupts the event dispatcher only, then the
thread will be actually stopped at destruction time.

As we don't run events in the notifier, but rather poll manually here,
is there any value in calling exit() ?

> > +}
> > +
> > +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.

You got me... Copyed from the Rockchip HAL implementation.
Quite arbitrary indeed!

>
> > +	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.
>

I considered several options here, including using a custom event
notifier that accepts a set of fds instead of a single one to exploit
our notifier/dispatcher infrastructure. Unfortunately, that would
require manualy tracking of the number of completed fences here, or
the implementation of a custom dispatcher that works on a set of fds
and only signal completion when all of them are done.

At the same time I considered instrumenting poll to work with multiple
fds instead of going through a syscall for each one of them. In this
case too, manual tracking of the number of completed requests would be
required, and once I considered that we deal with up to 3 buffers, and
most of the time with a single one, I considered writing such an
error-prone and quite-harsh to debug code not 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.
>

To be honest I don't mind the intruduction of CaptureRequest in the
CameraDevice, the rest I agree, it's awful.

A proper library-wide solution is needed, but this closes a gap and
does not introduce any techincal debt

So I do as well think that:
> Overall I think this is totally fine for now.
>