[libcamera-devel] [PATCH v3 11/11] android: Remove CameraWorker
Jacopo Mondi
jacopo at jmondi.org
Wed Dec 1 13:10:16 CET 2021
Hi Laurent
On Wed, Dec 01, 2021 at 01:59:46PM +0200, Laurent Pinchart wrote:
> On Wed, Dec 01, 2021 at 11:42:49AM +0100, Jacopo Mondi wrote:
> > > >
> > > > 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 ?
>
Yes, at the same time the HAL is suggested to copy in the values
passed in by the framework.
I can however try and if nothing seems wrong move buffer.fence without
going through a temporary variable.
> > > > }
> > > >
> > > > 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