[libcamera-devel] [PATCH v2 01/12] libcamera: request: Make Request class Extensible
Jacopo Mondi
jacopo at jmondi.org
Fri Nov 26 12:05:06 CET 2021
Hi Laurent,
On Sun, Nov 21, 2021 at 06:31:35PM +0200, Laurent Pinchart wrote:
> 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 ?
>
As the comment on the function implementation reports, it is required
by the tracing framework.
TRACEPOINT_EVENT(
libcamera,
request_complete_buffer,
TP_ARGS(
libcamera::Request::Private *, req,
libcamera::FrameBuffer *, buf
),
TP_FIELDS(
ctf_integer_hex(uintptr_t, request, reinterpret_cast<uintptr_t>(req))
ctf_integer(uint64_t, cookie, req->cookie())
ctf_integer(int, status, req->status())
ctf_integer_hex(uintptr_t, buffer, reinterpret_cast<uintptr_t>(buf))
ctf_enum(libcamera, buffer_status, uint32_t, buf_status, buf->metadata().status)
)
)
*req is now a Request::Private and we have
ctf_integer(uint64_t, cookie, req->cookie())
ctf_integer(int, status, req->status())
Which I can workaround with:
ctf_integer(uint64_t, cookie, req->_o<libcamera::Request>()->cookie())
ctf_integer(int, status, req->_o<libcamera::Request>()->status())
> > + 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.
>
ack, I was unsure. Actually removing a friend to then give access to
all other to one field is not a huge win, I concur :)
But at least I can move the friend statement to the Private class if
the field is moved there too!
> > +
> > +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