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

9984 diff mbox series

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.

Signed-off-by: Jacopo Mondi <jacopo@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

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@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 :-)

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

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

> +
> +	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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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 ?

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

>
> > +
> > +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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> 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@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@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

Hi Niklas,

On Wed, Oct 07, 2020 at 06:13:32PM +0200, Niklas Söderlund wrote:
> 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@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?
>

Please not that also request->request_ is part of the CaptureRequest,
so also the two lines below should be moved there.

Also, CaptureRequest is created in the HAL thread so it would need to
invokeMethod() on a CameraWorker::Worker to delegate there the
waitFence() call. As I've pictured it it seems more complex that using
a friend statement, but I might be 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 ;-)

:/ not super nice

>
> >
> > >
> > > > +
> > > > +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@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund

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@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',
>  ])

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

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

Hi Laurent,

On Thu, Oct 08, 2020 at 11:33:26PM +0300, Laurent Pinchart wrote:
> 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@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.
>

Right, we poll manually, but we dispatch messages which indeed run on
the event loop

> 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 agree we are.

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

flush seems to be meant to be called before a new configuration

     * on the given device. The framework will use this to dump all state as
     * quickly as possible in order to prepare for a configure_streams() call.

Not in the teardown path.

If I got you right we might hit a small (not that small actually)
windown where a message is added to the thread's queue but never
processed as a Thread::exit() just after the queueing happens
interrupts the dispatcher for good.

As far as the HAL is concerned that pending request should be signaled
as ERROR before the HAL::stop() completes ? In this case, would
instrumenting wait() with an additional message queue swip  help ? We
can emit a signal for every non-handled message..

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

Hi Jacopo,

On Fri, Oct 09, 2020 at 11:31:59AM +0200, Jacopo Mondi wrote:
> On Thu, Oct 08, 2020 at 11:33:26PM +0300, Laurent Pinchart wrote:
> > 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@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.
> 
> Right, we poll manually, but we dispatch messages which indeed run on
> the event loop
> 
> > 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 agree we are.
> 
> >
> > - 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.
> 
> flush seems to be meant to be called before a new configuration
> 
>      * on the given device. The framework will use this to dump all state as
>      * quickly as possible in order to prepare for a configure_streams() call.
> 
> Not in the teardown path.

Indeed. I however wonder if in practice there will always be a flush()
before close().

> If I got you right we might hit a small (not that small actually)
> windown where a message is added to the thread's queue but never
> processed as a Thread::exit() just after the queueing happens
> interrupts the dispatcher for good.

Correct.

> As far as the HAL is concerned that pending request should be signaled
> as ERROR before the HAL::stop() completes ? In this case, would
> instrumenting wait() with an additional message queue swip  help ? We
> can emit a signal for every non-handled message..

How does that work on the camera service side though ? Does it expect
close() to block until all requests are returned ? Can it even support
request completion callbacks while close() is in progress ? The android
camera HAL API doesn't have an explicit stop(), it's pretty annoying :-S

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