[v2,3/3] libcamera: request: Move all members to internal private class
diff mbox series

Message ID 20250331141714.512538-3-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v2,1/3] libcamera: request: Make `controls_`/`metadata_` members
Related show

Commit Message

Barnabás Pőcze March 31, 2025, 2:17 p.m. UTC 
Move all members of `Request` into `Request::Private`.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/request.h |  10 ++-
 include/libcamera/request.h          |  18 ++---
 src/libcamera/request.cpp            | 104 ++++++++++++++-------------
 3 files changed, 69 insertions(+), 63 deletions(-)

Comments

Kieran Bingham March 31, 2025, 5:59 p.m. UTC | #1 
Quoting Barnabás Pőcze (2025-03-31 15:17:14)
> Move all members of `Request` into `Request::Private`.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  include/libcamera/internal/request.h |  10 ++-
>  include/libcamera/request.h          |  18 ++---
>  src/libcamera/request.cpp            | 104 ++++++++++++++-------------
>  3 files changed, 69 insertions(+), 63 deletions(-)
> 
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 73e9bb5cc..009ebbf14 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_; }
> @@ -39,7 +39,7 @@ public:
>         bool completeBuffer(FrameBuffer *buffer);
>         void complete();
>         void cancel();
> -       void reset();
> +       void reset(ReuseFlag flags);
>  
>         void prepare(std::chrono::milliseconds timeout = 0ms);
>         Signal<> prepared;
> @@ -57,10 +57,16 @@ private:
>         bool cancelled_;
>         uint32_t sequence_ = 0;
>         bool prepared_ = false;
> +       Status status_ = RequestPending;
> +       const uint64_t cookie_;
>  
>         std::unordered_set<FrameBuffer *> pending_;
>         std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>         std::unique_ptr<Timer> timer_;
> +
> +       ControlList controls_;
> +       ControlList metadata_;
> +       BufferMap bufferMap_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 3061e2fb0..609b55885 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -49,16 +49,17 @@ public:
>  
>         void reuse(ReuseFlag flags = Default);
>  
> -       ControlList &controls() { return controls_; }
> -       ControlList &metadata() { return metadata_; }
> -       const BufferMap &buffers() const { return bufferMap_; }
> +       ControlList &controls();
> +       ControlList &metadata();
> +
> +       const BufferMap &buffers() const;
>         int addBuffer(const Stream *stream, FrameBuffer *buffer,
>                       std::unique_ptr<Fence> fence = nullptr);
>         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,13 +67,6 @@ public:
>  
>  private:
>         LIBCAMERA_DISABLE_COPY(Request)
> -
> -       ControlList controls_;
> -       ControlList metadata_;
> -       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 fc364441a..1f1be1c7e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -53,9 +53,12 @@ LOG_DEFINE_CATEGORY(Request)
>  /**
>   * \brief Create a Request::Private
>   * \param camera The Camera that creates the request
> + * \param cookie Opaque cookie for application use
>   */
> -Request::Private::Private(Camera *camera)
> -       : camera_(camera), cancelled_(false)
> +Request::Private::Private(Camera *camera, uint64_t cookie)
> +       : camera_(camera), cancelled_(false), cookie_(cookie),
> +         controls_(controls::controls, camera->_d()->validator()),
> +         metadata_(controls::controls) /* \todo Add a validator for metadata controls. */
>  {
>  }
>  
> @@ -121,10 +124,10 @@ void Request::Private::complete()
>  {
>         Request *request = _o<Request>();
>  
> -       ASSERT(request->status() == RequestPending);
> +       ASSERT(status_ == RequestPending);
>         ASSERT(!hasPendingBuffers());
>  
> -       request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> +       status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
>         LOG(Request, Debug) << request->toString();
>  
> @@ -158,26 +161,39 @@ void Request::Private::cancel()
>  {
>         LIBCAMERA_TRACEPOINT(request_cancel, this);
>  
> -       Request *request = _o<Request>();
> -       ASSERT(request->status() == RequestPending);
> +       ASSERT(status_ == RequestPending);
>  
>         doCancelRequest();
>  }
>  
>  /**
> - * \brief Reset the request internal data to default values
> - *
> - * After calling this function, all request internal data will have default
> - * values as if the Request::Private instance had just been constructed.
> + * \copydoc Request::reuse()
> + * \sa Request::reuse()
>   */
> -void Request::Private::reset()
> +void Request::Private::reset(Request::ReuseFlag flags)
>  {
>         sequence_ = 0;
>         cancelled_ = false;
>         prepared_ = false;
> +       status_ = RequestPending;
> +
>         pending_.clear();
>         notifiers_.clear();
>         timer_.reset();
> +
> +       controls_.clear();
> +       metadata_.clear();
> +
> +       if (flags & ReuseBuffers) {
> +               auto *request = _o<Request>();
> +
> +               for (const auto &[stream, buffer] : bufferMap_) {
> +                       buffer->_d()->setRequest(request);
> +                       pending_.insert(buffer);
> +               }
> +       } else {
> +               bufferMap_.clear();
> +       }
>  }
>  
>  /*
> @@ -353,14 +369,11 @@ void Request::Private::timeout()
>   * completely opaque to libcamera.
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -       : Extensible(std::make_unique<Private>(camera)),
> -         controls_(controls::controls, camera->_d()->validator()),
> -         metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
> -         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()
> @@ -382,26 +395,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();
> -       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
> @@ -415,9 +412,12 @@ void Request::reuse(ReuseFlag flags)
>   *
>   * \return A reference to the ControlList in this request
>   */
> +ControlList &Request::controls()
> +{
> +       return _d()->controls_;
> +}
>  
>  /**
> - * \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
> @@ -425,6 +425,10 @@ void Request::reuse(ReuseFlag flags)
>   *
>   * \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
> @@ -475,7 +479,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;
> @@ -490,15 +494,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
> @@ -507,20 +502,25 @@ 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 &bufferMap = _d()->bufferMap_;
> +
> +       const auto it = bufferMap.find(stream);
> +       if (it == bufferMap.end())
>                 return nullptr;
>  
>         return it->second;
>  }
>  
>  /**
> - * \fn Request::metadata()
>   * \brief Retrieve the request's metadata
>   * \todo Offer a read-only API towards applications while keeping a read/write
>   * API internally.
>   * \return The metadata associated with the request
>   */
> +ControlList &Request::metadata()
> +{
> +       return _d()->metadata_;
> +}
>  
>  /**
>   * \brief Retrieve the sequence number for the request
> @@ -543,13 +543,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
> @@ -560,6 +562,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
> -- 
> 2.49.0
>
Laurent Pinchart April 1, 2025, 10:57 p.m. UTC | #2 
Hi Barnabás,

Thank you for the patch.

On Mon, Mar 31, 2025 at 04:17:14PM +0200, Barnabás Pőcze wrote:
> Move all members of `Request` into `Request::Private`.

The commit message should explain why.

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/request.h |  10 ++-
>  include/libcamera/request.h          |  18 ++---
>  src/libcamera/request.cpp            | 104 ++++++++++++++-------------
>  3 files changed, 69 insertions(+), 63 deletions(-)
> 
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 73e9bb5cc..009ebbf14 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_; }
> @@ -39,7 +39,7 @@ public:
>  	bool completeBuffer(FrameBuffer *buffer);
>  	void complete();
>  	void cancel();
> -	void reset();
> +	void reset(ReuseFlag flags);
>  
>  	void prepare(std::chrono::milliseconds timeout = 0ms);
>  	Signal<> prepared;
> @@ -57,10 +57,16 @@ private:
>  	bool cancelled_;
>  	uint32_t sequence_ = 0;
>  	bool prepared_ = false;
> +	Status status_ = RequestPending;
> +	const uint64_t cookie_;
>  
>  	std::unordered_set<FrameBuffer *> pending_;
>  	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>  	std::unique_ptr<Timer> timer_;
> +
> +	ControlList controls_;
> +	ControlList metadata_;
> +	BufferMap bufferMap_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 3061e2fb0..609b55885 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -49,16 +49,17 @@ public:
>  
>  	void reuse(ReuseFlag flags = Default);
>  
> -	ControlList &controls() { return controls_; }
> -	ControlList &metadata() { return metadata_; }
> -	const BufferMap &buffers() const { return bufferMap_; }
> +	ControlList &controls();
> +	ControlList &metadata();
> +
> +	const BufferMap &buffers() const;
>  	int addBuffer(const Stream *stream, FrameBuffer *buffer,
>  		      std::unique_ptr<Fence> fence = nullptr);
>  	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,13 +67,6 @@ public:
>  
>  private:
>  	LIBCAMERA_DISABLE_COPY(Request)
> -
> -	ControlList controls_;
> -	ControlList metadata_;
> -	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 fc364441a..1f1be1c7e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -53,9 +53,12 @@ LOG_DEFINE_CATEGORY(Request)
>  /**
>   * \brief Create a Request::Private
>   * \param camera The Camera that creates the request
> + * \param cookie Opaque cookie for application use
>   */
> -Request::Private::Private(Camera *camera)
> -	: camera_(camera), cancelled_(false)
> +Request::Private::Private(Camera *camera, uint64_t cookie)
> +	: camera_(camera), cancelled_(false), cookie_(cookie),
> +	  controls_(controls::controls, camera->_d()->validator()),
> +	  metadata_(controls::controls) /* \todo Add a validator for metadata controls. */
>  {
>  }
>  
> @@ -121,10 +124,10 @@ void Request::Private::complete()
>  {
>  	Request *request = _o<Request>();
>  
> -	ASSERT(request->status() == RequestPending);
> +	ASSERT(status_ == RequestPending);
>  	ASSERT(!hasPendingBuffers());
>  
> -	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
> +	status_ = cancelled_ ? RequestCancelled : RequestComplete;
>  
>  	LOG(Request, Debug) << request->toString();
>  
> @@ -158,26 +161,39 @@ void Request::Private::cancel()
>  {
>  	LIBCAMERA_TRACEPOINT(request_cancel, this);
>  
> -	Request *request = _o<Request>();
> -	ASSERT(request->status() == RequestPending);
> +	ASSERT(status_ == RequestPending);
>  
>  	doCancelRequest();
>  }
>  
>  /**
> - * \brief Reset the request internal data to default values
> - *
> - * After calling this function, all request internal data will have default
> - * values as if the Request::Private instance had just been constructed.
> + * \copydoc Request::reuse()
> + * \sa Request::reuse()
>   */
> -void Request::Private::reset()
> +void Request::Private::reset(Request::ReuseFlag flags)
>  {
>  	sequence_ = 0;
>  	cancelled_ = false;
>  	prepared_ = false;
> +	status_ = RequestPending;
> +
>  	pending_.clear();
>  	notifiers_.clear();
>  	timer_.reset();
> +
> +	controls_.clear();
> +	metadata_.clear();
> +
> +	if (flags & ReuseBuffers) {
> +		auto *request = _o<Request>();

		Request *request = _o<Request>();

> +
> +		for (const auto &[stream, buffer] : bufferMap_) {
> +			buffer->_d()->setRequest(request);
> +			pending_.insert(buffer);
> +		}
> +	} else {
> +		bufferMap_.clear();
> +	}
>  }
>  
>  /*
> @@ -353,14 +369,11 @@ void Request::Private::timeout()
>   * completely opaque to libcamera.
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
> -	: Extensible(std::make_unique<Private>(camera)),
> -	  controls_(controls::controls, camera->_d()->validator()),
> -	  metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
> -	  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()
> @@ -382,26 +395,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();
> -	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
> @@ -415,9 +412,12 @@ void Request::reuse(ReuseFlag flags)
>   *
>   * \return A reference to the ControlList in this request
>   */
> +ControlList &Request::controls()
> +{
> +	return _d()->controls_;
> +}
>  
>  /**
> - * \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
> @@ -425,6 +425,10 @@ void Request::reuse(ReuseFlag flags)
>   *
>   * \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
> @@ -475,7 +479,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;
> @@ -490,15 +494,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.
> - */

Maybe move this to Request::Private instead of deleting it ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -
>  /**
>   * \brief Return the buffer associated with a stream
>   * \param[in] stream The stream the buffer is associated to
> @@ -507,20 +502,25 @@ 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 &bufferMap = _d()->bufferMap_;
> +
> +	const auto it = bufferMap.find(stream);
> +	if (it == bufferMap.end())
>  		return nullptr;
>  
>  	return it->second;
>  }
>  
>  /**
> - * \fn Request::metadata()
>   * \brief Retrieve the request's metadata
>   * \todo Offer a read-only API towards applications while keeping a read/write
>   * API internally.
>   * \return The metadata associated with the request
>   */
> +ControlList &Request::metadata()
> +{
> +	return _d()->metadata_;
> +}
>  
>  /**
>   * \brief Retrieve the sequence number for the request
> @@ -543,13 +543,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
> @@ -560,6 +562,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

Patch
diff mbox series 

diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
index 73e9bb5cc..009ebbf14 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_; }
@@ -39,7 +39,7 @@  public:
 	bool completeBuffer(FrameBuffer *buffer);
 	void complete();
 	void cancel();
-	void reset();
+	void reset(ReuseFlag flags);
 
 	void prepare(std::chrono::milliseconds timeout = 0ms);
 	Signal<> prepared;
@@ -57,10 +57,16 @@  private:
 	bool cancelled_;
 	uint32_t sequence_ = 0;
 	bool prepared_ = false;
+	Status status_ = RequestPending;
+	const uint64_t cookie_;
 
 	std::unordered_set<FrameBuffer *> pending_;
 	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
 	std::unique_ptr<Timer> timer_;
+
+	ControlList controls_;
+	ControlList metadata_;
+	BufferMap bufferMap_;
 };
 
 } /* namespace libcamera */
diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index 3061e2fb0..609b55885 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -49,16 +49,17 @@  public:
 
 	void reuse(ReuseFlag flags = Default);
 
-	ControlList &controls() { return controls_; }
-	ControlList &metadata() { return metadata_; }
-	const BufferMap &buffers() const { return bufferMap_; }
+	ControlList &controls();
+	ControlList &metadata();
+
+	const BufferMap &buffers() const;
 	int addBuffer(const Stream *stream, FrameBuffer *buffer,
 		      std::unique_ptr<Fence> fence = nullptr);
 	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,13 +67,6 @@  public:
 
 private:
 	LIBCAMERA_DISABLE_COPY(Request)
-
-	ControlList controls_;
-	ControlList metadata_;
-	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 fc364441a..1f1be1c7e 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -53,9 +53,12 @@  LOG_DEFINE_CATEGORY(Request)
 /**
  * \brief Create a Request::Private
  * \param camera The Camera that creates the request
+ * \param cookie Opaque cookie for application use
  */
-Request::Private::Private(Camera *camera)
-	: camera_(camera), cancelled_(false)
+Request::Private::Private(Camera *camera, uint64_t cookie)
+	: camera_(camera), cancelled_(false), cookie_(cookie),
+	  controls_(controls::controls, camera->_d()->validator()),
+	  metadata_(controls::controls) /* \todo Add a validator for metadata controls. */
 {
 }
 
@@ -121,10 +124,10 @@  void Request::Private::complete()
 {
 	Request *request = _o<Request>();
 
-	ASSERT(request->status() == RequestPending);
+	ASSERT(status_ == RequestPending);
 	ASSERT(!hasPendingBuffers());
 
-	request->status_ = cancelled_ ? RequestCancelled : RequestComplete;
+	status_ = cancelled_ ? RequestCancelled : RequestComplete;
 
 	LOG(Request, Debug) << request->toString();
 
@@ -158,26 +161,39 @@  void Request::Private::cancel()
 {
 	LIBCAMERA_TRACEPOINT(request_cancel, this);
 
-	Request *request = _o<Request>();
-	ASSERT(request->status() == RequestPending);
+	ASSERT(status_ == RequestPending);
 
 	doCancelRequest();
 }
 
 /**
- * \brief Reset the request internal data to default values
- *
- * After calling this function, all request internal data will have default
- * values as if the Request::Private instance had just been constructed.
+ * \copydoc Request::reuse()
+ * \sa Request::reuse()
  */
-void Request::Private::reset()
+void Request::Private::reset(Request::ReuseFlag flags)
 {
 	sequence_ = 0;
 	cancelled_ = false;
 	prepared_ = false;
+	status_ = RequestPending;
+
 	pending_.clear();
 	notifiers_.clear();
 	timer_.reset();
+
+	controls_.clear();
+	metadata_.clear();
+
+	if (flags & ReuseBuffers) {
+		auto *request = _o<Request>();
+
+		for (const auto &[stream, buffer] : bufferMap_) {
+			buffer->_d()->setRequest(request);
+			pending_.insert(buffer);
+		}
+	} else {
+		bufferMap_.clear();
+	}
 }
 
 /*
@@ -353,14 +369,11 @@  void Request::Private::timeout()
  * completely opaque to libcamera.
  */
 Request::Request(Camera *camera, uint64_t cookie)
-	: Extensible(std::make_unique<Private>(camera)),
-	  controls_(controls::controls, camera->_d()->validator()),
-	  metadata_(controls::controls), /* \todo Add a validator for metadata controls. */
-	  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()
@@ -382,26 +395,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();
-	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
@@ -415,9 +412,12 @@  void Request::reuse(ReuseFlag flags)
  *
  * \return A reference to the ControlList in this request
  */
+ControlList &Request::controls()
+{
+	return _d()->controls_;
+}
 
 /**
- * \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
@@ -425,6 +425,10 @@  void Request::reuse(ReuseFlag flags)
  *
  * \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
@@ -475,7 +479,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;
@@ -490,15 +494,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
@@ -507,20 +502,25 @@  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 &bufferMap = _d()->bufferMap_;
+
+	const auto it = bufferMap.find(stream);
+	if (it == bufferMap.end())
 		return nullptr;
 
 	return it->second;
 }
 
 /**
- * \fn Request::metadata()
  * \brief Retrieve the request's metadata
  * \todo Offer a read-only API towards applications while keeping a read/write
  * API internally.
  * \return The metadata associated with the request
  */
+ControlList &Request::metadata()
+{
+	return _d()->metadata_;
+}
 
 /**
  * \brief Retrieve the sequence number for the request
@@ -543,13 +543,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
@@ -560,6 +562,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