[libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 1 11:42:49 CET 2021
Hi Laurent
On Wed, Dec 01, 2021 at 04:15:58AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Dec 01, 2021 at 12:36:34AM +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>
> > ---
> > src/android/camera_device.cpp | 46 ++++++------
> > src/android/camera_device.h | 3 -
> > src/android/camera_request.cpp | 8 +-
> > 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, 36 insertions(+), 237 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 1938b10509fa..b7114c615733 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -15,10 +15,12 @@
> >
> > #include <libcamera/base/log.h>
> > #include <libcamera/base/thread.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>
> >
> > @@ -406,7 +408,6 @@ void CameraDevice::flush()
> > state_ = State::Flushing;
> > }
> >
> > - worker_.stop();
> > camera_->stop();
> >
> > MutexLocker stateLock(stateMutex_);
> > @@ -419,7 +420,6 @@ void CameraDevice::stop()
> > if (state_ == State::Stopped)
> > return;
> >
> > - worker_.stop();
> > camera_->stop();
> >
> > descriptors_ = {};
> > @@ -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:
> > /*
> > @@ -943,7 +939,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > 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, std::make_unique<Fence>(std::move(buffer.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)
> > @@ -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,24 @@ 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 the fence has been closed, send -1 to the framework. */
> > + std::unique_ptr<Fence> fence = buffer.frameBuffer->resetFence();
> > + if (!fence || !fence->isValid())
>
> I'm still not thrilled by the fact that we have two checks that both
> report the same condition. Let's leave this aside for now, we can
> address the issue by improving the API on top (a \todo somewhere would
> be good though).
>
> > + break;
>
> Is this correct, don't you need to process the other buffers ? It seems
> that
>
> /*
> * If handling of the fence has failed, send to the
> * framework the acquire_fence fd as release_fence so
> * it can close it.
> */
> std::unique_ptr<Fence> fence = buffer.frameBuffer->resetFence();
> if (fence)
> buffer.fence = fence->reset();
>
> should be enough.
>
Ups, I was sure the break was there already, I wen throught too many
versions! Indeed other buffers should be processed (and the buffer
status set!). It's weird this doesn't cause huge issues :/
> > +
> > + /*
> > + * If handling of the fence has failed, send to the
> > + * framework the acquire_fence fd as release_fence so
> > + * it can close it.
> > + */
> > + buffer.fence = std::move(fence->reset());
>
> No need for std::move().
>
> > + }
> > buffer.status = Camera3RequestDescriptor::Status::Success;
> > }
> >
> > @@ -1172,7 +1172,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)
> > @@ -1186,7 +1186,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 51fe7da2cb47..494d79d2a3ea 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..c46689a52d6d 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;
> > @@ -57,8 +56,11 @@ Camera3RequestDescriptor::StreamBuffer::StreamBuffer(
> > CameraStream *cameraStream, const camera3_stream_buffer_t &buffer,
> > Camera3RequestDescriptor *requestDescriptor)
> > : stream(cameraStream), camera3Buffer(buffer.buffer),
> > - fence(buffer.acquire_fence), request(requestDescriptor)
> > + request(requestDescriptor)
> > {
> > + /* Do not reset buffer.acquire_fence by moving it to fence. */
> > + int fenceFd = buffer.acquire_fence;
> > + fence = UniqueFD(std::move(fenceFd));
>
> Would it be a problem to move buffer.acquire_fence ?
I don't know. There are no requirements about that that I found, but I
thought that not resetting buffer.acquire_fence (which if I'm not
mistaken is owned by the framework) was safer.. Probably it's not an
issue but who knows what the camera service does with it...
>
> > }
> >
> > Camera3RequestDescriptor::StreamBuffer::~StreamBuffer() = default;
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index f3cb6d643961..4372ad775c19 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -13,6 +13,7 @@
> > #include <vector>
> >
> > #include <libcamera/base/class.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;
> > @@ -71,7 +71,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 9023c13c40f0..08252341e9cf 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -169,16 +169,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