| Message ID | 20211120111313.106621-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > [Move all internal fields to Request::Private and remove friend declaration] > Signed-off-by: Jacopo Mondi <jacopo@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();
Hi Jacopo, Another comment. On Sun, Nov 21, 2021 at 06:31:37PM +0200, Laurent Pinchart wrote: > On Sat, Nov 20, 2021 at 12:13:02PM +0100, Jacopo Mondi wrote: > > From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> > > [Move all internal fields to Request::Private and remove friend declaration] > > Signed-off-by: Jacopo Mondi <jacopo@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(); I'm not fond of the underscore prefix. A name that describes the purpose of the function would be better. > > + > > + 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();
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@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@ideasonboard.com> > > [Move all internal fields to Request::Private and remove friend declaration] > > Signed-off-by: Jacopo Mondi <jacopo@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
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@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@ideasonboard.com> > > > [Move all internal fields to Request::Private and remove friend declaration] > > > Signed-off-by: Jacopo Mondi <jacopo@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();
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; + Request::Status status() const; + + bool completeBuffer(FrameBuffer *buffer); + void complete(); + void cancel(); + void reuse(); + + uint32_t sequence_ = 0; + +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();