[libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 1 12:59:46 CET 2021
On Wed, Dec 01, 2021 at 11:42:49AM +0100, Jacopo Mondi wrote:
> 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...
Onwership of the fence is transferred to the HAL when the request is
queued, isn't 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