[libcamera-devel] [PATCH v2 01/12] libcamera: request: Make Request class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Nov 21 17:31:35 CET 2021
Hi Jacopo,
Thank you for the patch.
On Sat, Nov 20, 2021 at 12:13:02PM +0100, Jacopo Mondi wrote:
> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> Implement the D-Pointer design pattern in the Request class to allow
> changing internal data without affecting the public ABI.
>
> Move the internal fields that are not needed to implement the public
> API to the Request::Private class already. This allow to remove
s/allow/allows/
> the friend class declaration for the PipelineHandler class, which can
> now use the Request::Private API.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> [Move all internal fields to Request::Private and remove friend declaration]
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/internal/meson.build | 1 +
> include/libcamera/internal/request.h | 51 ++++
> .../libcamera/internal/tracepoints/request.tp | 9 +-
> include/libcamera/request.h | 19 +-
> src/libcamera/pipeline_handler.cpp | 15 +-
> src/libcamera/request.cpp | 256 ++++++++++++------
> 6 files changed, 245 insertions(+), 106 deletions(-)
> create mode 100644 include/libcamera/internal/request.h
>
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 665fd6de4ed3..cae65b0604ff 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -34,6 +34,7 @@ libcamera_internal_headers = files([
> 'pipeline_handler.h',
> 'process.h',
> 'pub_key.h',
> + 'request.h',
> 'source_paths.h',
> 'sysfs.h',
> 'v4l2_device.h',
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> new file mode 100644
> index 000000000000..59bddde3a090
> --- /dev/null
> +++ b/include/libcamera/internal/request.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * request.h - Request class private data
> + */
> +#ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
> +#define __LIBCAMERA_INTERNAL_REQUEST_H__
> +
> +#include <memory>
> +
> +#include <libcamera/request.h>
> +
> +namespace libcamera {
> +
> +class Camera;
> +class FrameBuffer;
> +
> +class Request::Private : public Extensible::Private
> +{
> + LIBCAMERA_DECLARE_PUBLIC(Request)
> +
> +public:
> + Private(Camera *camera);
> + ~Private();
> +
> + Camera *camera() const { return camera_; }
> + bool hasPendingBuffers() const;
> +
> + uint64_t cookie() const;
Why do we need a cookie() function here, which wraps Request::cookie(),
when cookie_ is stored in the Request class ?
> + Request::Status status() const;
Same here.
> +
> + bool completeBuffer(FrameBuffer *buffer);
> + void complete();
> + void cancel();
> + void reuse();
> +
> + uint32_t sequence_ = 0;
The reason why you can drop the friend statement is because you make
this member variable public. That's not very nice :-S I'd keep it in
Request for now, along with cookie_ and status_, until we decide how to
handle those members.
> +
> +private:
> + void _cancel();
> +
> + Camera *camera_;
> + bool cancelled_;
> +
> + std::unordered_set<FrameBuffer *> pending_;
> +};
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_INTERNAL_REQUEST_H__ */
> diff --git a/include/libcamera/internal/tracepoints/request.tp b/include/libcamera/internal/tracepoints/request.tp
> index 37d8c46f4d96..37cd2f8864ce 100644
> --- a/include/libcamera/internal/tracepoints/request.tp
> +++ b/include/libcamera/internal/tracepoints/request.tp
> @@ -5,8 +5,9 @@
> * request.tp - Tracepoints for the request object
> */
>
> +#include <libcamera/internal/request.h>
> +
> #include <libcamera/framebuffer.h>
> -#include <libcamera/request.h>
>
> TRACEPOINT_EVENT_CLASS(
> libcamera,
> @@ -62,7 +63,7 @@ TRACEPOINT_EVENT_INSTANCE(
> request,
> request_complete,
> TP_ARGS(
> - libcamera::Request *, req
> + libcamera::Request::Private *, req
> )
> )
>
> @@ -71,7 +72,7 @@ TRACEPOINT_EVENT_INSTANCE(
> request,
> request_cancel,
> TP_ARGS(
> - libcamera::Request *, req
> + libcamera::Request::Private *, req
> )
> )
>
> @@ -79,7 +80,7 @@ TRACEPOINT_EVENT(
> libcamera,
> request_complete_buffer,
> TP_ARGS(
> - libcamera::Request *, req,
> + libcamera::Request::Private *, req,
> libcamera::FrameBuffer *, buf
> ),
> TP_FIELDS(
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index d16904e6b679..f0c5163d987e 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -25,8 +25,10 @@ class CameraControlValidator;
> class FrameBuffer;
> class Stream;
>
> -class Request
> +class Request : public Extensible
> {
> + LIBCAMERA_DECLARE_PRIVATE()
> +
> public:
> enum Status {
> RequestPending,
> @@ -52,34 +54,23 @@ public:
> int addBuffer(const Stream *stream, FrameBuffer *buffer);
> FrameBuffer *findBuffer(const Stream *stream) const;
>
> - uint32_t sequence() const { return sequence_; }
> + uint32_t sequence() const;
> uint64_t cookie() const { return cookie_; }
> Status status() const { return status_; }
>
> - bool hasPendingBuffers() const { return !pending_.empty(); }
> + bool hasPendingBuffers() const;
>
> std::string toString() const;
>
> private:
> LIBCAMERA_DISABLE_COPY(Request)
>
> - friend class PipelineHandler;
> -
> - void complete();
> - void cancel();
> -
> - bool completeBuffer(FrameBuffer *buffer);
> -
> - Camera *camera_;
> ControlList *controls_;
> ControlList *metadata_;
> BufferMap bufferMap_;
> - std::unordered_set<FrameBuffer *> pending_;
>
> - uint32_t sequence_;
> const uint64_t cookie_;
> Status status_;
> - bool cancelled_;
> };
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index f69c4f03b80f..67fdf1d8db01 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -19,6 +19,7 @@
> #include "libcamera/internal/camera.h"
> #include "libcamera/internal/device_enumerator.h"
> #include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/request.h"
> #include "libcamera/internal/tracepoints.h"
>
> /**
> @@ -311,15 +312,15 @@ void PipelineHandler::queueRequest(Request *request)
> {
> LIBCAMERA_TRACEPOINT(request_queue, request);
>
> - Camera *camera = request->camera_;
> + Camera *camera = request->_d()->camera();
> Camera::Private *data = camera->_d();
> data->queuedRequests_.push_back(request);
>
> - request->sequence_ = data->requestSequence_++;
> + request->_d()->sequence_ = data->requestSequence_++;
>
> int ret = queueRequestDevice(camera, request);
> if (ret) {
> - request->cancel();
> + request->_d()->cancel();
> completeRequest(request);
> }
> }
> @@ -360,9 +361,9 @@ void PipelineHandler::queueRequest(Request *request)
> */
> bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> {
> - Camera *camera = request->camera_;
> + Camera *camera = request->_d()->camera();
> camera->bufferCompleted.emit(request, buffer);
> - return request->completeBuffer(buffer);
> + return request->_d()->completeBuffer(buffer);
> }
>
> /**
> @@ -381,9 +382,9 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> */
> void PipelineHandler::completeRequest(Request *request)
> {
> - Camera *camera = request->camera_;
> + Camera *camera = request->_d()->camera();
>
> - request->complete();
> + request->_d()->complete();
>
> Camera::Private *data = camera->_d();
>
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 17fefab7ad0e..90c436648405 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -5,7 +5,7 @@
> * request.cpp - Capture request handling
> */
>
> -#include <libcamera/request.h>
> +#include "libcamera/internal/request.h"
>
> #include <map>
> #include <sstream>
> @@ -23,7 +23,7 @@
> #include "libcamera/internal/tracepoints.h"
>
> /**
> - * \file request.h
> + * \file libcamera/request.h
> * \brief Describes a frame capture request to be processed by a camera
> */
>
> @@ -31,6 +31,165 @@ namespace libcamera {
>
> LOG_DEFINE_CATEGORY(Request)
>
> +/**
> + * \class Request::Private
> + * \brief Request private data
> + *
> + * The Request::Private class stores all private data associated with a
> + * request. It implements the d-pointer design pattern to hide core
> + * Request data from the public API, and exposes utility functions to
> + * internal users of the request (namely the PipelineHandler class and its
> + * subclasses).
> + */
> +
> +/**
> + * \brief Create a Request::Private
> + * \param camera The Camera that creates the request
> + */
> +Request::Private::Private(Camera *camera)
> + : camera_(camera), cancelled_(false)
> +{
> +}
> +
> +Request::Private::~Private()
> +{
> + _cancel();
> +}
> +
> +/**
> + * \fn Request::Private::camera()
> + * \brief Retrieve the camera this request has been queued to
> + * \return The Camera this request has been queued to, or nullptr if the
> + * request hasn't been queued
> + */
> +
> +/**
> + * \brief Check if a request has buffers yet to be completed
> + *
> + * \return True if the request has buffers pending for completion, false
> + * otherwise
> + */
> +bool Request::Private::hasPendingBuffers() const
> +{
> + return !pending_.empty();
> +}
> +
> +/**
> + * \copydoc Request::cookie()
> + *
> + * Used by the tracing framework
> + */
> +uint64_t Request::Private::cookie() const
> +{
> + return _o<Request>()->cookie();
> +}
> +
> +/**
> + * \copydoc Request::status()
> + *
> + * Used by the tracing framework
> + */
> +Request::Status Request::Private::status() const
> +{
> + return _o<Request>()->status();
> +}
> +
> +/**
> + * \brief Complete a buffer for the request
> + * \param[in] buffer The buffer that has completed
> + *
> + * A request tracks the status of all buffers it contains through a set of
> + * pending buffers. This function removes the \a buffer from the set to mark it
> + * as complete. All buffers associate with the request shall be marked as
> + * complete by calling this function once and once only before reporting the
> + * request as complete with the complete() function.
> + *
> + * \return True if all buffers contained in the request have completed, false
> + * otherwise
> + */
> +bool Request::Private::completeBuffer(FrameBuffer *buffer)
> +{
> + LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
> +
> + int ret = pending_.erase(buffer);
> + ASSERT(ret == 1);
> +
> + buffer->_d()->setRequest(nullptr);
> +
> + if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> + cancelled_ = true;
> +
> + return !hasPendingBuffers();
> +}
> +
> +/**
> + * \brief Complete a queued request
> + *
> + * Mark the request as complete by updating its status to RequestComplete,
> + * unless buffers have been cancelled in which case the status is set to
> + * RequestCancelled.
> + */
> +void Request::Private::complete()
> +{
> + Request *request = _o<Request>();
> +
> + ASSERT(request->status() == RequestPending);
> + ASSERT(!hasPendingBuffers());
> +
> + request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> +
> + LOG(Request, Debug) << request->toString();
> +
> + LIBCAMERA_TRACEPOINT(request_complete, this);
> +}
> +
> +void Request::Private::_cancel()
> +{
> + Request *request = _o<Request>();
> +
> + for (FrameBuffer *buffer : pending_) {
> + buffer->cancel();
> + camera_->bufferCompleted.emit(request, buffer);
> + }
> +
> + cancelled_ = true;
> + pending_.clear();
> +}
> +
> +/**
> + * \brief Cancel a queued request
> + *
> + * Mark the request and its associated buffers as cancelled and complete it.
> + *
> + * Set each pending buffer in error state and emit the buffer completion signal
> + * before completing the Request.
> + */
> +void Request::Private::cancel()
> +{
> + LIBCAMERA_TRACEPOINT(request_cancel, this);
> +
> + Request *request = _o<Request>();
> + ASSERT(request->status() == RequestPending);
> +
> + _cancel();
> +}
> +
> +/**
> + * \copydoc Request::reuse()
> + */
> +void Request::Private::reuse()
> +{
> + sequence_ = 0;
> + cancelled_ = false;
> + pending_.clear();
> +}
> +/**
> + * \var Request::Private::sequence_
> + * \brief The request sequence number
> + *
> + * \copydoc Request::sequence()
> + */
> +
> /**
> * \enum Request::Status
> * Request completion status
> @@ -75,8 +234,8 @@ LOG_DEFINE_CATEGORY(Request)
> * completely opaque to libcamera.
> */
> Request::Request(Camera *camera, uint64_t cookie)
> - : camera_(camera), sequence_(0), cookie_(cookie),
> - status_(RequestPending), cancelled_(false)
> + : Extensible(std::make_unique<Private>(camera)),
> + cookie_(cookie), status_(RequestPending)
> {
> controls_ = new ControlList(controls::controls,
> camera->_d()->validator());
> @@ -113,20 +272,19 @@ void Request::reuse(ReuseFlag flags)
> {
> LIBCAMERA_TRACEPOINT(request_reuse, this);
>
> - pending_.clear();
> + _d()->reuse();
> +
> if (flags & ReuseBuffers) {
> for (auto pair : bufferMap_) {
> FrameBuffer *buffer = pair.second;
> buffer->_d()->setRequest(this);
> - pending_.insert(buffer);
> + _d()->pending_.insert(buffer);
> }
> } else {
> bufferMap_.clear();
> }
>
> - sequence_ = 0;
> status_ = RequestPending;
> - cancelled_ = false;
>
> controls_->clear();
> metadata_->clear();
> @@ -188,7 +346,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer)
> }
>
> buffer->_d()->setRequest(this);
> - pending_.insert(buffer);
> + _d()->pending_.insert(buffer);
> bufferMap_[stream] = buffer;
>
> return 0;
> @@ -227,7 +385,6 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
> */
>
> /**
> - * \fn Request::sequence()
> * \brief Retrieve the sequence number for the request
> *
> * When requests are queued, they are given a sequential number to track the
> @@ -242,6 +399,10 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
> *
> * \return The request sequence number
> */
> +uint32_t Request::sequence() const
> +{
> + return _d()->sequence_;
> +}
>
> /**
> * \fn Request::cookie()
> @@ -263,81 +424,14 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
> */
>
> /**
> - * \fn Request::hasPendingBuffers()
> * \brief Check if a request has buffers yet to be completed
> *
> * \return True if the request has buffers pending for completion, false
> * otherwise
> */
> -
> -/**
> - * \brief Complete a queued request
> - *
> - * Mark the request as complete by updating its status to RequestComplete,
> - * unless buffers have been cancelled in which case the status is set to
> - * RequestCancelled.
> - */
> -void Request::complete()
> -{
> - ASSERT(status_ == RequestPending);
> - ASSERT(!hasPendingBuffers());
> -
> - status_ = cancelled_ ? RequestCancelled : RequestComplete;
> -
> - LOG(Request, Debug) << toString();
> -
> - LIBCAMERA_TRACEPOINT(request_complete, this);
> -}
> -
> -/**
> - * \brief Cancel a queued request
> - *
> - * Mark the request and its associated buffers as cancelled and complete it.
> - *
> - * Set each pending buffer in error state and emit the buffer completion signal
> - * before completing the Request.
> - */
> -void Request::cancel()
> -{
> - LIBCAMERA_TRACEPOINT(request_cancel, this);
> -
> - ASSERT(status_ == RequestPending);
> -
> - for (FrameBuffer *buffer : pending_) {
> - buffer->cancel();
> - camera_->bufferCompleted.emit(this, buffer);
> - }
> -
> - pending_.clear();
> - cancelled_ = true;
> -}
> -
> -/**
> - * \brief Complete a buffer for the request
> - * \param[in] buffer The buffer that has completed
> - *
> - * A request tracks the status of all buffers it contains through a set of
> - * pending buffers. This function removes the \a buffer from the set to mark it
> - * as complete. All buffers associate with the request shall be marked as
> - * complete by calling this function once and once only before reporting the
> - * request as complete with the complete() function.
> - *
> - * \return True if all buffers contained in the request have completed, false
> - * otherwise
> - */
> -bool Request::completeBuffer(FrameBuffer *buffer)
> +bool Request::hasPendingBuffers() const
> {
> - LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
> -
> - int ret = pending_.erase(buffer);
> - ASSERT(ret == 1);
> -
> - buffer->_d()->setRequest(nullptr);
> -
> - if (buffer->metadata().status == FrameMetadata::FrameCancelled)
> - cancelled_ = true;
> -
> - return !hasPendingBuffers();
> + return !_d()->pending_.empty();
> }
>
> /**
> @@ -356,8 +450,8 @@ std::string Request::toString() const
> static const char *statuses = "PCX";
>
> /* Example Output: Request(55:P:1/2:6523524) */
> - ss << "Request(" << sequence_ << ":" << statuses[status_] << ":"
> - << pending_.size() << "/" << bufferMap_.size() << ":"
> + ss << "Request(" << sequence() << ":" << statuses[status_] << ":"
> + << _d()->pending_.size() << "/" << bufferMap_.size() << ":"
> << cookie_ << ")";
>
> return ss.str();
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list