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

[Niklas Söderlund]

Hi Jacopo,

On 2020-10-07 15:32:57 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Oct 07, 2020 at 02:30:00PM +0200, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your work.
> >
> > On 2020-10-06 18:06:36 +0200, Jacopo Mondi wrote:
> > > The CameraWork class creates a Worker instance and runs on an internal
> > > thread. The worker waits on a set of fences before queueing a capture
> > > requests to the the libcamera::Camera.
> >
> > I really like this solution!
> >
> > >
> > > 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();
> > > +}
> > > +
> > > +void CameraWorker::queueRequest(std::unique_ptr<CaptureRequest> request)
> > > +{
> > > +	CaptureRequest *req = request.release();
> > > +	worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued, req);
> > > +}
> > > +
> > > +int CameraWorker::Worker::waitFence(int fence)
> > > +{
> > > +	struct pollfd fds = { fence, POLLIN, 0 };
> > > +	constexpr unsigned int timeoutMs = 300;
> > > +	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);
> > > +		if (ret < 0)
> > > +			return;
> > > +
> > > +		close(fence);
> > > +	}
> >
> > If you move this to a new function CaptureRequest::waitFences() you cold
> > remove the friend statement :-)
> >
> 
> How can I do so ? A CameraRequest is created by the CameraDevice and
> moved to the CameraWorker. Then the CameraWorker schedules it on its
> worker_ which runs in a dedicated thread (that's why it can wait
> without interrupting the HAL thread).
> 
> Should I moveToThread() every CaptureRequest we receive ? It's
> potentially a costly operation as it interrupts the thread's even
> dispatcher and potentially moves messages from one queue to another.
> 
> I don't think it's worth it ?

Sorry I think I expressed myself unclear. What I meant to say was that 
the for loop could be moved to a function inside CaptureRequest that 
would be called here instead of the loop. The two lines below and the 
function itself would remain. Or am I missing something?

> 
> > > +
> > > +	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);
> > > +	}
> >
> > If you move these to the .cpp file you can get away with forward
> > declaration of most of the classes used.
> 
> Not really, as I can't forward declare symbols from a namespace.

Good point, did not think of that. But for the record you can forward 
declare in a different namespace,

    namespace libcamera {
        class Request;
    }

But I'm not sure I like how it looks ;-)

> 
> >
> > > +
> > > +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_;
> >
> > Could you not remove this and store a shared_ptr inside the Worker?
> >
> 
> mmm, yes I probably should, there's no point in having the
> shared_ptr<> here and passing the raw pointer to the worker.
> 
> Thanks
>   j
> 
> > > +
> > > +	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',
> > >  ])
> > > --
> > > 2.28.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund