[libcamera-devel] [PATCH 10/10] android: Remove CameraWorker
Kieran Bingham
kieran.bingham at ideasonboard.com
Tue Nov 9 17:22:17 CET 2021
Quoting Jacopo Mondi (2021-10-28 12:15:20)
> 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.
Who doesn't love a huge code remove ... ;-)
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/android/camera_device.cpp | 39 ++++------
> src/android/camera_device.h | 5 +-
> 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, 19 insertions(+), 233 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..16237674ea24 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -406,7 +406,6 @@ void CameraDevice::flush()
> state_ = State::Flushing;
> }
>
> - worker_.stop();
> camera_->stop();
>
> MutexLocker stateLock(stateMutex_);
> @@ -419,7 +418,6 @@ void CameraDevice::stop()
> if (state_ == State::Stopped)
> return;
>
> - worker_.stop();
> camera_->stop();
>
> descriptors_ = {};
> @@ -714,9 +712,10 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> }
>
> std::unique_ptr<FrameBuffer>
> -CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> +CameraDevice::createFrameBuffer(const Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> PixelFormat pixelFormat, const Size &size)
> {
> + const buffer_handle_t camera3buffer = *streamBuffer.camera3Buffer;
> CameraBuffer buf(camera3buffer, pixelFormat, size, PROT_READ);
> if (!buf.isValid()) {
> LOG(HAL, Fatal) << "Failed to create CameraBuffer";
> @@ -736,7 +735,8 @@ CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer,
> planes[i].length = buf.size(i);
> }
>
> - return std::make_unique<FrameBuffer>(planes);
> + return std::make_unique<FrameBuffer>(planes, 0,
Aha - 0 is the cookie.
Could we make that clearer somehow? I was confused...
perhaps something like
return std::make_unique<FrameBuffer>(planes, 0 /* cookie */,
streamBuffer.fence);
or
constexpr unsigned int cookie = 0;
return std::make_unique<FrameBuffer>(planes, cookie,
streamBuffer.fence);
?
But otherwise, this looks like it cleans up and uses the new given API
accordingly.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> + streamBuffer.fence);
> }
>
> int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> @@ -912,13 +912,9 @@ 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.
> */
> FrameBuffer *frameBuffer = nullptr;
> - int acquireFence = -1;
> switch (cameraStream->type()) {
> case CameraStream::Type::Mapped:
> /*
> @@ -939,11 +935,10 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> * lifetime management only.
> */
> buffer.frameBuffer =
> - createFrameBuffer(*buffer.camera3Buffer,
> + createFrameBuffer(buffer,
> cameraStream->configuration().pixelFormat,
> cameraStream->configuration().size);
> frameBuffer = buffer.frameBuffer.get();
> - acquireFence = buffer.fence;
> LOG(HAL, Debug) << ss.str() << " (direct)";
> break;
>
> @@ -970,12 +965,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> }
>
> descriptor->request_->addBuffer(cameraStream->stream(),
> - frameBuffer, acquireFence);
> + frameBuffer);
> }
>
> /*
> * 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 +996,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 +1034,17 @@ 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;
> + buffer.fence = buffer.frameBuffer->fence();
> buffer.status = Camera3RequestDescriptor::Status::Success;
> }
>
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 2a414020f1ad..d83f77a8cf22 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;
> @@ -86,7 +85,7 @@ private:
> void stop();
>
> std::unique_ptr<libcamera::FrameBuffer>
> - createFrameBuffer(const buffer_handle_t camera3buffer,
> + createFrameBuffer(const Camera3RequestDescriptor::StreamBuffer &streamBuffer,
> libcamera::PixelFormat pixelFormat,
> const libcamera::Size &size);
> void abortRequest(Camera3RequestDescriptor *descriptor) const;
> @@ -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 5bac1b8f7a5f..f656c3ebe448 100644
> --- a/src/android/camera_request.cpp
> +++ b/src/android/camera_request.cpp
> @@ -49,8 +49,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 cfafa4450d71..e019c38784ba 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;
> @@ -60,7 +59,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',
> --
> 2.33.1
>
More information about the libcamera-devel
mailing list