[libcamera-devel] [PATCH v2 10/12] android: Remove CameraWorker
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Nov 21 20:00:00 CET 2021
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));
> 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.
> }
>
> /*
> * 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.
> +
> + 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.
> + 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.
> + }
> 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