[libcamera-devel] [PATCH v2 10/12] android: Remove CameraWorker
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 26 13:25:29 CET 2021
Hi Laurent
On Sun, Nov 21, 2021 at 09:00:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> 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.
> > 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;
}
src/android/camera_device.cpp
std::unique_ptr<Fence> fence =
buffer.frameBuffer->resetFence();
if (!fence)
break;
buffer.fence = fence->fd().reset();
>
> > + }
> > 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