[libcamera-devel] [PATCH v2 10/12] android: Remove CameraWorker
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Nov 27 00:13:10 CET 2021
Hi Jacopo,
On Fri, Nov 26, 2021 at 01:25:29PM +0100, Jacopo Mondi wrote:
> On Sun, Nov 21, 2021 at 09:00:00PM +0200, Laurent Pinchart wrote:
> > On Sat, Nov 20, 2021 at 12:13:11PM +0100, Jacopo Mondi wrote:
> > > The CameraWorker class purpose was to handle acquire fences for incoming
> > > capture requests directed to libcamera.
> > >
> > > Now that fences are handled by the core library, it is not required to
> > > handle them in the HAL and the CameraWorker and CaptureRequest classes
> > > can be dropped.
> > >
> > > Update the core in CameraDevice class accordingly to queue Requests
> > > directly to the libcamera::Camera and set the release_fence to the
> > > value of the FrameBuffer::fence() for streams of type ::Direct.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > src/android/camera_device.cpp | 43 +++++------
> > > src/android/camera_device.h | 3 -
> > > src/android/camera_request.cpp | 3 +-
> > > src/android/camera_request.h | 3 +-
> > > src/android/camera_worker.cpp | 129 ---------------------------------
> > > src/android/camera_worker.h | 72 ------------------
> > > src/android/meson.build | 1 -
> > > 7 files changed, 24 insertions(+), 230 deletions(-)
> > > delete mode 100644 src/android/camera_worker.cpp
> > > delete mode 100644 src/android/camera_worker.h
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index f2e0bdbdbbf6..29f0abd96ba9 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -19,6 +19,7 @@
> > >
> > > #include <libcamera/control_ids.h>
> > > #include <libcamera/controls.h>
> > > +#include <libcamera/fence.h>
> > > #include <libcamera/formats.h>
> > > #include <libcamera/property_ids.h>
> > >
> > > @@ -406,7 +407,6 @@ void CameraDevice::flush()
> > > state_ = State::Flushing;
> > > }
> > >
> > > - worker_.stop();
> > > camera_->stop();
> > >
> > > MutexLocker stateLock(stateMutex_);
> > > @@ -419,7 +419,6 @@ void CameraDevice::stop()
> > > if (state_ == State::Stopped)
> > > return;
> > >
> > > - worker_.stop();
> > > camera_->stop();
> > >
> > > descriptors_ = {};
> > > @@ -912,13 +911,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >
> > > /*
> > > * Inspect the camera stream type, create buffers opportunely
> > > - * and add them to the Request if required. Only acquire fences
> > > - * for streams of type Direct are handled by the CameraWorker,
> > > - * while fences for streams of type Internal and Mapped are
> > > - * handled at post-processing time.
> > > + * and add them to the Request if required.
> > > */
> > > + FileDescriptor fenceFd;
> > > FrameBuffer *frameBuffer = nullptr;
> > > - int acquireFence = -1;
> > > switch (cameraStream->type()) {
> > > case CameraStream::Type::Mapped:
> > > /*
> > > @@ -943,7 +939,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > cameraStream->configuration().pixelFormat,
> > > cameraStream->configuration().size);
> > > frameBuffer = buffer.frameBuffer.get();
> > > - acquireFence = buffer.fence;
> > > + fenceFd = FileDescriptor(std::move(buffer.fence));
>
> This also is a concern for me: the FileDescriptor that is passed to
> the Fence should be created by moving the file descriptor, in order
> not to duplicate it. There's no way I found to enforce that through
> the Fence API.
Going for a unique fd could fix that (assuming it would only have a
constructor that takes ownership of the int fd).
> > > LOG(HAL, Debug) << ss.str() << " (direct)";
> > > break;
> > >
> > > @@ -969,13 +965,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > return -ENOMEM;
> > > }
> > >
> > > + std::unique_ptr<Fence> fence = std::make_unique<Fence>(fenceFd);
> > > descriptor->request_->addBuffer(cameraStream->stream(),
> > > - frameBuffer, acquireFence);
> > > + frameBuffer, std::move(fence));
> >
> > frameBuffer,
> > std::make_unique<Fence>(fenceFd));
> >
> > could work too, up to you.
>
> Better indeed, thanks
>
> >
> > > }
> > >
> > > /*
> > > * Translate controls from Android to libcamera and queue the request
> > > - * to the CameraWorker thread.
> > > + * to the camera.
> > > */
> > > int ret = processControls(descriptor.get());
> > > if (ret)
> > > @@ -1001,26 +998,23 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > }
> > >
> > > if (state_ == State::Stopped) {
> > > - worker_.start();
> > > -
> > > ret = camera_->start();
> > > if (ret) {
> > > LOG(HAL, Error) << "Failed to start camera";
> > > - worker_.stop();
> > > return ret;
> > > }
> > >
> > > state_ = State::Running;
> > > }
> > >
> > > - CaptureRequest *request = descriptor->request_.get();
> > > + Request *request = descriptor->request_.get();
> > >
> > > {
> > > MutexLocker descriptorsLock(descriptorsMutex_);
> > > descriptors_.push(std::move(descriptor));
> > > }
> > >
> > > - worker_.queueRequest(request);
> > > + camera_->queueRequest(request);
> > >
> > > return 0;
> > > }
> > > @@ -1042,16 +1036,23 @@ void CameraDevice::requestComplete(Request *request)
> > > /*
> > > * Streams of type Direct have been queued to the
> > > * libcamera::Camera and their acquire fences have
> > > - * already been waited on by the CameraWorker.
> > > + * been handled by the library.
> > > + *
> > > + * If the fence has been signalled correctly it will be -1 and
> > > + * closed, otherwise it is set to the 'acquire_fence' value and
> > > + * the framework will have to close it.
> > > *
> > > * Acquire fences of streams of type Internal and Mapped
> > > * will be handled during post-processing.
> > > - *
> > > - * \todo Instrument the CameraWorker to set the acquire
> > > - * fence to -1 once it has handled it and remove this check.
> > > */
> > > - if (stream->type() == CameraStream::Type::Direct)
> > > - buffer.fence = -1;
> > > + if (stream->type() == CameraStream::Type::Direct) {
> > > + Fence *fence = buffer.frameBuffer->fence();
> > > + if (!fence)
> > > + break;
> >
> > Can this happen ? If not I'd ASSERT(), if it can we need better error
> > handling.
>
> Yes it does. If the fence has correctly been signalled it has been
> reset by the core.
>
> > > +
> > > + FileDescriptor fenceFd = std::move(fence->fd());
> >
> > This won't work as you expect. fence is a Fence *, and Fence::fd()
> > returns a const FileDescriptor &. std::move() will thus return a const
> > FileDescriptor &&. FileDescriptor has no assignment operator=() that
> > takes a *const* rvalue reference, so the copy assignment operator will
> > be called, creating a copy of FileDescriptor.
> >
> > This in itself isn't a big issue, as copying a FileDescriptor won't
> > duplicate the underlying fd. The construct is however misleading, so I'd
> > drop the std::move.
>
> Right, misleading indeed
>
> > > + buffer.fence = fenceFd.fd();
> >
> > This will also not work as expected, and it's a problem. The value
> > returned by fd() is valid, but only until the last FileDescriptor
> > instance referencing the same fd gets destroyed. Due to the copy above,
> > this happens when the FrameBuffer is destroyed (or added to a new
> > request for applications that reuse FrameBuffer instances, as you call
> > closeFence() in addRequest()). The fd you send back to the camera
> > service will thus not be valid.
> >
> > To fix this, I think we need
> >
> > - A FrameBuffer::resetFence() that returns a unique pointer to the
> > fence, to make the lifetime of the fence deterministric when requests
> > complete.
> >
> > - A way to extract the file descriptor from the fence without closing
> > it. This requires a new function in the FileDescriptor class, which
> > would be dangerous to call if there are multiple copies of the
> > FileDescriptor. This is where a unique fd class could help.
> >
> > We could also have a Fence::resetFileDescriptor() function to extract
> > the FileDescriptor from the Fence object, but we can call the new
> > function to extract the numerical fd from the FileDescriptor directly
> > on Fence::fd(), which should I think be good enough.
>
> Ouch.
>
> Would something like this be better?
>
> include/libcamera/file_descriptor.h
>
> int fd() const { return fd_; }
>
> - private:
> int fd_;
> };
>
>
> src/libcamera/file_descriptor.cpp
>
> int FileDescriptor::reset()
> {
> int fd = fd_->fd();
>
> fd_->fd_ = -1;
> fd_.reset();
>
> return fd;
> }
The FileDescriptor function should be called release() to match the
std::unique_ptr<> nomenclature.
> src/android/camera_device.cpp
>
> std::unique_ptr<Fence> fence =
> buffer.frameBuffer->resetFence();
> if (!fence)
> break;
>
> buffer.fence = fence->fd().reset();
It would work, but I don't think it's a good idea I'm afraid.
FileDescriptor is analog to std::shared_ptr<>, and there's a reason why
shared_ptr has no release() function. Its behaviour would be ill-defined
when multiple instances of shared_ptr point to the same object.
To solve this, I think we'll need a std::unique_ptr<>-like wrapper for
file descriptors. If you don't beat me to it, I'll have a look at that
over the weekend, based on the ScopedFD class that Hiro has proposed a
while ago (possibly renamed to UniqueFD, or to something else, I'm not
sure yet).
> > > + }
> > > buffer.status = Camera3RequestDescriptor::Status::Success;
> > > }
> > >
> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > > index 2a414020f1ad..c8cc78688bec 100644
> > > --- a/src/android/camera_device.h
> > > +++ b/src/android/camera_device.h
> > > @@ -30,7 +30,6 @@
> > > #include "camera_capabilities.h"
> > > #include "camera_metadata.h"
> > > #include "camera_stream.h"
> > > -#include "camera_worker.h"
> > > #include "jpeg/encoder.h"
> > >
> > > class Camera3RequestDescriptor;
> > > @@ -105,8 +104,6 @@ private:
> > > unsigned int id_;
> > > camera3_device_t camera3Device_;
> > >
> > > - CameraWorker worker_;
> > > -
> > > libcamera::Mutex stateMutex_; /* Protects access to the camera state. */
> > > State state_;
> > >
> > > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> > > index 8162aa78e953..917769070cd9 100644
> > > --- a/src/android/camera_request.cpp
> > > +++ b/src/android/camera_request.cpp
> > > @@ -47,8 +47,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> > > * Create the CaptureRequest, stored as a unique_ptr<> to tie its
> > > * lifetime to the descriptor.
> > > */
> > > - request_ = std::make_unique<CaptureRequest>(camera,
> > > - reinterpret_cast<uint64_t>(this));
> > > + request_ = camera->createRequest(reinterpret_cast<uint64_t>(this));
> > > }
> > >
> > > Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;
> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > > index 8d1204e5590b..69e1fe9fc379 100644
> > > --- a/src/android/camera_request.h
> > > +++ b/src/android/camera_request.h
> > > @@ -20,7 +20,6 @@
> > > #include <hardware/camera3.h>
> > >
> > > #include "camera_metadata.h"
> > > -#include "camera_worker.h"
> > >
> > > class CameraBuffer;
> > > class CameraStream;
> > > @@ -71,7 +70,7 @@ public:
> > > std::vector<StreamBuffer> buffers_;
> > >
> > > CameraMetadata settings_;
> > > - std::unique_ptr<CaptureRequest> request_;
> > > + std::unique_ptr<libcamera::Request> request_;
> > > std::unique_ptr<CameraMetadata> resultMetadata_;
> > >
> > > bool complete_ = false;
> > > diff --git a/src/android/camera_worker.cpp b/src/android/camera_worker.cpp
> > > deleted file mode 100644
> > > index dabb305407a2..000000000000
> > > --- a/src/android/camera_worker.cpp
> > > +++ /dev/null
> > > @@ -1,129 +0,0 @@
> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > -/*
> > > - * Copyright (C) 2020, Google Inc.
> > > - *
> > > - * camera_worker.cpp - Process capture requests on behalf of the Camera HAL
> > > - */
> > > -
> > > -#include "camera_worker.h"
> > > -
> > > -#include <errno.h>
> > > -#include <string.h>
> > > -#include <sys/poll.h>
> > > -#include <unistd.h>
> > > -
> > > -#include "camera_device.h"
> > > -
> > > -using namespace libcamera;
> > > -
> > > -LOG_DECLARE_CATEGORY(HAL)
> > > -
> > > -/*
> > > - * \class CaptureRequest
> > > - * \brief Wrap a libcamera::Request associated with buffers and fences
> > > - *
> > > - * A CaptureRequest is constructed by the CameraDevice, filled with
> > > - * buffers and fences provided by the camera3 framework and then processed
> > > - * by the CameraWorker which queues it to the libcamera::Camera after handling
> > > - * fences.
> > > - */
> > > -CaptureRequest::CaptureRequest(Camera *camera, uint64_t cookie)
> > > - : camera_(camera)
> > > -{
> > > - request_ = camera_->createRequest(cookie);
> > > -}
> > > -
> > > -void CaptureRequest::addBuffer(Stream *stream, FrameBuffer *buffer, int fence)
> > > -{
> > > - request_->addBuffer(stream, buffer);
> > > - acquireFences_.push_back(fence);
> > > -}
> > > -
> > > -void CaptureRequest::queue()
> > > -{
> > > - camera_->queueRequest(request_.get());
> > > -}
> > > -
> > > -/*
> > > - * \class CameraWorker
> > > - * \brief Process a CaptureRequest on an internal thread
> > > - *
> > > - * The CameraWorker class wraps a Worker that runs on an internal thread
> > > - * and schedules processing of CaptureRequest through it.
> > > - */
> > > -CameraWorker::CameraWorker()
> > > -{
> > > - worker_.moveToThread(this);
> > > -}
> > > -
> > > -void CameraWorker::start()
> > > -{
> > > - Thread::start();
> > > -}
> > > -
> > > -void CameraWorker::stop()
> > > -{
> > > - exit();
> > > - wait();
> > > -}
> > > -
> > > -void CameraWorker::run()
> > > -{
> > > - exec();
> > > - dispatchMessages(Message::Type::InvokeMessage);
> > > -}
> > > -
> > > -void CameraWorker::queueRequest(CaptureRequest *request)
> > > -{
> > > - /* Async process the request on the worker which runs its own thread. */
> > > - worker_.invokeMethod(&Worker::processRequest, ConnectionTypeQueued,
> > > - request);
> > > -}
> > > -
> > > -/*
> > > - * \class CameraWorker::Worker
> > > - * \brief Process a CaptureRequest handling acquisition fences
> > > - */
> > > -int CameraWorker::Worker::waitFence(int fence)
> > > -{
> > > - /*
> > > - * \todo Better characterize the timeout. Currently equal to the one
> > > - * used by the Rockchip Camera HAL on ChromeOS.
> > > - */
> > > - constexpr unsigned int timeoutMs = 300;
> > > - struct pollfd fds = { fence, POLLIN, 0 };
> > > -
> > > - do {
> > > - int ret = poll(&fds, 1, timeoutMs);
> > > - if (ret == 0)
> > > - return -ETIME;
> > > -
> > > - if (ret > 0) {
> > > - if (fds.revents & (POLLERR | POLLNVAL))
> > > - return -EINVAL;
> > > -
> > > - return 0;
> > > - }
> > > - } while (errno == EINTR || errno == EAGAIN);
> > > -
> > > - return -errno;
> > > -}
> > > -
> > > -void CameraWorker::Worker::processRequest(CaptureRequest *request)
> > > -{
> > > - /* Wait on all fences before queuing the Request. */
> > > - for (int fence : request->fences()) {
> > > - if (fence == -1)
> > > - continue;
> > > -
> > > - int ret = waitFence(fence);
> > > - close(fence);
> > > - if (ret < 0) {
> > > - LOG(HAL, Error) << "Failed waiting for fence: "
> > > - << fence << ": " << strerror(-ret);
> > > - return;
> > > - }
> > > - }
> > > -
> > > - request->queue();
> > > -}
> > > diff --git a/src/android/camera_worker.h b/src/android/camera_worker.h
> > > deleted file mode 100644
> > > index c94f16325925..000000000000
> > > --- a/src/android/camera_worker.h
> > > +++ /dev/null
> > > @@ -1,72 +0,0 @@
> > > -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > -/*
> > > - * Copyright (C) 2020, Google Inc.
> > > - *
> > > - * camera_worker.h - Process capture requests on behalf of the Camera HAL
> > > - */
> > > -#ifndef __ANDROID_CAMERA_WORKER_H__
> > > -#define __ANDROID_CAMERA_WORKER_H__
> > > -
> > > -#include <memory>
> > > -#include <stdint.h>
> > > -
> > > -#include <libcamera/base/object.h>
> > > -#include <libcamera/base/thread.h>
> > > -
> > > -#include <libcamera/camera.h>
> > > -#include <libcamera/framebuffer.h>
> > > -#include <libcamera/request.h>
> > > -#include <libcamera/stream.h>
> > > -
> > > -class CameraDevice;
> > > -
> > > -class CaptureRequest
> > > -{
> > > -public:
> > > - CaptureRequest(libcamera::Camera *camera, uint64_t cookie);
> > > -
> > > - const std::vector<int> &fences() const { return acquireFences_; }
> > > - libcamera::ControlList &controls() { return request_->controls(); }
> > > - const libcamera::ControlList &metadata() const
> > > - {
> > > - return request_->metadata();
> > > - }
> > > - unsigned long cookie() const { return request_->cookie(); }
> > > -
> > > - void addBuffer(libcamera::Stream *stream,
> > > - libcamera::FrameBuffer *buffer, int fence);
> > > - void queue();
> > > -
> > > -private:
> > > - libcamera::Camera *camera_;
> > > - std::vector<int> acquireFences_;
> > > - std::unique_ptr<libcamera::Request> request_;
> > > -};
> > > -
> > > -class CameraWorker : private libcamera::Thread
> > > -{
> > > -public:
> > > - CameraWorker();
> > > -
> > > - void start();
> > > - void stop();
> > > -
> > > - void queueRequest(CaptureRequest *request);
> > > -
> > > -protected:
> > > - void run() override;
> > > -
> > > -private:
> > > - class Worker : public libcamera::Object
> > > - {
> > > - public:
> > > - void processRequest(CaptureRequest *request);
> > > -
> > > - private:
> > > - int waitFence(int fence);
> > > - };
> > > -
> > > - Worker worker_;
> > > -};
> > > -
> > > -#endif /* __ANDROID_CAMERA_WORKER_H__ */
> > > diff --git a/src/android/meson.build b/src/android/meson.build
> > > index 332b177ca805..75b4bf207085 100644
> > > --- a/src/android/meson.build
> > > +++ b/src/android/meson.build
> > > @@ -47,7 +47,6 @@ android_hal_sources = files([
> > > 'camera_ops.cpp',
> > > 'camera_request.cpp',
> > > 'camera_stream.cpp',
> > > - 'camera_worker.cpp',
> > > 'jpeg/encoder_libjpeg.cpp',
> > > 'jpeg/exif.cpp',
> > > 'jpeg/post_processor_jpeg.cpp',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list