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

[Laurent Pinchart]

Hi Jacopo,

On Thu, Oct 08, 2020 at 10:36:24AM +0200, Jacopo Mondi wrote:
> On Thu, Oct 08, 2020 at 07:05:22AM +0300, Laurent Pinchart wrote:
> > 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() ?

Unless the Thread::run() method gets overridden (which isn't he case
here), it calls Thread::exec(), which runs an event loop. Thread::exit()
instructs the event loop to exit, which is required, otherwise the
thread will never stop and Thread::wait() will block forever.
Thread::wait() is also mandatory to call before destroying the Thread
object.

We can't override Thread::run() to not call exec(), as we rely on the
event loop. That's how the messages used by Object::invokeMethod() are
dispatched.

The conclusion is that we need to call exit() and wait() before
destroying the thread (this should happen in CameraWorker::stop()). We
also need to make sure that all messages posted to the thread's message
queue by worker_.invokeMethod() are handled before the thread is
stopped. There are two sides to it:

- We need to ensure that no messages get posted after calling
  Thread::exit(). This means not calling CameraWorker::queueRequest()
  after CameraWorker::stop(). Given that both functions are called from
  the camera service thread there should be no race condition, and given
  that stop() will be called when the camera is closed, queueRequest()
  won't be called anymore. I think we're safe here.

- I believe there's a race condition in the thread class. Even if
  Thread::exit() and Thread::postMessage() (the latter called from
  Object::invokeMethod) are call from a single thread, the message could
  be added to the queue, and exit() could instruct Thread::exect() to
  return before the event dispatcher gets a change to dispatch messages.
  Thread::exec() contains

	while (!data_->exit_.load(std::memory_order_acquire))
		dispatcher->processEvents();

  and EventDispatcherPoll::processEvents() calls
  Thread::dispatchMessages() at the very beginning, before sleeping.

  We don't implement flush() in the HAL, which I think would be related
  to the solution to this issue.

  Am I overthinking this, or is it a real issue ?

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

Maybe a \todo would help remembering the task ?

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

Ack.

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

It's not awful :-) It's just sub-optimal, and that's fine. I think
you've struct the right balance, the implementation does the job, it's
clean and lean, not over-designed, and probably as good as it can get
until this gets moved to the libcamera core.

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

-- 
Regards,

Laurent Pinchart