| Message ID | 20260113000808.15395-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > The Request class has a set of private member variables, with some of > them stored in the Request class itself, and some in the > Request::Private class. Storing the variables in the Request class > itself has the advantage that accessors can be inline, at the cost of > ABI breakage if variables need to be added, removed or otherwise > modified. > > The controls_ and metadata_ variables have recently been turned from > pointers to instances. This broke the ABI. To avoid further breakages, > move all remaining private member variables to Request::Private. The > performance impact of not inlining accessors will be negligible. I feel like this Request class is moving into a suboptimal direction. After this change a `Request` is essentially just a single pointer to its `Private` class. But `std::unique_ptr<Request>` is what is used in e.g. `Camera::createRequest()`, so essentially it is just introducing unnecessary indirection, and increases the number of allocations. But short of turning `Request` into a `RequestHandle` kind of thing, I am not sure what would be a good approach. > > While at it, drop an unneeded class forward declaration. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/request.h | 12 ++- > include/libcamera/request.h | 15 +--- > src/libcamera/request.cpp | 106 +++++++++++++++------------ > 3 files changed, 71 insertions(+), 62 deletions(-) > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > index 693097ee9a26..7715077b3f7c 100644 > --- a/include/libcamera/internal/request.h > +++ b/include/libcamera/internal/request.h > @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private > LIBCAMERA_DECLARE_PUBLIC(Request) > > public: > - Private(Camera *camera); > + Private(Camera *camera, uint64_t cookie); > ~Private(); > > Camera *camera() const { return camera_; } > @@ -41,7 +41,7 @@ public: > bool completeBuffer(FrameBuffer *buffer); > void complete(); > void cancel(); > - void reset(); > + void reset(Request::ReuseFlag flags); > > void prepare(std::chrono::milliseconds timeout = 0ms); > Signal<> prepared; > @@ -56,14 +56,20 @@ private: > void timeout(); > > Camera *camera_; > + const uint64_t cookie_; > + > + Status status_; > bool cancelled_; > uint32_t sequence_ = 0; > bool prepared_ = false; > > + ControlList controls_; > + ControlList metadata_; > + BufferMap bufferMap_; > + > std::unordered_set<FrameBuffer *> pending_; > std::map<FrameBuffer *, EventNotifier> notifiers_; > std::unique_ptr<Timer> timer_; > - ControlList metadata_; > }; > > } /* namespace libcamera */ > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 290983f61352..08ac6e8daba7 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -22,7 +22,6 @@ > namespace libcamera { > > class Camera; > -class CameraControlValidator; > class FrameBuffer; > class Stream; > > @@ -49,16 +48,16 @@ public: > > void reuse(ReuseFlag flags = Default); > > - ControlList &controls() { return controls_; } > + ControlList &controls(); > const ControlList &metadata() const; > - const BufferMap &buffers() const { return bufferMap_; } > + const BufferMap &buffers() const; > int addBuffer(const Stream *stream, FrameBuffer *buffer, > std::unique_ptr<Fence> &&fence = {}); > FrameBuffer *findBuffer(const Stream *stream) const; > > uint32_t sequence() const; > - uint64_t cookie() const { return cookie_; } > - Status status() const { return status_; } > + uint64_t cookie() const; > + Status status() const; > > bool hasPendingBuffers() const; > > @@ -66,12 +65,6 @@ public: > > private: > LIBCAMERA_DISABLE_COPY(Request) > - > - ControlList controls_; > - BufferMap bufferMap_; > - > - const uint64_t cookie_; > - Status status_; > }; > > std::ostream &operator<<(std::ostream &out, const Request &r); > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index 57f1f060d5b4..9d30091a9af7 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request) > > /** > * \brief Create a Request::Private > - * \param camera The Camera that creates the request > + * \param[in] camera The Camera that creates the request > + * \param[in] cookie Opaque cookie for application use > * > * \todo Add a validator for metadata controls. > */ > -Request::Private::Private(Camera *camera) > - : camera_(camera), cancelled_(false), metadata_(controls::controls) > +Request::Private::Private(Camera *camera, uint64_t cookie) > + : camera_(camera), cookie_(cookie), status_(RequestPending), > + cancelled_(false), > + controls_(camera->controls(), camera->_d()->validator()), > + metadata_(controls::controls) > { > } > > @@ -132,7 +136,7 @@ void Request::Private::complete() > ASSERT(request->status() == RequestPending); > ASSERT(!hasPendingBuffers()); > > - request->status_ = cancelled_ ? RequestCancelled : RequestComplete; > + status_ = cancelled_ ? RequestCancelled : RequestComplete; > > LOG(Request, Debug) << request->toString(); > > @@ -174,18 +178,38 @@ void Request::Private::cancel() > > /** > * \brief Reset the request internal data to default values > + * \param[in] flags Indicate whether or not to reuse the buffers > * > * After calling this function, all request internal data will have default > - * values as if the Request::Private instance had just been constructed. > + * values as if the Request::Private instance had just been constructed, with > + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is > + * set in \a flags. > */ > -void Request::Private::reset() > +void Request::Private::reset(Request::ReuseFlag flags) > { > - sequence_ = 0; > + status_ = RequestPending; > cancelled_ = false; > + sequence_ = 0; > prepared_ = false; > + > + controls_.clear(); > + metadata_.clear(); > + > pending_.clear(); > notifiers_.clear(); > timer_.reset(); > + > + if (flags & ReuseBuffers) { > + Request *request = _o<Request>(); > + > + for (auto pair : bufferMap_) { > + FrameBuffer *buffer = pair.second; > + buffer->_d()->setRequest(request); > + pending_.insert(buffer); > + } > + } else { > + bufferMap_.clear(); > + } > } > > /* > @@ -286,9 +310,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) > ASSERT(it != notifiers_.end()); > notifiers_.erase(it); > > - Request *request = _o<Request>(); > LOG(Request, Debug) > - << "Request " << request->cookie() << " buffer " << buffer > + << "Request " << cookie_ << " buffer " << buffer > << " fence signalled"; > > if (!notifiers_.empty()) > @@ -305,8 +328,7 @@ void Request::Private::timeout() > ASSERT(!notifiers_.empty()); > notifiers_.clear(); > > - Request *request = _o<Request>(); > - LOG(Request, Debug) << "Request prepare timeout: " << request->cookie(); > + LOG(Request, Debug) << "Request prepare timeout: " << cookie_; > > cancel(); > > @@ -361,13 +383,11 @@ void Request::Private::timeout() > * completely opaque to libcamera. > */ > Request::Request(Camera *camera, uint64_t cookie) > - : Extensible(std::make_unique<Private>(camera)), > - controls_(camera->controls(), camera->_d()->validator()), > - cookie_(cookie), status_(RequestPending) > + : Extensible(std::make_unique<Private>(camera, cookie)) > { > LIBCAMERA_TRACEPOINT(request_construct, this); > > - LOG(Request, Debug) << "Created request - cookie: " << cookie_; > + LOG(Request, Debug) << "Created request - cookie: " << cookie; > } > > Request::~Request() > @@ -389,26 +409,10 @@ void Request::reuse(ReuseFlag flags) > { > LIBCAMERA_TRACEPOINT(request_reuse, this); > > - _d()->reset(); > - > - if (flags & ReuseBuffers) { > - for (auto pair : bufferMap_) { > - FrameBuffer *buffer = pair.second; > - buffer->_d()->setRequest(this); > - _d()->pending_.insert(buffer); > - } > - } else { > - bufferMap_.clear(); > - } > - > - status_ = RequestPending; > - > - controls_.clear(); > - _d()->metadata_.clear(); > + _d()->reset(flags); > } > > /** > - * \fn Request::controls() > * \brief Retrieve the request's ControlList > * > * Requests store a list of controls to be applied to all frames captured for > @@ -422,6 +426,10 @@ void Request::reuse(ReuseFlag flags) > * > * \return A reference to the ControlList in this request > */ > +ControlList &Request::controls() > +{ > + return _d()->controls_; > +} > > /** > * \brief Retrieve the request's metadata > @@ -433,14 +441,19 @@ const ControlList &Request::metadata() const > } > > /** > - * \fn Request::buffers() > * \brief Retrieve the request's streams to buffers map > * > * Return a reference to the map that associates each Stream part of the > - * request to the FrameBuffer the Stream output should be directed to. > + * request to the FrameBuffer the Stream output should be directed to. If a > + * stream is not utilised in this request there will be no buffer for that > + * stream in the map. > * > * \return The map of Stream to FrameBuffer > */ > +const Request::BufferMap &Request::buffers() const > +{ > + return _d()->bufferMap_; > +} > > /** > * \brief Add a FrameBuffer with its associated Stream to the Request > @@ -493,7 +506,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > return -EEXIST; > } > > - auto [it, inserted] = bufferMap_.try_emplace(stream, buffer); > + auto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer); > if (!inserted) { > LOG(Request, Error) << "FrameBuffer already set for stream"; > return -EEXIST; > @@ -508,15 +521,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > return 0; > } > > -/** > - * \var Request::bufferMap_ > - * \brief Mapping of streams to buffers for this request > - * > - * The bufferMap_ tracks the buffers associated with each stream. If a stream is > - * not utilised in this request there will be no buffer for that stream in the > - * map. > - */ > - > /** > * \brief Return the buffer associated with a stream > * \param[in] stream The stream the buffer is associated to > @@ -525,8 +529,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > */ > FrameBuffer *Request::findBuffer(const Stream *stream) const > { > - const auto it = bufferMap_.find(stream); > - if (it == bufferMap_.end()) > + const auto it = _d()->bufferMap_.find(stream); > + if (it == _d()->bufferMap_.end()) > return nullptr; > > return it->second; > @@ -553,13 +557,15 @@ uint32_t Request::sequence() const > } > > /** > - * \fn Request::cookie() > * \brief Retrieve the cookie set when the request was created > * \return The request cookie > */ > +uint64_t Request::cookie() const > +{ > + return _d()->cookie_; > +} > > /** > - * \fn Request::status() > * \brief Retrieve the request completion status > * > * The request status indicates whether the request has completed successfully > @@ -570,6 +576,10 @@ uint32_t Request::sequence() const > * > * \return The request completion status > */ > +Request::Status Request::status() const > +{ > + return _d()->status_; > +} > > /** > * \brief Check if a request has buffers yet to be completed
On Tue, Jan 13, 2026 at 01:31:06PM +0100, Barnabás Pőcze wrote: > 2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > > The Request class has a set of private member variables, with some of > > them stored in the Request class itself, and some in the > > Request::Private class. Storing the variables in the Request class > > itself has the advantage that accessors can be inline, at the cost of > > ABI breakage if variables need to be added, removed or otherwise > > modified. > > > > The controls_ and metadata_ variables have recently been turned from > > pointers to instances. This broke the ABI. To avoid further breakages, > > move all remaining private member variables to Request::Private. The > > performance impact of not inlining accessors will be negligible. > > I feel like this Request class is moving into a suboptimal direction. > After this change a `Request` is essentially just a single pointer to > its `Private` class. But `std::unique_ptr<Request>` is what is used > in e.g. `Camera::createRequest()`, so essentially it is just introducing > unnecessary indirection, and increases the number of allocations. > > But short of turning `Request` into a `RequestHandle` kind of thing, > I am not sure what would be a good approach. I agree with you, the class effectively becomes a handle. As applications are not supposed to allocate Request instances manually, we could replace the Request::Private mechanism with inheritance. Request::Private could derive from Request, and Camera::createRequest() would allocate an instance of Request::Private and return a pointer to Request. That would however require a virtual destructor. This would break the ABI but not the API. This being said, I don't think the current implementation causes a real performance issue, so I would prefer experimenting with it later and seeing if we should apply the same concept to more classes instead of rushing it out for Request right now. > > While at it, drop an unneeded class forward declaration. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/request.h | 12 ++- > > include/libcamera/request.h | 15 +--- > > src/libcamera/request.cpp | 106 +++++++++++++++------------ > > 3 files changed, 71 insertions(+), 62 deletions(-) > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > index 693097ee9a26..7715077b3f7c 100644 > > --- a/include/libcamera/internal/request.h > > +++ b/include/libcamera/internal/request.h > > @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private > > LIBCAMERA_DECLARE_PUBLIC(Request) > > > > public: > > - Private(Camera *camera); > > + Private(Camera *camera, uint64_t cookie); > > ~Private(); > > > > Camera *camera() const { return camera_; } > > @@ -41,7 +41,7 @@ public: > > bool completeBuffer(FrameBuffer *buffer); > > void complete(); > > void cancel(); > > - void reset(); > > + void reset(Request::ReuseFlag flags); > > > > void prepare(std::chrono::milliseconds timeout = 0ms); > > Signal<> prepared; > > @@ -56,14 +56,20 @@ private: > > void timeout(); > > > > Camera *camera_; > > + const uint64_t cookie_; > > + > > + Status status_; > > bool cancelled_; > > uint32_t sequence_ = 0; > > bool prepared_ = false; > > > > + ControlList controls_; > > + ControlList metadata_; > > + BufferMap bufferMap_; > > + > > std::unordered_set<FrameBuffer *> pending_; > > std::map<FrameBuffer *, EventNotifier> notifiers_; > > std::unique_ptr<Timer> timer_; > > - ControlList metadata_; > > }; > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 290983f61352..08ac6e8daba7 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -22,7 +22,6 @@ > > namespace libcamera { > > > > class Camera; > > -class CameraControlValidator; > > class FrameBuffer; > > class Stream; > > > > @@ -49,16 +48,16 @@ public: > > > > void reuse(ReuseFlag flags = Default); > > > > - ControlList &controls() { return controls_; } > > + ControlList &controls(); > > const ControlList &metadata() const; > > - const BufferMap &buffers() const { return bufferMap_; } > > + const BufferMap &buffers() const; > > int addBuffer(const Stream *stream, FrameBuffer *buffer, > > std::unique_ptr<Fence> &&fence = {}); > > FrameBuffer *findBuffer(const Stream *stream) const; > > > > uint32_t sequence() const; > > - uint64_t cookie() const { return cookie_; } > > - Status status() const { return status_; } > > + uint64_t cookie() const; > > + Status status() const; > > > > bool hasPendingBuffers() const; > > > > @@ -66,12 +65,6 @@ public: > > > > private: > > LIBCAMERA_DISABLE_COPY(Request) > > - > > - ControlList controls_; > > - BufferMap bufferMap_; > > - > > - const uint64_t cookie_; > > - Status status_; > > }; > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index 57f1f060d5b4..9d30091a9af7 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request) > > > > /** > > * \brief Create a Request::Private > > - * \param camera The Camera that creates the request > > + * \param[in] camera The Camera that creates the request > > + * \param[in] cookie Opaque cookie for application use > > * > > * \todo Add a validator for metadata controls. > > */ > > -Request::Private::Private(Camera *camera) > > - : camera_(camera), cancelled_(false), metadata_(controls::controls) > > +Request::Private::Private(Camera *camera, uint64_t cookie) > > + : camera_(camera), cookie_(cookie), status_(RequestPending), > > + cancelled_(false), > > + controls_(camera->controls(), camera->_d()->validator()), > > + metadata_(controls::controls) > > { > > } > > > > @@ -132,7 +136,7 @@ void Request::Private::complete() > > ASSERT(request->status() == RequestPending); > > ASSERT(!hasPendingBuffers()); > > > > - request->status_ = cancelled_ ? RequestCancelled : RequestComplete; > > + status_ = cancelled_ ? RequestCancelled : RequestComplete; > > > > LOG(Request, Debug) << request->toString(); > > > > @@ -174,18 +178,38 @@ void Request::Private::cancel() > > > > /** > > * \brief Reset the request internal data to default values > > + * \param[in] flags Indicate whether or not to reuse the buffers > > * > > * After calling this function, all request internal data will have default > > - * values as if the Request::Private instance had just been constructed. > > + * values as if the Request::Private instance had just been constructed, with > > + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is > > + * set in \a flags. > > */ > > -void Request::Private::reset() > > +void Request::Private::reset(Request::ReuseFlag flags) > > { > > - sequence_ = 0; > > + status_ = RequestPending; > > cancelled_ = false; > > + sequence_ = 0; > > prepared_ = false; > > + > > + controls_.clear(); > > + metadata_.clear(); > > + > > pending_.clear(); > > notifiers_.clear(); > > timer_.reset(); > > + > > + if (flags & ReuseBuffers) { > > + Request *request = _o<Request>(); > > + > > + for (auto pair : bufferMap_) { > > + FrameBuffer *buffer = pair.second; > > + buffer->_d()->setRequest(request); > > + pending_.insert(buffer); > > + } > > + } else { > > + bufferMap_.clear(); > > + } > > } > > > > /* > > @@ -286,9 +310,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) > > ASSERT(it != notifiers_.end()); > > notifiers_.erase(it); > > > > - Request *request = _o<Request>(); > > LOG(Request, Debug) > > - << "Request " << request->cookie() << " buffer " << buffer > > + << "Request " << cookie_ << " buffer " << buffer > > << " fence signalled"; > > > > if (!notifiers_.empty()) > > @@ -305,8 +328,7 @@ void Request::Private::timeout() > > ASSERT(!notifiers_.empty()); > > notifiers_.clear(); > > > > - Request *request = _o<Request>(); > > - LOG(Request, Debug) << "Request prepare timeout: " << request->cookie(); > > + LOG(Request, Debug) << "Request prepare timeout: " << cookie_; > > > > cancel(); > > > > @@ -361,13 +383,11 @@ void Request::Private::timeout() > > * completely opaque to libcamera. > > */ > > Request::Request(Camera *camera, uint64_t cookie) > > - : Extensible(std::make_unique<Private>(camera)), > > - controls_(camera->controls(), camera->_d()->validator()), > > - cookie_(cookie), status_(RequestPending) > > + : Extensible(std::make_unique<Private>(camera, cookie)) > > { > > LIBCAMERA_TRACEPOINT(request_construct, this); > > > > - LOG(Request, Debug) << "Created request - cookie: " << cookie_; > > + LOG(Request, Debug) << "Created request - cookie: " << cookie; > > } > > > > Request::~Request() > > @@ -389,26 +409,10 @@ void Request::reuse(ReuseFlag flags) > > { > > LIBCAMERA_TRACEPOINT(request_reuse, this); > > > > - _d()->reset(); > > - > > - if (flags & ReuseBuffers) { > > - for (auto pair : bufferMap_) { > > - FrameBuffer *buffer = pair.second; > > - buffer->_d()->setRequest(this); > > - _d()->pending_.insert(buffer); > > - } > > - } else { > > - bufferMap_.clear(); > > - } > > - > > - status_ = RequestPending; > > - > > - controls_.clear(); > > - _d()->metadata_.clear(); > > + _d()->reset(flags); > > } > > > > /** > > - * \fn Request::controls() > > * \brief Retrieve the request's ControlList > > * > > * Requests store a list of controls to be applied to all frames captured for > > @@ -422,6 +426,10 @@ void Request::reuse(ReuseFlag flags) > > * > > * \return A reference to the ControlList in this request > > */ > > +ControlList &Request::controls() > > +{ > > + return _d()->controls_; > > +} > > > > /** > > * \brief Retrieve the request's metadata > > @@ -433,14 +441,19 @@ const ControlList &Request::metadata() const > > } > > > > /** > > - * \fn Request::buffers() > > * \brief Retrieve the request's streams to buffers map > > * > > * Return a reference to the map that associates each Stream part of the > > - * request to the FrameBuffer the Stream output should be directed to. > > + * request to the FrameBuffer the Stream output should be directed to. If a > > + * stream is not utilised in this request there will be no buffer for that > > + * stream in the map. > > * > > * \return The map of Stream to FrameBuffer > > */ > > +const Request::BufferMap &Request::buffers() const > > +{ > > + return _d()->bufferMap_; > > +} > > > > /** > > * \brief Add a FrameBuffer with its associated Stream to the Request > > @@ -493,7 +506,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > return -EEXIST; > > } > > > > - auto [it, inserted] = bufferMap_.try_emplace(stream, buffer); > > + auto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer); > > if (!inserted) { > > LOG(Request, Error) << "FrameBuffer already set for stream"; > > return -EEXIST; > > @@ -508,15 +521,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > return 0; > > } > > > > -/** > > - * \var Request::bufferMap_ > > - * \brief Mapping of streams to buffers for this request > > - * > > - * The bufferMap_ tracks the buffers associated with each stream. If a stream is > > - * not utilised in this request there will be no buffer for that stream in the > > - * map. > > - */ > > - > > /** > > * \brief Return the buffer associated with a stream > > * \param[in] stream The stream the buffer is associated to > > @@ -525,8 +529,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > */ > > FrameBuffer *Request::findBuffer(const Stream *stream) const > > { > > - const auto it = bufferMap_.find(stream); > > - if (it == bufferMap_.end()) > > + const auto it = _d()->bufferMap_.find(stream); > > + if (it == _d()->bufferMap_.end()) > > return nullptr; > > > > return it->second; > > @@ -553,13 +557,15 @@ uint32_t Request::sequence() const > > } > > > > /** > > - * \fn Request::cookie() > > * \brief Retrieve the cookie set when the request was created > > * \return The request cookie > > */ > > +uint64_t Request::cookie() const > > +{ > > + return _d()->cookie_; > > +} > > > > /** > > - * \fn Request::status() > > * \brief Retrieve the request completion status > > * > > * The request status indicates whether the request has completed successfully > > @@ -570,6 +576,10 @@ uint32_t Request::sequence() const > > * > > * \return The request completion status > > */ > > +Request::Status Request::status() const > > +{ > > + return _d()->status_; > > +} > > > > /** > > * \brief Check if a request has buffers yet to be completed
Hi Laurent, Quoting Laurent Pinchart (2026-01-13 14:46:15) > On Tue, Jan 13, 2026 at 01:31:06PM +0100, Barnabás Pőcze wrote: > > 2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > > > The Request class has a set of private member variables, with some of > > > them stored in the Request class itself, and some in the > > > Request::Private class. Storing the variables in the Request class > > > itself has the advantage that accessors can be inline, at the cost of > > > ABI breakage if variables need to be added, removed or otherwise > > > modified. > > > > > > The controls_ and metadata_ variables have recently been turned from > > > pointers to instances. This broke the ABI. To avoid further breakages, > > > move all remaining private member variables to Request::Private. The > > > performance impact of not inlining accessors will be negligible. > > > > I feel like this Request class is moving into a suboptimal direction. > > After this change a `Request` is essentially just a single pointer to > > its `Private` class. But `std::unique_ptr<Request>` is what is used > > in e.g. `Camera::createRequest()`, so essentially it is just introducing > > unnecessary indirection, and increases the number of allocations. > > > > But short of turning `Request` into a `RequestHandle` kind of thing, > > I am not sure what would be a good approach. I share similar concerns of Barnabás. Isn't this kind of mixing two concepts? Moving everything into an internal implementation is the pimpl pattern and helps in keeping the ABI stable. My understanding of Request::Private class was that it is thought for private data that should not be visible to the external application. I see that it could also be used to pimpl. But I am not sure if that was the intention. There are other classes like StreamConfiguration that we might want to change in the future in an ABI breaking way, so I don't see (without pimpling everything) how we could completely prevent ABI breaks. So I miss the incentive of breaking the request ABI again (without immediate need) if we don't decide for a strategy libcamera wide. Regards, Stefan > > I agree with you, the class effectively becomes a handle. > > As applications are not supposed to allocate Request instances manually, > we could replace the Request::Private mechanism with inheritance. > Request::Private could derive from Request, and Camera::createRequest() > would allocate an instance of Request::Private and return a pointer to > Request. That would however require a virtual destructor. This would > break the ABI but not the API. > > This being said, I don't think the current implementation causes a real > performance issue, so I would prefer experimenting with it later and > seeing if we should apply the same concept to more classes instead of > rushing it out for Request right now. > > > > While at it, drop an unneeded class forward declaration. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/internal/request.h | 12 ++- > > > include/libcamera/request.h | 15 +--- > > > src/libcamera/request.cpp | 106 +++++++++++++++------------ > > > 3 files changed, 71 insertions(+), 62 deletions(-) > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > > index 693097ee9a26..7715077b3f7c 100644 > > > --- a/include/libcamera/internal/request.h > > > +++ b/include/libcamera/internal/request.h > > > @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private > > > LIBCAMERA_DECLARE_PUBLIC(Request) > > > > > > public: > > > - Private(Camera *camera); > > > + Private(Camera *camera, uint64_t cookie); > > > ~Private(); > > > > > > Camera *camera() const { return camera_; } > > > @@ -41,7 +41,7 @@ public: > > > bool completeBuffer(FrameBuffer *buffer); > > > void complete(); > > > void cancel(); > > > - void reset(); > > > + void reset(Request::ReuseFlag flags); > > > > > > void prepare(std::chrono::milliseconds timeout = 0ms); > > > Signal<> prepared; > > > @@ -56,14 +56,20 @@ private: > > > void timeout(); > > > > > > Camera *camera_; > > > + const uint64_t cookie_; > > > + > > > + Status status_; > > > bool cancelled_; > > > uint32_t sequence_ = 0; > > > bool prepared_ = false; > > > > > > + ControlList controls_; > > > + ControlList metadata_; > > > + BufferMap bufferMap_; > > > + > > > std::unordered_set<FrameBuffer *> pending_; > > > std::map<FrameBuffer *, EventNotifier> notifiers_; > > > std::unique_ptr<Timer> timer_; > > > - ControlList metadata_; > > > }; > > > > > > } /* namespace libcamera */ > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > index 290983f61352..08ac6e8daba7 100644 > > > --- a/include/libcamera/request.h > > > +++ b/include/libcamera/request.h > > > @@ -22,7 +22,6 @@ > > > namespace libcamera { > > > > > > class Camera; > > > -class CameraControlValidator; > > > class FrameBuffer; > > > class Stream; > > > > > > @@ -49,16 +48,16 @@ public: > > > > > > void reuse(ReuseFlag flags = Default); > > > > > > - ControlList &controls() { return controls_; } > > > + ControlList &controls(); > > > const ControlList &metadata() const; > > > - const BufferMap &buffers() const { return bufferMap_; } > > > + const BufferMap &buffers() const; > > > int addBuffer(const Stream *stream, FrameBuffer *buffer, > > > std::unique_ptr<Fence> &&fence = {}); > > > FrameBuffer *findBuffer(const Stream *stream) const; > > > > > > uint32_t sequence() const; > > > - uint64_t cookie() const { return cookie_; } > > > - Status status() const { return status_; } > > > + uint64_t cookie() const; > > > + Status status() const; > > > > > > bool hasPendingBuffers() const; > > > > > > @@ -66,12 +65,6 @@ public: > > > > > > private: > > > LIBCAMERA_DISABLE_COPY(Request) > > > - > > > - ControlList controls_; > > > - BufferMap bufferMap_; > > > - > > > - const uint64_t cookie_; > > > - Status status_; > > > }; > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > index 57f1f060d5b4..9d30091a9af7 100644 > > > --- a/src/libcamera/request.cpp > > > +++ b/src/libcamera/request.cpp > > > @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request) > > > > > > /** > > > * \brief Create a Request::Private > > > - * \param camera The Camera that creates the request > > > + * \param[in] camera The Camera that creates the request > > > + * \param[in] cookie Opaque cookie for application use > > > * > > > * \todo Add a validator for metadata controls. > > > */ > > > -Request::Private::Private(Camera *camera) > > > - : camera_(camera), cancelled_(false), metadata_(controls::controls) > > > +Request::Private::Private(Camera *camera, uint64_t cookie) > > > + : camera_(camera), cookie_(cookie), status_(RequestPending), > > > + cancelled_(false), > > > + controls_(camera->controls(), camera->_d()->validator()), > > > + metadata_(controls::controls) > > > { > > > } > > > > > > @@ -132,7 +136,7 @@ void Request::Private::complete() > > > ASSERT(request->status() == RequestPending); > > > ASSERT(!hasPendingBuffers()); > > > > > > - request->status_ = cancelled_ ? RequestCancelled : RequestComplete; > > > + status_ = cancelled_ ? RequestCancelled : RequestComplete; > > > > > > LOG(Request, Debug) << request->toString(); > > > > > > @@ -174,18 +178,38 @@ void Request::Private::cancel() > > > > > > /** > > > * \brief Reset the request internal data to default values > > > + * \param[in] flags Indicate whether or not to reuse the buffers > > > * > > > * After calling this function, all request internal data will have default > > > - * values as if the Request::Private instance had just been constructed. > > > + * values as if the Request::Private instance had just been constructed, with > > > + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is > > > + * set in \a flags. > > > */ > > > -void Request::Private::reset() > > > +void Request::Private::reset(Request::ReuseFlag flags) > > > { > > > - sequence_ = 0; > > > + status_ = RequestPending; > > > cancelled_ = false; > > > + sequence_ = 0; > > > prepared_ = false; > > > + > > > + controls_.clear(); > > > + metadata_.clear(); > > > + > > > pending_.clear(); > > > notifiers_.clear(); > > > timer_.reset(); > > > + > > > + if (flags & ReuseBuffers) { > > > + Request *request = _o<Request>(); > > > + > > > + for (auto pair : bufferMap_) { > > > + FrameBuffer *buffer = pair.second; > > > + buffer->_d()->setRequest(request); > > > + pending_.insert(buffer); > > > + } > > > + } else { > > > + bufferMap_.clear(); > > > + } > > > } > > > > > > /* > > > @@ -286,9 +310,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) > > > ASSERT(it != notifiers_.end()); > > > notifiers_.erase(it); > > > > > > - Request *request = _o<Request>(); > > > LOG(Request, Debug) > > > - << "Request " << request->cookie() << " buffer " << buffer > > > + << "Request " << cookie_ << " buffer " << buffer > > > << " fence signalled"; > > > > > > if (!notifiers_.empty()) > > > @@ -305,8 +328,7 @@ void Request::Private::timeout() > > > ASSERT(!notifiers_.empty()); > > > notifiers_.clear(); > > > > > > - Request *request = _o<Request>(); > > > - LOG(Request, Debug) << "Request prepare timeout: " << request->cookie(); > > > + LOG(Request, Debug) << "Request prepare timeout: " << cookie_; > > > > > > cancel(); > > > > > > @@ -361,13 +383,11 @@ void Request::Private::timeout() > > > * completely opaque to libcamera. > > > */ > > > Request::Request(Camera *camera, uint64_t cookie) > > > - : Extensible(std::make_unique<Private>(camera)), > > > - controls_(camera->controls(), camera->_d()->validator()), > > > - cookie_(cookie), status_(RequestPending) > > > + : Extensible(std::make_unique<Private>(camera, cookie)) > > > { > > > LIBCAMERA_TRACEPOINT(request_construct, this); > > > > > > - LOG(Request, Debug) << "Created request - cookie: " << cookie_; > > > + LOG(Request, Debug) << "Created request - cookie: " << cookie; > > > } > > > > > > Request::~Request() > > > @@ -389,26 +409,10 @@ void Request::reuse(ReuseFlag flags) > > > { > > > LIBCAMERA_TRACEPOINT(request_reuse, this); > > > > > > - _d()->reset(); > > > - > > > - if (flags & ReuseBuffers) { > > > - for (auto pair : bufferMap_) { > > > - FrameBuffer *buffer = pair.second; > > > - buffer->_d()->setRequest(this); > > > - _d()->pending_.insert(buffer); > > > - } > > > - } else { > > > - bufferMap_.clear(); > > > - } > > > - > > > - status_ = RequestPending; > > > - > > > - controls_.clear(); > > > - _d()->metadata_.clear(); > > > + _d()->reset(flags); > > > } > > > > > > /** > > > - * \fn Request::controls() > > > * \brief Retrieve the request's ControlList > > > * > > > * Requests store a list of controls to be applied to all frames captured for > > > @@ -422,6 +426,10 @@ void Request::reuse(ReuseFlag flags) > > > * > > > * \return A reference to the ControlList in this request > > > */ > > > +ControlList &Request::controls() > > > +{ > > > + return _d()->controls_; > > > +} > > > > > > /** > > > * \brief Retrieve the request's metadata > > > @@ -433,14 +441,19 @@ const ControlList &Request::metadata() const > > > } > > > > > > /** > > > - * \fn Request::buffers() > > > * \brief Retrieve the request's streams to buffers map > > > * > > > * Return a reference to the map that associates each Stream part of the > > > - * request to the FrameBuffer the Stream output should be directed to. > > > + * request to the FrameBuffer the Stream output should be directed to. If a > > > + * stream is not utilised in this request there will be no buffer for that > > > + * stream in the map. > > > * > > > * \return The map of Stream to FrameBuffer > > > */ > > > +const Request::BufferMap &Request::buffers() const > > > +{ > > > + return _d()->bufferMap_; > > > +} > > > > > > /** > > > * \brief Add a FrameBuffer with its associated Stream to the Request > > > @@ -493,7 +506,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > return -EEXIST; > > > } > > > > > > - auto [it, inserted] = bufferMap_.try_emplace(stream, buffer); > > > + auto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer); > > > if (!inserted) { > > > LOG(Request, Error) << "FrameBuffer already set for stream"; > > > return -EEXIST; > > > @@ -508,15 +521,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > return 0; > > > } > > > > > > -/** > > > - * \var Request::bufferMap_ > > > - * \brief Mapping of streams to buffers for this request > > > - * > > > - * The bufferMap_ tracks the buffers associated with each stream. If a stream is > > > - * not utilised in this request there will be no buffer for that stream in the > > > - * map. > > > - */ > > > - > > > /** > > > * \brief Return the buffer associated with a stream > > > * \param[in] stream The stream the buffer is associated to > > > @@ -525,8 +529,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > */ > > > FrameBuffer *Request::findBuffer(const Stream *stream) const > > > { > > > - const auto it = bufferMap_.find(stream); > > > - if (it == bufferMap_.end()) > > > + const auto it = _d()->bufferMap_.find(stream); > > > + if (it == _d()->bufferMap_.end()) > > > return nullptr; > > > > > > return it->second; > > > @@ -553,13 +557,15 @@ uint32_t Request::sequence() const > > > } > > > > > > /** > > > - * \fn Request::cookie() > > > * \brief Retrieve the cookie set when the request was created > > > * \return The request cookie > > > */ > > > +uint64_t Request::cookie() const > > > +{ > > > + return _d()->cookie_; > > > +} > > > > > > /** > > > - * \fn Request::status() > > > * \brief Retrieve the request completion status > > > * > > > * The request status indicates whether the request has completed successfully > > > @@ -570,6 +576,10 @@ uint32_t Request::sequence() const > > > * > > > * \return The request completion status > > > */ > > > +Request::Status Request::status() const > > > +{ > > > + return _d()->status_; > > > +} > > > > > > /** > > > * \brief Check if a request has buffers yet to be completed > > -- > Regards, > > Laurent Pinchart
On Wed, Jan 14, 2026 at 10:49:35AM +0100, Stefan Klug wrote: > Quoting Laurent Pinchart (2026-01-13 14:46:15) > > On Tue, Jan 13, 2026 at 01:31:06PM +0100, Barnabás Pőcze wrote: > > > 2026. 01. 13. 1:07 keltezéssel, Laurent Pinchart írta: > > > > The Request class has a set of private member variables, with some of > > > > them stored in the Request class itself, and some in the > > > > Request::Private class. Storing the variables in the Request class > > > > itself has the advantage that accessors can be inline, at the cost of > > > > ABI breakage if variables need to be added, removed or otherwise > > > > modified. > > > > > > > > The controls_ and metadata_ variables have recently been turned from > > > > pointers to instances. This broke the ABI. To avoid further breakages, > > > > move all remaining private member variables to Request::Private. The > > > > performance impact of not inlining accessors will be negligible. > > > > > > I feel like this Request class is moving into a suboptimal direction. > > > After this change a `Request` is essentially just a single pointer to > > > its `Private` class. But `std::unique_ptr<Request>` is what is used > > > in e.g. `Camera::createRequest()`, so essentially it is just introducing > > > unnecessary indirection, and increases the number of allocations. > > > > > > But short of turning `Request` into a `RequestHandle` kind of thing, > > > I am not sure what would be a good approach. > > I share similar concerns of Barnabás. Isn't this kind of mixing two > concepts? Moving everything into an internal implementation is the pimpl > pattern and helps in keeping the ABI stable. My understanding of > Request::Private class was that it is thought for private data that > should not be visible to the external application. I see that it could > also be used to pimpl. But I am not sure if that was the intention. The Private classes are meant to implement the pimpl pattern. They also help making internal data available internally, but that's not the main goal. We used friend statements for that purpose before. Using Private for this purpose is nicer in my opinion, but it's still not the original goal. > There are other classes like StreamConfiguration that we might want to > change in the future in an ABI breaking way, so I don't see (without > pimpling everything) how we could completely prevent ABI breaks. Once we'll have a C API everything will be private and opaque, except for arguments passed to functions. > So I miss the incentive of breaking the request ABI again (without > immediate need) if we don't decide for a strategy libcamera wide. This patch was written before the last breakage that changed the handling of controls_ and metadata_. I had another patch in the series that turned the dynamically allocated members into unique_ptr, and as that broke the ABI, I decided to make everything private to avoid future breakages. That patch got dropped as the two member variables are now embedded in the Request or Request::Private classes, but I still think moving the last members to Request::Private will minimize future ABI breakages. Note that this particular ABI breakage doesn't change the API, it only requires recompilation. Given that we have other ABI breakages queued in the master branch (if I recall correctly), merging this one will not introduce any further inconvienence. > > I agree with you, the class effectively becomes a handle. > > > > As applications are not supposed to allocate Request instances manually, > > we could replace the Request::Private mechanism with inheritance. > > Request::Private could derive from Request, and Camera::createRequest() > > would allocate an instance of Request::Private and return a pointer to > > Request. That would however require a virtual destructor. This would > > break the ABI but not the API. > > > > This being said, I don't think the current implementation causes a real > > performance issue, so I would prefer experimenting with it later and > > seeing if we should apply the same concept to more classes instead of > > rushing it out for Request right now. > > > > > > While at it, drop an unneeded class forward declaration. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > include/libcamera/internal/request.h | 12 ++- > > > > include/libcamera/request.h | 15 +--- > > > > src/libcamera/request.cpp | 106 +++++++++++++++------------ > > > > 3 files changed, 71 insertions(+), 62 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h > > > > index 693097ee9a26..7715077b3f7c 100644 > > > > --- a/include/libcamera/internal/request.h > > > > +++ b/include/libcamera/internal/request.h > > > > @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private > > > > LIBCAMERA_DECLARE_PUBLIC(Request) > > > > > > > > public: > > > > - Private(Camera *camera); > > > > + Private(Camera *camera, uint64_t cookie); > > > > ~Private(); > > > > > > > > Camera *camera() const { return camera_; } > > > > @@ -41,7 +41,7 @@ public: > > > > bool completeBuffer(FrameBuffer *buffer); > > > > void complete(); > > > > void cancel(); > > > > - void reset(); > > > > + void reset(Request::ReuseFlag flags); > > > > > > > > void prepare(std::chrono::milliseconds timeout = 0ms); > > > > Signal<> prepared; > > > > @@ -56,14 +56,20 @@ private: > > > > void timeout(); > > > > > > > > Camera *camera_; > > > > + const uint64_t cookie_; > > > > + > > > > + Status status_; > > > > bool cancelled_; > > > > uint32_t sequence_ = 0; > > > > bool prepared_ = false; > > > > > > > > + ControlList controls_; > > > > + ControlList metadata_; > > > > + BufferMap bufferMap_; > > > > + > > > > std::unordered_set<FrameBuffer *> pending_; > > > > std::map<FrameBuffer *, EventNotifier> notifiers_; > > > > std::unique_ptr<Timer> timer_; > > > > - ControlList metadata_; > > > > }; > > > > > > > > } /* namespace libcamera */ > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > > > index 290983f61352..08ac6e8daba7 100644 > > > > --- a/include/libcamera/request.h > > > > +++ b/include/libcamera/request.h > > > > @@ -22,7 +22,6 @@ > > > > namespace libcamera { > > > > > > > > class Camera; > > > > -class CameraControlValidator; > > > > class FrameBuffer; > > > > class Stream; > > > > > > > > @@ -49,16 +48,16 @@ public: > > > > > > > > void reuse(ReuseFlag flags = Default); > > > > > > > > - ControlList &controls() { return controls_; } > > > > + ControlList &controls(); > > > > const ControlList &metadata() const; > > > > - const BufferMap &buffers() const { return bufferMap_; } > > > > + const BufferMap &buffers() const; > > > > int addBuffer(const Stream *stream, FrameBuffer *buffer, > > > > std::unique_ptr<Fence> &&fence = {}); > > > > FrameBuffer *findBuffer(const Stream *stream) const; > > > > > > > > uint32_t sequence() const; > > > > - uint64_t cookie() const { return cookie_; } > > > > - Status status() const { return status_; } > > > > + uint64_t cookie() const; > > > > + Status status() const; > > > > > > > > bool hasPendingBuffers() const; > > > > > > > > @@ -66,12 +65,6 @@ public: > > > > > > > > private: > > > > LIBCAMERA_DISABLE_COPY(Request) > > > > - > > > > - ControlList controls_; > > > > - BufferMap bufferMap_; > > > > - > > > > - const uint64_t cookie_; > > > > - Status status_; > > > > }; > > > > > > > > std::ostream &operator<<(std::ostream &out, const Request &r); > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > > > index 57f1f060d5b4..9d30091a9af7 100644 > > > > --- a/src/libcamera/request.cpp > > > > +++ b/src/libcamera/request.cpp > > > > @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request) > > > > > > > > /** > > > > * \brief Create a Request::Private > > > > - * \param camera The Camera that creates the request > > > > + * \param[in] camera The Camera that creates the request > > > > + * \param[in] cookie Opaque cookie for application use > > > > * > > > > * \todo Add a validator for metadata controls. > > > > */ > > > > -Request::Private::Private(Camera *camera) > > > > - : camera_(camera), cancelled_(false), metadata_(controls::controls) > > > > +Request::Private::Private(Camera *camera, uint64_t cookie) > > > > + : camera_(camera), cookie_(cookie), status_(RequestPending), > > > > + cancelled_(false), > > > > + controls_(camera->controls(), camera->_d()->validator()), > > > > + metadata_(controls::controls) > > > > { > > > > } > > > > > > > > @@ -132,7 +136,7 @@ void Request::Private::complete() > > > > ASSERT(request->status() == RequestPending); > > > > ASSERT(!hasPendingBuffers()); > > > > > > > > - request->status_ = cancelled_ ? RequestCancelled : RequestComplete; > > > > + status_ = cancelled_ ? RequestCancelled : RequestComplete; > > > > > > > > LOG(Request, Debug) << request->toString(); > > > > > > > > @@ -174,18 +178,38 @@ void Request::Private::cancel() > > > > > > > > /** > > > > * \brief Reset the request internal data to default values > > > > + * \param[in] flags Indicate whether or not to reuse the buffers > > > > * > > > > * After calling this function, all request internal data will have default > > > > - * values as if the Request::Private instance had just been constructed. > > > > + * values as if the Request::Private instance had just been constructed, with > > > > + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is > > > > + * set in \a flags. > > > > */ > > > > -void Request::Private::reset() > > > > +void Request::Private::reset(Request::ReuseFlag flags) > > > > { > > > > - sequence_ = 0; > > > > + status_ = RequestPending; > > > > cancelled_ = false; > > > > + sequence_ = 0; > > > > prepared_ = false; > > > > + > > > > + controls_.clear(); > > > > + metadata_.clear(); > > > > + > > > > pending_.clear(); > > > > notifiers_.clear(); > > > > timer_.reset(); > > > > + > > > > + if (flags & ReuseBuffers) { > > > > + Request *request = _o<Request>(); > > > > + > > > > + for (auto pair : bufferMap_) { > > > > + FrameBuffer *buffer = pair.second; > > > > + buffer->_d()->setRequest(request); > > > > + pending_.insert(buffer); > > > > + } > > > > + } else { > > > > + bufferMap_.clear(); > > > > + } > > > > } > > > > > > > > /* > > > > @@ -286,9 +310,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) > > > > ASSERT(it != notifiers_.end()); > > > > notifiers_.erase(it); > > > > > > > > - Request *request = _o<Request>(); > > > > LOG(Request, Debug) > > > > - << "Request " << request->cookie() << " buffer " << buffer > > > > + << "Request " << cookie_ << " buffer " << buffer > > > > << " fence signalled"; > > > > > > > > if (!notifiers_.empty()) > > > > @@ -305,8 +328,7 @@ void Request::Private::timeout() > > > > ASSERT(!notifiers_.empty()); > > > > notifiers_.clear(); > > > > > > > > - Request *request = _o<Request>(); > > > > - LOG(Request, Debug) << "Request prepare timeout: " << request->cookie(); > > > > + LOG(Request, Debug) << "Request prepare timeout: " << cookie_; > > > > > > > > cancel(); > > > > > > > > @@ -361,13 +383,11 @@ void Request::Private::timeout() > > > > * completely opaque to libcamera. > > > > */ > > > > Request::Request(Camera *camera, uint64_t cookie) > > > > - : Extensible(std::make_unique<Private>(camera)), > > > > - controls_(camera->controls(), camera->_d()->validator()), > > > > - cookie_(cookie), status_(RequestPending) > > > > + : Extensible(std::make_unique<Private>(camera, cookie)) > > > > { > > > > LIBCAMERA_TRACEPOINT(request_construct, this); > > > > > > > > - LOG(Request, Debug) << "Created request - cookie: " << cookie_; > > > > + LOG(Request, Debug) << "Created request - cookie: " << cookie; > > > > } > > > > > > > > Request::~Request() > > > > @@ -389,26 +409,10 @@ void Request::reuse(ReuseFlag flags) > > > > { > > > > LIBCAMERA_TRACEPOINT(request_reuse, this); > > > > > > > > - _d()->reset(); > > > > - > > > > - if (flags & ReuseBuffers) { > > > > - for (auto pair : bufferMap_) { > > > > - FrameBuffer *buffer = pair.second; > > > > - buffer->_d()->setRequest(this); > > > > - _d()->pending_.insert(buffer); > > > > - } > > > > - } else { > > > > - bufferMap_.clear(); > > > > - } > > > > - > > > > - status_ = RequestPending; > > > > - > > > > - controls_.clear(); > > > > - _d()->metadata_.clear(); > > > > + _d()->reset(flags); > > > > } > > > > > > > > /** > > > > - * \fn Request::controls() > > > > * \brief Retrieve the request's ControlList > > > > * > > > > * Requests store a list of controls to be applied to all frames captured for > > > > @@ -422,6 +426,10 @@ void Request::reuse(ReuseFlag flags) > > > > * > > > > * \return A reference to the ControlList in this request > > > > */ > > > > +ControlList &Request::controls() > > > > +{ > > > > + return _d()->controls_; > > > > +} > > > > > > > > /** > > > > * \brief Retrieve the request's metadata > > > > @@ -433,14 +441,19 @@ const ControlList &Request::metadata() const > > > > } > > > > > > > > /** > > > > - * \fn Request::buffers() > > > > * \brief Retrieve the request's streams to buffers map > > > > * > > > > * Return a reference to the map that associates each Stream part of the > > > > - * request to the FrameBuffer the Stream output should be directed to. > > > > + * request to the FrameBuffer the Stream output should be directed to. If a > > > > + * stream is not utilised in this request there will be no buffer for that > > > > + * stream in the map. > > > > * > > > > * \return The map of Stream to FrameBuffer > > > > */ > > > > +const Request::BufferMap &Request::buffers() const > > > > +{ > > > > + return _d()->bufferMap_; > > > > +} > > > > > > > > /** > > > > * \brief Add a FrameBuffer with its associated Stream to the Request > > > > @@ -493,7 +506,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > > return -EEXIST; > > > > } > > > > > > > > - auto [it, inserted] = bufferMap_.try_emplace(stream, buffer); > > > > + auto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer); > > > > if (!inserted) { > > > > LOG(Request, Error) << "FrameBuffer already set for stream"; > > > > return -EEXIST; > > > > @@ -508,15 +521,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > > return 0; > > > > } > > > > > > > > -/** > > > > - * \var Request::bufferMap_ > > > > - * \brief Mapping of streams to buffers for this request > > > > - * > > > > - * The bufferMap_ tracks the buffers associated with each stream. If a stream is > > > > - * not utilised in this request there will be no buffer for that stream in the > > > > - * map. > > > > - */ > > > > - > > > > /** > > > > * \brief Return the buffer associated with a stream > > > > * \param[in] stream The stream the buffer is associated to > > > > @@ -525,8 +529,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, > > > > */ > > > > FrameBuffer *Request::findBuffer(const Stream *stream) const > > > > { > > > > - const auto it = bufferMap_.find(stream); > > > > - if (it == bufferMap_.end()) > > > > + const auto it = _d()->bufferMap_.find(stream); > > > > + if (it == _d()->bufferMap_.end()) > > > > return nullptr; > > > > > > > > return it->second; > > > > @@ -553,13 +557,15 @@ uint32_t Request::sequence() const > > > > } > > > > > > > > /** > > > > - * \fn Request::cookie() > > > > * \brief Retrieve the cookie set when the request was created > > > > * \return The request cookie > > > > */ > > > > +uint64_t Request::cookie() const > > > > +{ > > > > + return _d()->cookie_; > > > > +} > > > > > > > > /** > > > > - * \fn Request::status() > > > > * \brief Retrieve the request completion status > > > > * > > > > * The request status indicates whether the request has completed successfully > > > > @@ -570,6 +576,10 @@ uint32_t Request::sequence() const > > > > * > > > > * \return The request completion status > > > > */ > > > > +Request::Status Request::status() const > > > > +{ > > > > + return _d()->status_; > > > > +} > > > > > > > > /** > > > > * \brief Check if a request has buffers yet to be completed
diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h index 693097ee9a26..7715077b3f7c 100644 --- a/include/libcamera/internal/request.h +++ b/include/libcamera/internal/request.h @@ -30,7 +30,7 @@ class Request::Private : public Extensible::Private LIBCAMERA_DECLARE_PUBLIC(Request) public: - Private(Camera *camera); + Private(Camera *camera, uint64_t cookie); ~Private(); Camera *camera() const { return camera_; } @@ -41,7 +41,7 @@ public: bool completeBuffer(FrameBuffer *buffer); void complete(); void cancel(); - void reset(); + void reset(Request::ReuseFlag flags); void prepare(std::chrono::milliseconds timeout = 0ms); Signal<> prepared; @@ -56,14 +56,20 @@ private: void timeout(); Camera *camera_; + const uint64_t cookie_; + + Status status_; bool cancelled_; uint32_t sequence_ = 0; bool prepared_ = false; + ControlList controls_; + ControlList metadata_; + BufferMap bufferMap_; + std::unordered_set<FrameBuffer *> pending_; std::map<FrameBuffer *, EventNotifier> notifiers_; std::unique_ptr<Timer> timer_; - ControlList metadata_; }; } /* namespace libcamera */ diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 290983f61352..08ac6e8daba7 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -22,7 +22,6 @@ namespace libcamera { class Camera; -class CameraControlValidator; class FrameBuffer; class Stream; @@ -49,16 +48,16 @@ public: void reuse(ReuseFlag flags = Default); - ControlList &controls() { return controls_; } + ControlList &controls(); const ControlList &metadata() const; - const BufferMap &buffers() const { return bufferMap_; } + const BufferMap &buffers() const; int addBuffer(const Stream *stream, FrameBuffer *buffer, std::unique_ptr<Fence> &&fence = {}); FrameBuffer *findBuffer(const Stream *stream) const; uint32_t sequence() const; - uint64_t cookie() const { return cookie_; } - Status status() const { return status_; } + uint64_t cookie() const; + Status status() const; bool hasPendingBuffers() const; @@ -66,12 +65,6 @@ public: private: LIBCAMERA_DISABLE_COPY(Request) - - ControlList controls_; - BufferMap bufferMap_; - - const uint64_t cookie_; - Status status_; }; std::ostream &operator<<(std::ostream &out, const Request &r); diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index 57f1f060d5b4..9d30091a9af7 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -52,12 +52,16 @@ LOG_DEFINE_CATEGORY(Request) /** * \brief Create a Request::Private - * \param camera The Camera that creates the request + * \param[in] camera The Camera that creates the request + * \param[in] cookie Opaque cookie for application use * * \todo Add a validator for metadata controls. */ -Request::Private::Private(Camera *camera) - : camera_(camera), cancelled_(false), metadata_(controls::controls) +Request::Private::Private(Camera *camera, uint64_t cookie) + : camera_(camera), cookie_(cookie), status_(RequestPending), + cancelled_(false), + controls_(camera->controls(), camera->_d()->validator()), + metadata_(controls::controls) { } @@ -132,7 +136,7 @@ void Request::Private::complete() ASSERT(request->status() == RequestPending); ASSERT(!hasPendingBuffers()); - request->status_ = cancelled_ ? RequestCancelled : RequestComplete; + status_ = cancelled_ ? RequestCancelled : RequestComplete; LOG(Request, Debug) << request->toString(); @@ -174,18 +178,38 @@ void Request::Private::cancel() /** * \brief Reset the request internal data to default values + * \param[in] flags Indicate whether or not to reuse the buffers * * After calling this function, all request internal data will have default - * values as if the Request::Private instance had just been constructed. + * values as if the Request::Private instance had just been constructed, with + * the exception of bufferMap_ if the Request::ReuseFlag::ReuseBuffers flag is + * set in \a flags. */ -void Request::Private::reset() +void Request::Private::reset(Request::ReuseFlag flags) { - sequence_ = 0; + status_ = RequestPending; cancelled_ = false; + sequence_ = 0; prepared_ = false; + + controls_.clear(); + metadata_.clear(); + pending_.clear(); notifiers_.clear(); timer_.reset(); + + if (flags & ReuseBuffers) { + Request *request = _o<Request>(); + + for (auto pair : bufferMap_) { + FrameBuffer *buffer = pair.second; + buffer->_d()->setRequest(request); + pending_.insert(buffer); + } + } else { + bufferMap_.clear(); + } } /* @@ -286,9 +310,8 @@ void Request::Private::notifierActivated(FrameBuffer *buffer) ASSERT(it != notifiers_.end()); notifiers_.erase(it); - Request *request = _o<Request>(); LOG(Request, Debug) - << "Request " << request->cookie() << " buffer " << buffer + << "Request " << cookie_ << " buffer " << buffer << " fence signalled"; if (!notifiers_.empty()) @@ -305,8 +328,7 @@ void Request::Private::timeout() ASSERT(!notifiers_.empty()); notifiers_.clear(); - Request *request = _o<Request>(); - LOG(Request, Debug) << "Request prepare timeout: " << request->cookie(); + LOG(Request, Debug) << "Request prepare timeout: " << cookie_; cancel(); @@ -361,13 +383,11 @@ void Request::Private::timeout() * completely opaque to libcamera. */ Request::Request(Camera *camera, uint64_t cookie) - : Extensible(std::make_unique<Private>(camera)), - controls_(camera->controls(), camera->_d()->validator()), - cookie_(cookie), status_(RequestPending) + : Extensible(std::make_unique<Private>(camera, cookie)) { LIBCAMERA_TRACEPOINT(request_construct, this); - LOG(Request, Debug) << "Created request - cookie: " << cookie_; + LOG(Request, Debug) << "Created request - cookie: " << cookie; } Request::~Request() @@ -389,26 +409,10 @@ void Request::reuse(ReuseFlag flags) { LIBCAMERA_TRACEPOINT(request_reuse, this); - _d()->reset(); - - if (flags & ReuseBuffers) { - for (auto pair : bufferMap_) { - FrameBuffer *buffer = pair.second; - buffer->_d()->setRequest(this); - _d()->pending_.insert(buffer); - } - } else { - bufferMap_.clear(); - } - - status_ = RequestPending; - - controls_.clear(); - _d()->metadata_.clear(); + _d()->reset(flags); } /** - * \fn Request::controls() * \brief Retrieve the request's ControlList * * Requests store a list of controls to be applied to all frames captured for @@ -422,6 +426,10 @@ void Request::reuse(ReuseFlag flags) * * \return A reference to the ControlList in this request */ +ControlList &Request::controls() +{ + return _d()->controls_; +} /** * \brief Retrieve the request's metadata @@ -433,14 +441,19 @@ const ControlList &Request::metadata() const } /** - * \fn Request::buffers() * \brief Retrieve the request's streams to buffers map * * Return a reference to the map that associates each Stream part of the - * request to the FrameBuffer the Stream output should be directed to. + * request to the FrameBuffer the Stream output should be directed to. If a + * stream is not utilised in this request there will be no buffer for that + * stream in the map. * * \return The map of Stream to FrameBuffer */ +const Request::BufferMap &Request::buffers() const +{ + return _d()->bufferMap_; +} /** * \brief Add a FrameBuffer with its associated Stream to the Request @@ -493,7 +506,7 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, return -EEXIST; } - auto [it, inserted] = bufferMap_.try_emplace(stream, buffer); + auto [it, inserted] = _d()->bufferMap_.try_emplace(stream, buffer); if (!inserted) { LOG(Request, Error) << "FrameBuffer already set for stream"; return -EEXIST; @@ -508,15 +521,6 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, return 0; } -/** - * \var Request::bufferMap_ - * \brief Mapping of streams to buffers for this request - * - * The bufferMap_ tracks the buffers associated with each stream. If a stream is - * not utilised in this request there will be no buffer for that stream in the - * map. - */ - /** * \brief Return the buffer associated with a stream * \param[in] stream The stream the buffer is associated to @@ -525,8 +529,8 @@ int Request::addBuffer(const Stream *stream, FrameBuffer *buffer, */ FrameBuffer *Request::findBuffer(const Stream *stream) const { - const auto it = bufferMap_.find(stream); - if (it == bufferMap_.end()) + const auto it = _d()->bufferMap_.find(stream); + if (it == _d()->bufferMap_.end()) return nullptr; return it->second; @@ -553,13 +557,15 @@ uint32_t Request::sequence() const } /** - * \fn Request::cookie() * \brief Retrieve the cookie set when the request was created * \return The request cookie */ +uint64_t Request::cookie() const +{ + return _d()->cookie_; +} /** - * \fn Request::status() * \brief Retrieve the request completion status * * The request status indicates whether the request has completed successfully @@ -570,6 +576,10 @@ uint32_t Request::sequence() const * * \return The request completion status */ +Request::Status Request::status() const +{ + return _d()->status_; +} /** * \brief Check if a request has buffers yet to be completed
The Request class has a set of private member variables, with some of them stored in the Request class itself, and some in the Request::Private class. Storing the variables in the Request class itself has the advantage that accessors can be inline, at the cost of ABI breakage if variables need to be added, removed or otherwise modified. The controls_ and metadata_ variables have recently been turned from pointers to instances. This broke the ABI. To avoid further breakages, move all remaining private member variables to Request::Private. The performance impact of not inlining accessors will be negligible. While at it, drop an unneeded class forward declaration. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/request.h | 12 ++- include/libcamera/request.h | 15 +--- src/libcamera/request.cpp | 106 +++++++++++++++------------ 3 files changed, 71 insertions(+), 62 deletions(-)