[libcamera-devel] [PATCH v2 01/12] libcamera: request: Make Request class Extensible
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Nov 30 05:54:47 CET 2021
Hi Jacopo,
On Fri, Nov 26, 2021 at 12:05:06PM +0100, Jacopo Mondi wrote:
> On Sun, Nov 21, 2021 at 06:31:35PM +0200, Laurent Pinchart wrote:
> > 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.
Ah indeed sorry.
> 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())
You can pick either option. If you prefer keeping the Private::cookie()
function, you can also make it inline if desired.
> > > + 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