[libcamera-devel] [RFC 1/2] android: camera_worker: Introduce CameraWorker
Jacopo Mondi
jacopo at jmondi.org
Wed Oct 7 18:27:35 CEST 2020
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 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?
>
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 at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
>
> --
> Regards,
> Niklas Söderlund
More information about the libcamera-devel
mailing list