[libcamera-devel] [v4.1] android: Remove CameraWorker
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 8 13:02:42 CET 2021
Hi Jacopo,
Thank you for the patch.
On Tue, Dec 07, 2021 at 12:17:10PM +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.
>
> While at it make CameraRequest::StreamBuffer::fence a UniqueFD to ease
> the management of the fences file descriptor values.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>
> Rebased on top of the latest version of UniqueFD, which does not include
> patch:
> "[v4 22/22] libcamera: base: unique_fd: Pass rvalue reference to constructor and reset()"
>
> ---
>
> src/android/camera_device.cpp | 42 +++++------
> src/android/camera_device.h | 3 -
> src/android/camera_request.cpp | 3 +-
> src/android/camera_request.h | 6 +-
> src/android/camera_stream.cpp | 10 +--
> src/android/camera_worker.cpp | 129 ---------------------------------
> src/android/camera_worker.h | 70 ------------------
> src/android/meson.build | 1 -
> 8 files changed, 28 insertions(+), 236 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 9f772b58d80d..f28fdd6a2955 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -14,10 +14,12 @@
> #include <vector>
>
> #include <libcamera/base/log.h>
> +#include <libcamera/base/unique_fd.h>
> #include <libcamera/base/utils.h>
>
> #include <libcamera/control_ids.h>
> #include <libcamera/controls.h>
> +#include <libcamera/fence.h>
> #include <libcamera/formats.h>
> #include <libcamera/property_ids.h>
>
> @@ -420,7 +422,6 @@ void CameraDevice::flush()
> state_ = State::Flushing;
> }
>
> - worker_.stop();
> camera_->stop();
>
> MutexLocker stateLock(stateMutex_);
> @@ -433,7 +434,6 @@ void CameraDevice::stop()
> if (state_ == State::Stopped)
> return;
>
> - worker_.stop();
> camera_->stop();
>
> {
> @@ -930,13 +930,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.
> */
> FrameBuffer *frameBuffer = nullptr;
> - int acquireFence = -1;
> + UniqueFD acquireFence;
>
> MutexLocker lock(descriptor->streamsProcessMutex_);
>
> @@ -964,7 +961,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> cameraStream->configuration().pixelFormat,
> cameraStream->configuration().size);
> frameBuffer = buffer.frameBuffer.get();
> - acquireFence = buffer.fence;
> + acquireFence = std::move(buffer.fence);
> LOG(HAL, Debug) << ss.str() << " (direct)";
> break;
>
> @@ -990,13 +987,14 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> return -ENOMEM;
> }
>
> + auto fence = std::make_unique<Fence>(std::move(acquireFence));
> descriptor->request_->addBuffer(cameraStream->stream(),
> - frameBuffer, acquireFence);
> + frameBuffer, std::move(fence));
> }
>
> /*
> * Translate controls from Android to libcamera and queue the request
> - * to the CameraWorker thread.
> + * to the camera.
> */
> int ret = processControls(descriptor.get());
> if (ret)
> @@ -1022,26 +1020,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;
> }
> @@ -1063,16 +1058,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.
> + * already been waited on by the library.
> *
> * 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) {
> + /* If handling of the fence has failed restore buffer.fence. */
> + std::unique_ptr<Fence> fence = buffer.frameBuffer->releaseFence();
> + if (fence)
> + buffer.fence = fence->release();
> + }
> buffer.status = Camera3RequestDescriptor::Status::Success;
> }
>
> @@ -1193,7 +1189,7 @@ void CameraDevice::sendCaptureResults()
> std::vector<camera3_stream_buffer_t> resultBuffers;
> resultBuffers.reserve(descriptor->buffers_.size());
>
> - for (const auto &buffer : descriptor->buffers_) {
> + for (auto &buffer : descriptor->buffers_) {
> camera3_buffer_status status = CAMERA3_BUFFER_STATUS_ERROR;
>
> if (buffer.status == Camera3RequestDescriptor::Status::Success)
> @@ -1207,7 +1203,7 @@ void CameraDevice::sendCaptureResults()
> */
> resultBuffers.push_back({ buffer.stream->camera3Stream(),
> buffer.camera3Buffer, status,
> - -1, buffer.fence });
> + -1, buffer.fence.release() });
> }
>
> captureResult.num_output_buffers = resultBuffers.size();
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index e5d9cda2ce3a..64050416e67e 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -29,7 +29,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_ LIBCAMERA_TSA_GUARDED_BY(stateMutex_);
>
> 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 d9b04f166fd7..37b6ae326f3e 100644
> --- a/src/android/camera_request.h
> +++ b/src/android/camera_request.h
> @@ -13,6 +13,7 @@
>
> #include <libcamera/base/class.h>
> #include <libcamera/base/mutex.h>
> +#include <libcamera/base/unique_fd.h>
>
> #include <libcamera/camera.h>
> #include <libcamera/framebuffer.h>
> @@ -20,7 +21,6 @@
> #include <hardware/camera3.h>
>
> #include "camera_metadata.h"
> -#include "camera_worker.h"
>
> class CameraBuffer;
> class CameraStream;
> @@ -45,7 +45,7 @@ public:
> CameraStream *stream;
> buffer_handle_t *camera3Buffer;
> std::unique_ptr<libcamera::FrameBuffer> frameBuffer;
> - int fence;
> + libcamera::UniqueFD fence;
> Status status = Status::Success;
> libcamera::FrameBuffer *internalBuffer = nullptr;
> const libcamera::FrameBuffer *srcBuffer = nullptr;
> @@ -72,7 +72,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_stream.cpp b/src/android/camera_stream.cpp
> index ee9bbe48993a..c21574501916 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -170,16 +170,16 @@ int CameraStream::process(Camera3RequestDescriptor::StreamBuffer *streamBuffer)
> ASSERT(type_ != Type::Direct);
>
> /* Handle waiting on fences on the destination buffer. */
> - if (streamBuffer->fence != -1) {
> - int ret = waitFence(streamBuffer->fence);
> + if (streamBuffer->fence.isValid()) {
> + int ret = waitFence(streamBuffer->fence.get());
> if (ret < 0) {
> LOG(HAL, Error) << "Failed waiting for fence: "
> - << streamBuffer->fence << ": " << strerror(-ret);
> + << streamBuffer->fence.get() << ": "
> + << strerror(-ret);
> return ret;
> }
>
> - ::close(streamBuffer->fence);
> - streamBuffer->fence = -1;
> + streamBuffer->fence.reset();
> }
>
> const StreamConfiguration &output = configuration();
> 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 26ecc2732046..000000000000
> --- a/src/android/camera_worker.h
> +++ /dev/null
> @@ -1,70 +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
> - */
> -
> -#pragma once
> -
> -#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_;
> -};
> 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