[libcamera-devel,v2] libcamera, android, cam, gstreamer, qcam, v4l2: Reuse Request

9854 diff mbox series

Allow reuse of the Request object by implementing a reset() function.
This means that the applications now have the responsibility of freeing
the Request objects, so make all libcamera users (cam, qcam,
v4l2-compat, gstreamer, android) do so.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- clear controls_ and metadata_ and validator_ in Request::reset()
- use unique_ptr on application side, prior to queueRequest, and use
  regular pointer for completion handler
- make qcam's reuse request nicer
- update Camera::queueRequest() and Camera::createRequest() documentation
- add documentation for Request::reset()
- make v4l2-compat reuse request
- make gstreamer and android use the new createRequest API, though they
  do not actually reuse the requests
  - I haven't yet tested android
---
 include/libcamera/camera.h        |  2 +-
 include/libcamera/request.h       |  2 ++
 src/android/camera_device.cpp     |  8 +++++-
 src/cam/capture.cpp               | 19 ++++++------
 src/cam/capture.h                 |  3 ++
 src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
 src/libcamera/camera.cpp          | 15 ++++------
 src/libcamera/request.cpp         | 24 ++++++++++++++++
 src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
 src/qcam/main_window.h            | 26 +++++------------
 src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
 src/v4l2/v4l2_camera.h            |  4 ++-
 test/camera/buffer_import.cpp     | 15 +++++-----
 test/camera/capture.cpp           | 15 +++++-----
 test/camera/statemachine.cpp      |  9 ++----
 15 files changed, 141 insertions(+), 93 deletions(-)

Hi Paul,

Thanks for your work.

On 2020-09-29 18:52:00 +0900, Paul Elder wrote:
> Allow reuse of the Request object by implementing a reset() function.
> This means that the applications now have the responsibility of freeing
> the Request objects, so make all libcamera users (cam, qcam,
> v4l2-compat, gstreamer, android) do so.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - clear controls_ and metadata_ and validator_ in Request::reset()
> - use unique_ptr on application side, prior to queueRequest, and use
>   regular pointer for completion handler
> - make qcam's reuse request nicer
> - update Camera::queueRequest() and Camera::createRequest() documentation
> - add documentation for Request::reset()
> - make v4l2-compat reuse request
> - make gstreamer and android use the new createRequest API, though they
>   do not actually reuse the requests
>   - I haven't yet tested android
> ---
>  include/libcamera/camera.h        |  2 +-
>  include/libcamera/request.h       |  2 ++
>  src/android/camera_device.cpp     |  8 +++++-
>  src/cam/capture.cpp               | 19 ++++++------
>  src/cam/capture.h                 |  3 ++
>  src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
>  src/libcamera/camera.cpp          | 15 ++++------
>  src/libcamera/request.cpp         | 24 ++++++++++++++++
>  src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
>  src/qcam/main_window.h            | 26 +++++------------
>  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
>  src/v4l2/v4l2_camera.h            |  4 ++-
>  test/camera/buffer_import.cpp     | 15 +++++-----
>  test/camera/capture.cpp           | 15 +++++-----
>  test/camera/statemachine.cpp      |  9 ++----
>  15 files changed, 141 insertions(+), 93 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 272c12c3..e4036b63 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -93,7 +93,7 @@ public:
>  	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
>  	int configure(CameraConfiguration *config);
>  
> -	Request *createRequest(uint64_t cookie = 0);
> +	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);

I like this.

>  	int queueRequest(Request *request);
>  
>  	int start();
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 5976ac50..9f615a24 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -38,6 +38,8 @@ public:
>  	Request &operator=(const Request &) = delete;
>  	~Request();
>  
> +	void reset();
> +
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
>  	const BufferMap &buffers() const { return bufferMap_; }
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 70d77a17..7f288617 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		new Camera3RequestDescriptor(camera3Request->frame_number,
>  					     camera3Request->num_output_buffers);
>  
> -	Request *request =
> +	std::unique_ptr<Request> r =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> +	Request *request = r.release();
> +	if (!request) {
> +		LOG(HAL, Error) << "Failed to create request";
> +		return -ENOMEM;
> +	}

I don't like this ;-)

Would it not make sens to leverage the unique_ptr?

>  
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>  		CameraStream *cameraStream =
> @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  
>  	delete descriptor;
> +	delete request;
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> index 5510c009..5b9238e2 100644
> --- a/src/cam/capture.cpp
> +++ b/src/cam/capture.cpp
> @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
>  		writer_ = nullptr;
>  	}
>  
> +	requests_.clear();
> +
>  	delete allocator;
>  
>  	return ret;
> @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  	 * example pushing a button. For now run all streams all the time.
>  	 */
>  
> -	std::vector<Request *> requests;
>  	for (unsigned int i = 0; i < nbuffers; i++) {
> -		Request *request = camera_->createRequest();
> +		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request) {
>  			std::cerr << "Can't create request" << std::endl;
>  			return -ENOMEM;
> @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  				writer_->mapBuffer(buffer.get());
>  		}
>  
> -		requests.push_back(request);
> +		requests_.push_back(std::move(request));
>  	}
>  
>  	ret = camera_->start();
> @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
>  		return ret;
>  	}
>  
> -	for (Request *request : requests) {
> -		ret = camera_->queueRequest(request);
> +	for (std::unique_ptr<Request> &request : requests_) {
> +		ret = camera_->queueRequest(request.get());

Would it not make sens to take this all the way and have 
Camera::queueRequest() take a unique pointer that is returned in 
requestComplete(). Then the application could store the unique ptr and 
reuse it or let it expire and have the request deleted?

>  		if (ret < 0) {
>  			std::cerr << "Can't queue request" << std::endl;
>  			camera_->stop();
> @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
>  	if (request->status() == Request::RequestCancelled)
>  		return;
>  
> -	const Request::BufferMap &buffers = request->buffers();
> +	const Request::BufferMap buffers = request->buffers();
>  
>  	/*
>  	 * Compute the frame rate. The timestamp is arbitrarily retrieved from
> @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)
>  	 * Create a new request and populate it with one buffer for each
>  	 * stream.
>  	 */
> -	request = camera_->createRequest();
> -	if (!request) {
> -		std::cerr << "Can't create request" << std::endl;
> -		return;
> -	}
> +	request->reset();
>  
>  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
>  		const Stream *stream = it->first;
> diff --git a/src/cam/capture.h b/src/cam/capture.h
> index 0aebdac9..45e5e8a9 100644
> --- a/src/cam/capture.h
> +++ b/src/cam/capture.h
> @@ -9,6 +9,7 @@
>  
>  #include <memory>
>  #include <stdint.h>
> +#include <vector>
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
> @@ -43,6 +44,8 @@ private:
>  	EventLoop *loop_;
>  	unsigned int captureCount_;
>  	unsigned int captureLimit_;
> +
> +	std::vector<std::unique_ptr<libcamera::Request>> requests_;
>  };
>  
>  #endif /* __CAM_CAPTURE_H__ */
> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> index 1bfc2e2f..ca223df6 100644
> --- a/src/gstreamer/gstlibcamerasrc.cpp
> +++ b/src/gstreamer/gstlibcamerasrc.cpp
> @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
>  		if (item.second)
>  			gst_buffer_unref(item.second);
>  	}
> +
> +	delete request_;
>  }
>  
>  void RequestWrap::attachBuffer(GstBuffer *buffer)
> @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
>  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
>  	GstLibcameraSrcState *state = self->state;
>  
> -	Request *request = state->cam_->createRequest();
> +	std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> +	Request *request = request_unique.release();
>  	auto wrap = std::make_unique<RequestWrap>(request);
>  	for (GstPad *srcpad : state->srcpads_) {
>  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
>  			 * queueing this one due to lack of buffers.
>  			 */
>  			delete request;
> -			request = nullptr;
>  			break;
>  		}
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index ae16a64a..8c2bbc76 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
>   * handler, and is completely opaque to libcamera.
>   *
>   * The ownership of the returned request is passed to the caller, which is
> - * responsible for either queueing the request or deleting it.
> + * responsible for either queueing the request or deleting it. The caller
> + * continues to own the request after queueing it, so deleting the request
> + * is the responsibility of the application after the request completes.
>   *
>   * \context This function is \threadsafe. It may only be called when the camera
>   * is in the Configured or Running state as defined in \ref camera_operation.
>   *
>   * \return A pointer to the newly created request, or nullptr on error
>   */
> -Request *Camera::createRequest(uint64_t cookie)
> +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
>  {
>  	int ret = p_->isAccessAllowed(Private::CameraConfigured,
>  				      Private::CameraRunning);
>  	if (ret < 0)
>  		return nullptr;
>  
> -	return new Request(this, cookie);
> +	return std::make_unique<Request>(this, cookie);
>  }
>  
>  /**
> @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
>   * Once the request has been queued, the camera will notify its completion
>   * through the \ref requestCompleted signal.
>   *
> - * Ownership of the request is transferred to the camera. It will be deleted
> - * automatically after it completes.
> - *
>   * \context This function is \threadsafe. It may only be called when the camera
>   * is in the Running state as defined in \ref camera_operation.
>   *
> @@ -974,13 +973,11 @@ int Camera::stop()
>   * \param[in] request The request that has completed
>   *
>   * This function is called by the pipeline handler to notify the camera that
> - * the request has completed. It emits the requestCompleted signal and deletes
> - * the request.
> + * the request has completed. It emits the requestCompleted signal.
>   */
>  void Camera::requestComplete(Request *request)
>  {
>  	requestCompleted.emit(request);
> -	delete request;
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 60b30692..dd3040fe 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -85,6 +85,30 @@ Request::~Request()
>  	delete validator_;
>  }
>  
> +/*
> + * \brief Reset the request
> + *
> + * Reset the request, to allow it to be reused and requeued without
> + * destruction. All contents of the request will be reset, so any references
> + * previously returned by the request will become invalid.
> + */

Would it make sens to split this function in two? One that resets the 
status_, cancelled_, metadata_ and any output controls and one that 
clears the buffers.

The reset of output parameters could be called by Camera::queueRequest() 
and the clearBuffers() would be called optionally by applications.

That way applications would be able to both reuse a request by either by 
keeping all buffer associations or clearing them and repopulating them 
with new buffers. Also by clearing output parameters in the framework 
applications will be unable to get it wrong.

> +void Request::reset()
> +{
> +	bufferMap_.clear();
> +	pending_.clear();
> +
> +	status_ = RequestPending;
> +	cancelled_ = false;
> +
> +	delete metadata_;
> +	delete controls_;
> +	delete validator_;
> +
> +	validator_ = new CameraControlValidator(camera_);
> +	controls_ = new ControlList(controls::controls, validator_);
> +	metadata_ = new ControlList(controls::controls);
> +}
> +
>  /**
>   * \fn Request::controls()
>   * \brief Retrieve the request's ControlList
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index 985743f3..c2f8b175 100644
> --- a/src/qcam/main_window.cpp
> +++ b/src/qcam/main_window.cpp
> @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
>  int MainWindow::startCapture()
>  {
>  	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> -	std::vector<Request *> requests;
>  	int ret;
>  
>  	/* Verify roles are supported. */
> @@ -486,7 +485,7 @@ int MainWindow::startCapture()
>  	while (!freeBuffers_[vfStream_].isEmpty()) {
>  		FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
>  
> -		Request *request = camera_->createRequest();
> +		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request) {
>  			qWarning() << "Can't create request";
>  			ret = -ENOMEM;
> @@ -499,7 +498,7 @@ int MainWindow::startCapture()
>  			goto error;
>  		}
>  
> -		requests.push_back(request);
> +		requests_.push_back(std::move(request));
>  	}
>  
>  	/* Start the title timer and the camera. */
> @@ -518,8 +517,8 @@ int MainWindow::startCapture()
>  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
>  
>  	/* Queue all requests. */
> -	for (Request *request : requests) {
> -		ret = camera_->queueRequest(request);
> +	for (std::unique_ptr<Request> &request : requests_) {
> +		ret = camera_->queueRequest(request.get());
>  		if (ret < 0) {
>  			qWarning() << "Can't queue request";
>  			goto error_disconnect;
> @@ -535,8 +534,7 @@ error_disconnect:
>  	camera_->stop();
>  
>  error:
> -	for (Request *request : requests)
> -		delete request;
> +	requests_.clear();
>  
>  	for (auto &iter : mappedBuffers_) {
>  		const MappedBuffer &buffer = iter.second;
> @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
>  	}
>  	mappedBuffers_.clear();
>  
> +	requests_.clear();
> +
>  	delete allocator_;
>  
>  	isCapturing_ = false;
> @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
>  	 */
>  	{
>  		QMutexLocker locker(&mutex_);
> -		doneQueue_.enqueue({ request->buffers(), request->metadata() });
> +		doneQueue_.enqueue(request);
>  	}
>  
>  	QCoreApplication::postEvent(this, new CaptureEvent);
> @@ -714,8 +714,7 @@ void MainWindow::processCapture()
>  	 * if stopCapture() has been called while a CaptureEvent was posted but
>  	 * not processed yet. Return immediately in that case.
>  	 */
> -	CaptureRequest request;
> -
> +	Request *request;
>  	{
>  		QMutexLocker locker(&mutex_);
>  		if (doneQueue_.isEmpty())
> @@ -725,11 +724,19 @@ void MainWindow::processCapture()
>  	}
>  
>  	/* Process buffers. */
> -	if (request.buffers_.count(vfStream_))
> -		processViewfinder(request.buffers_[vfStream_]);
> +	if (request->buffers().count(vfStream_)) {
> +		{
> +			QMutexLocker locker(&mutex_);
> +			freeQueue_.enqueue(request);
> +		}
> +		Request::BufferMap buffers = request->buffers();
> +		processViewfinder(buffers[vfStream_]);
> +	}
>  
> -	if (request.buffers_.count(rawStream_))
> -		processRaw(request.buffers_[rawStream_], request.metadata_);
> +	if (request->buffers().count(rawStream_)) {
> +		Request::BufferMap buffers = request->buffers();
> +		processRaw(buffers[rawStream_], request->metadata());
> +	}
>  }
>  
>  void MainWindow::processViewfinder(FrameBuffer *buffer)
> @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
>  
>  void MainWindow::queueRequest(FrameBuffer *buffer)
>  {
> -	Request *request = camera_->createRequest();
> -	if (!request) {
> -		qWarning() << "Can't create request";
> -		return;
> +	Request *request;
> +	{
> +		QMutexLocker locker(&mutex_);
> +		if (freeQueue_.isEmpty())
> +			return;
> +
> +		request = freeQueue_.dequeue();
>  	}
>  
> +	request->reset();
> +
>  	request->addBuffer(vfStream_, buffer);
>  
>  	if (captureRaw_) {
> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> index 5c61a4df..64bcfebc 100644
> --- a/src/qcam/main_window.h
> +++ b/src/qcam/main_window.h
> @@ -8,6 +8,7 @@
>  #define __QCAM_MAIN_WINDOW_H__
>  
>  #include <memory>
> +#include <vector>
>  
>  #include <QElapsedTimer>
>  #include <QIcon>
> @@ -22,6 +23,7 @@
>  #include <libcamera/camera_manager.h>
>  #include <libcamera/controls.h>
>  #include <libcamera/framebuffer_allocator.h>
> +#include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
>  #include "../cam/stream_options.h"
> @@ -41,23 +43,6 @@ enum {
>  	OptStream = 's',
>  };
>  
> -class CaptureRequest
> -{
> -public:
> -	CaptureRequest()
> -	{
> -	}
> -
> -	CaptureRequest(const Request::BufferMap &buffers,
> -		       const ControlList &metadata)
> -		: buffers_(buffers), metadata_(metadata)
> -	{
> -	}
> -
> -	Request::BufferMap buffers_;
> -	ControlList metadata_;
> -};
> -
>  class MainWindow : public QMainWindow
>  {
>  	Q_OBJECT
> @@ -128,13 +113,16 @@ private:
>  	Stream *vfStream_;
>  	Stream *rawStream_;
>  	std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> -	QQueue<CaptureRequest> doneQueue_;
> -	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> +	QQueue<Request *> doneQueue_;
> +	QQueue<Request *> freeQueue_;
> +	QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
>  
>  	uint64_t lastBufferTime_;
>  	QElapsedTimer frameRateInterval_;
>  	uint32_t previousFrames_;
>  	uint32_t framesCaptured_;
> +
> +	std::vector<std::unique_ptr<Request>> requests_;
>  };
>  
>  #endif /* __QCAM_MAIN_WINDOW__ */
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index 3565f369..0d59502a 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
>  
>  void V4L2Camera::close()
>  {
> +	requestPool_.clear();
> +
>  	delete bufferAllocator_;
>  	bufferAllocator_ = nullptr;
>  
> @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
>  	return 0;
>  }
>  
> -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> +int V4L2Camera::allocBuffers(unsigned int count)
>  {
>  	Stream *stream = config_->at(0).stream();
>  
> -	return bufferAllocator_->allocate(stream);
> +	int ret = bufferAllocator_->allocate(stream);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (unsigned int i = 0; i < count; i++) {
> +		std::unique_ptr<Request> request = camera_->createRequest(i);
> +		if (!request) {
> +			requestPool_.clear();
> +			return -ENOMEM;
> +		}
> +		requestPool_.push_back(std::move(request));
> +	}
> +
> +	return ret;
>  }
>  
>  void V4L2Camera::freeBuffers()
>  {
>  	pendingRequests_.clear();
> +	requestPool_.clear();
>  
>  	Stream *stream = config_->at(0).stream();
>  	bufferAllocator_->free(stream);
> @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
>  
>  	isRunning_ = true;
>  
> -	for (std::unique_ptr<Request> &req : pendingRequests_) {
> +	for (Request *req : pendingRequests_) {
>  		/* \todo What should we do if this returns -EINVAL? */
> -		ret = camera_->queueRequest(req.release());
> +		ret = camera_->queueRequest(req);
>  		if (ret < 0)
>  			return ret == -EACCES ? -EBUSY : ret;
>  	}
> @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
>  
>  int V4L2Camera::qbuf(unsigned int index)
>  {
> -	std::unique_ptr<Request> request =
> -		std::unique_ptr<Request>(camera_->createRequest(index));
> -	if (!request) {
> -		LOG(V4L2Compat, Error) << "Can't create request";
> -		return -ENOMEM;
> +	if (index >= requestPool_.size()) {
> +		LOG(V4L2Compat, Error) << "Invalid index";
> +		return -EINVAL;
>  	}
> +	Request *request = requestPool_[index].get();
> +	request->reset();
>  
>  	Stream *stream = config_->at(0).stream();
>  	FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
>  	}
>  
>  	if (!isRunning_) {
> -		pendingRequests_.push_back(std::move(request));
> +		pendingRequests_.push_back(request);
>  		return 0;
>  	}
>  
> -	ret = camera_->queueRequest(request.release());
> +	ret = camera_->queueRequest(request);
>  	if (ret < 0) {
>  		LOG(V4L2Compat, Error) << "Can't queue request";
>  		return ret == -EACCES ? -EBUSY : ret;
> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> index 1fc5ebef..a6c35a2e 100644
> --- a/src/v4l2/v4l2_camera.h
> +++ b/src/v4l2/v4l2_camera.h
> @@ -76,7 +76,9 @@ private:
>  	std::mutex bufferLock_;
>  	FrameBufferAllocator *bufferAllocator_;
>  
> -	std::deque<std::unique_ptr<Request>> pendingRequests_;
> +	std::vector<std::unique_ptr<Request>> requestPool_;
> +
> +	std::deque<Request *> pendingRequests_;
>  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
>  
>  	int efd_;
> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> index 64e96264..9cfdb388 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -50,7 +50,7 @@ protected:
>  		if (request->status() != Request::RequestComplete)
>  			return;
>  
> -		const Request::BufferMap &buffers = request->buffers();
> +		const Request::BufferMap buffers = request->buffers();
>  
>  		completeRequestsCount_++;
>  
> @@ -58,7 +58,7 @@ protected:
>  		const Stream *stream = buffers.begin()->first;
>  		FrameBuffer *buffer = buffers.begin()->second;
>  
> -		request = camera_->createRequest();
> +		request->reset();
>  		request->addBuffer(stream, buffer);
>  		camera_->queueRequest(request);
>  	}
> @@ -98,9 +98,8 @@ protected:
>  		if (ret != TestPass)
>  			return ret;
>  
> -		std::vector<Request *> requests;
>  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> -			Request *request = camera_->createRequest();
> +			std::unique_ptr<Request> request = camera_->createRequest();
>  			if (!request) {
>  				std::cout << "Failed to create request" << std::endl;
>  				return TestFail;
> @@ -111,7 +110,7 @@ protected:
>  				return TestFail;
>  			}
>  
> -			requests.push_back(request);
> +			requests_.push_back(std::move(request));
>  		}
>  
>  		completeRequestsCount_ = 0;
> @@ -125,8 +124,8 @@ protected:
>  			return TestFail;
>  		}
>  
> -		for (Request *request : requests) {
> -			if (camera_->queueRequest(request)) {
> +		for (std::unique_ptr<Request> &request : requests_) {
> +			if (camera_->queueRequest(request.get())) {
>  				std::cout << "Failed to queue request" << std::endl;
>  				return TestFail;
>  			}
> @@ -160,6 +159,8 @@ protected:
>  	}
>  
>  private:
> +	std::vector<std::unique_ptr<Request>> requests_;
> +
>  	unsigned int completeBuffersCount_;
>  	unsigned int completeRequestsCount_;
>  	std::unique_ptr<CameraConfiguration> config_;
> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> index 51bbd258..5c86c490 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -44,7 +44,7 @@ protected:
>  		if (request->status() != Request::RequestComplete)
>  			return;
>  
> -		const Request::BufferMap &buffers = request->buffers();
> +		const Request::BufferMap buffers = request->buffers();
>  
>  		completeRequestsCount_++;
>  
> @@ -52,7 +52,7 @@ protected:
>  		const Stream *stream = buffers.begin()->first;
>  		FrameBuffer *buffer = buffers.begin()->second;
>  
> -		request = camera_->createRequest();
> +		request->reset();
>  		request->addBuffer(stream, buffer);
>  		camera_->queueRequest(request);
>  	}
> @@ -98,9 +98,8 @@ protected:
>  		if (ret < 0)
>  			return TestFail;
>  
> -		std::vector<Request *> requests;
>  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> -			Request *request = camera_->createRequest();
> +			std::unique_ptr<Request> request = camera_->createRequest();
>  			if (!request) {
>  				cout << "Failed to create request" << endl;
>  				return TestFail;
> @@ -111,7 +110,7 @@ protected:
>  				return TestFail;
>  			}
>  
> -			requests.push_back(request);
> +			requests_.push_back(std::move(request));
>  		}
>  
>  		completeRequestsCount_ = 0;
> @@ -125,8 +124,8 @@ protected:
>  			return TestFail;
>  		}
>  
> -		for (Request *request : requests) {
> -			if (camera_->queueRequest(request)) {
> +		for (std::unique_ptr<Request> &request : requests_) {
> +			if (camera_->queueRequest(request.get())) {
>  				cout << "Failed to queue request" << endl;
>  				return TestFail;
>  			}
> @@ -161,6 +160,8 @@ protected:
>  		return TestPass;
>  	}
>  
> +	std::vector<std::unique_ptr<Request>> requests_;
> +
>  	std::unique_ptr<CameraConfiguration> config_;
>  	FrameBufferAllocator *allocator_;
>  };
> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> index 28faeb91..e63ab298 100644
> --- a/test/camera/statemachine.cpp
> +++ b/test/camera/statemachine.cpp
> @@ -101,13 +101,10 @@ protected:
>  			return TestFail;
>  
>  		/* Test operations which should pass. */
> -		Request *request2 = camera_->createRequest();
> +		std::unique_ptr<Request> request2 = camera_->createRequest();
>  		if (!request2)
>  			return TestFail;
>  
> -		/* Never handed to hardware so need to manually delete it. */
> -		delete request2;
> -
>  		/* Test valid state transitions, end in Running state. */
>  		if (camera_->release())
>  			return TestFail;
> @@ -146,7 +143,7 @@ protected:
>  			return TestFail;
>  
>  		/* Test operations which should pass. */
> -		Request *request = camera_->createRequest();
> +		std::unique_ptr<Request> request = camera_->createRequest();
>  		if (!request)
>  			return TestFail;
>  
> @@ -154,7 +151,7 @@ protected:
>  		if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
>  			return TestFail;
>  
> -		if (camera_->queueRequest(request))
> +		if (camera_->queueRequest(request.get()))
>  			return TestFail;
>  
>  		/* Test valid state transitions, end in Available state. */
> -- 
> 2.27.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the review.

On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your work.
> 
> On 2020-09-29 18:52:00 +0900, Paul Elder wrote:
> > Allow reuse of the Request object by implementing a reset() function.
> > This means that the applications now have the responsibility of freeing
> > the Request objects, so make all libcamera users (cam, qcam,
> > v4l2-compat, gstreamer, android) do so.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > Changes in v2:
> > - clear controls_ and metadata_ and validator_ in Request::reset()
> > - use unique_ptr on application side, prior to queueRequest, and use
> >   regular pointer for completion handler
> > - make qcam's reuse request nicer
> > - update Camera::queueRequest() and Camera::createRequest() documentation
> > - add documentation for Request::reset()
> > - make v4l2-compat reuse request
> > - make gstreamer and android use the new createRequest API, though they
> >   do not actually reuse the requests
> >   - I haven't yet tested android
> > ---
> >  include/libcamera/camera.h        |  2 +-
> >  include/libcamera/request.h       |  2 ++
> >  src/android/camera_device.cpp     |  8 +++++-
> >  src/cam/capture.cpp               | 19 ++++++------
> >  src/cam/capture.h                 |  3 ++
> >  src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
> >  src/libcamera/camera.cpp          | 15 ++++------
> >  src/libcamera/request.cpp         | 24 ++++++++++++++++
> >  src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
> >  src/qcam/main_window.h            | 26 +++++------------
> >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
> >  src/v4l2/v4l2_camera.h            |  4 ++-
> >  test/camera/buffer_import.cpp     | 15 +++++-----
> >  test/camera/capture.cpp           | 15 +++++-----
> >  test/camera/statemachine.cpp      |  9 ++----
> >  15 files changed, 141 insertions(+), 93 deletions(-)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 272c12c3..e4036b63 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -93,7 +93,7 @@ public:
> >  	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> >  	int configure(CameraConfiguration *config);
> >  
> > -	Request *createRequest(uint64_t cookie = 0);
> > +	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> 
> I like this.

:)

> >  	int queueRequest(Request *request);
> >  
> >  	int start();
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 5976ac50..9f615a24 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -38,6 +38,8 @@ public:
> >  	Request &operator=(const Request &) = delete;
> >  	~Request();
> >  
> > +	void reset();
> > +
> >  	ControlList &controls() { return *controls_; }
> >  	ControlList &metadata() { return *metadata_; }
> >  	const BufferMap &buffers() const { return bufferMap_; }
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 70d77a17..7f288617 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> >  					     camera3Request->num_output_buffers);
> >  
> > -	Request *request =
> > +	std::unique_ptr<Request> r =
> >  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > +	Request *request = r.release();
> > +	if (!request) {
> > +		LOG(HAL, Error) << "Failed to create request";
> > +		return -ENOMEM;
> > +	}
> 
> I don't like this ;-)

:(

> Would it not make sens to leverage the unique_ptr?

From what I could tell, android doesn't requeue the request in the
request completion handler (CameraDevice::requestComplete()), which
means that android uses the request transiently, like how request was
meant to be used before (no reuse). So I kept it that way.

Or do you mean that I should let the unique pointer die on its own by
letting it go out of scope? Wouldn't that free the request its pointing
to, and segfault somewhere between this and the completion handler?

> 
> >  
> >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> >  		CameraStream *cameraStream =
> > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
> >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> >  
> >  	delete descriptor;
> > +	delete request;
> >  }
> >  
> >  std::string CameraDevice::logPrefix() const
> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > index 5510c009..5b9238e2 100644
> > --- a/src/cam/capture.cpp
> > +++ b/src/cam/capture.cpp
> > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
> >  		writer_ = nullptr;
> >  	}
> >  
> > +	requests_.clear();
> > +
> >  	delete allocator;
> >  
> >  	return ret;
> > @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  	 * example pushing a button. For now run all streams all the time.
> >  	 */
> >  
> > -	std::vector<Request *> requests;
> >  	for (unsigned int i = 0; i < nbuffers; i++) {
> > -		Request *request = camera_->createRequest();
> > +		std::unique_ptr<Request> request = camera_->createRequest();
> >  		if (!request) {
> >  			std::cerr << "Can't create request" << std::endl;
> >  			return -ENOMEM;
> > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  				writer_->mapBuffer(buffer.get());
> >  		}
> >  
> > -		requests.push_back(request);
> > +		requests_.push_back(std::move(request));
> >  	}
> >  
> >  	ret = camera_->start();
> > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> >  		return ret;
> >  	}
> >  
> > -	for (Request *request : requests) {
> > -		ret = camera_->queueRequest(request);
> > +	for (std::unique_ptr<Request> &request : requests_) {
> > +		ret = camera_->queueRequest(request.get());
> 
> Would it not make sens to take this all the way and have 
> Camera::queueRequest() take a unique pointer that is returned in 
> requestComplete(). Then the application could store the unique ptr and 
> reuse it or let it expire and have the request deleted?

The problem with queueRequest() taking a unique pointer is that the
camera then owns the request... but it can't give ownership back to the
application since the completion handler signal can have multiple slots.
We can't give all of them a unique pointer, but if we give them a
regular pointer, then whose responsibility is it for freeing the
request? So we make the application keep ownership of the request. If it
relinquishes ownership then it can free it at the end of the completion
handler (like android and gstreamer does now).

> >  		if (ret < 0) {
> >  			std::cerr << "Can't queue request" << std::endl;
> >  			camera_->stop();
> > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
> >  	if (request->status() == Request::RequestCancelled)
> >  		return;
> >  
> > -	const Request::BufferMap &buffers = request->buffers();
> > +	const Request::BufferMap buffers = request->buffers();
> >  
> >  	/*
> >  	 * Compute the frame rate. The timestamp is arbitrarily retrieved from
> > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)
> >  	 * Create a new request and populate it with one buffer for each
> >  	 * stream.
> >  	 */
> > -	request = camera_->createRequest();
> > -	if (!request) {
> > -		std::cerr << "Can't create request" << std::endl;
> > -		return;
> > -	}
> > +	request->reset();
> >  
> >  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> >  		const Stream *stream = it->first;
> > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > index 0aebdac9..45e5e8a9 100644
> > --- a/src/cam/capture.h
> > +++ b/src/cam/capture.h
> > @@ -9,6 +9,7 @@
> >  
> >  #include <memory>
> >  #include <stdint.h>
> > +#include <vector>
> >  
> >  #include <libcamera/buffer.h>
> >  #include <libcamera/camera.h>
> > @@ -43,6 +44,8 @@ private:
> >  	EventLoop *loop_;
> >  	unsigned int captureCount_;
> >  	unsigned int captureLimit_;
> > +
> > +	std::vector<std::unique_ptr<libcamera::Request>> requests_;
> >  };
> >  
> >  #endif /* __CAM_CAPTURE_H__ */
> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > index 1bfc2e2f..ca223df6 100644
> > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
> >  		if (item.second)
> >  			gst_buffer_unref(item.second);
> >  	}
> > +
> > +	delete request_;
> >  }
> >  
> >  void RequestWrap::attachBuffer(GstBuffer *buffer)
> > @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> >  	GstLibcameraSrcState *state = self->state;
> >  
> > -	Request *request = state->cam_->createRequest();
> > +	std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> > +	Request *request = request_unique.release();
> >  	auto wrap = std::make_unique<RequestWrap>(request);
> >  	for (GstPad *srcpad : state->srcpads_) {
> >  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  			 * queueing this one due to lack of buffers.
> >  			 */
> >  			delete request;
> > -			request = nullptr;
> >  			break;
> >  		}
> >  
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index ae16a64a..8c2bbc76 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
> >   * handler, and is completely opaque to libcamera.
> >   *
> >   * The ownership of the returned request is passed to the caller, which is
> > - * responsible for either queueing the request or deleting it.
> > + * responsible for either queueing the request or deleting it. The caller
> > + * continues to own the request after queueing it, so deleting the request
> > + * is the responsibility of the application after the request completes.
> >   *
> >   * \context This function is \threadsafe. It may only be called when the camera
> >   * is in the Configured or Running state as defined in \ref camera_operation.
> >   *
> >   * \return A pointer to the newly created request, or nullptr on error
> >   */
> > -Request *Camera::createRequest(uint64_t cookie)
> > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> >  {
> >  	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> >  				      Private::CameraRunning);
> >  	if (ret < 0)
> >  		return nullptr;
> >  
> > -	return new Request(this, cookie);
> > +	return std::make_unique<Request>(this, cookie);
> >  }
> >  
> >  /**
> > @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
> >   * Once the request has been queued, the camera will notify its completion
> >   * through the \ref requestCompleted signal.
> >   *
> > - * Ownership of the request is transferred to the camera. It will be deleted
> > - * automatically after it completes.
> > - *
> >   * \context This function is \threadsafe. It may only be called when the camera
> >   * is in the Running state as defined in \ref camera_operation.
> >   *
> > @@ -974,13 +973,11 @@ int Camera::stop()
> >   * \param[in] request The request that has completed
> >   *
> >   * This function is called by the pipeline handler to notify the camera that
> > - * the request has completed. It emits the requestCompleted signal and deletes
> > - * the request.
> > + * the request has completed. It emits the requestCompleted signal.
> >   */
> >  void Camera::requestComplete(Request *request)
> >  {
> >  	requestCompleted.emit(request);
> > -	delete request;
> >  }
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 60b30692..dd3040fe 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -85,6 +85,30 @@ Request::~Request()
> >  	delete validator_;
> >  }
> >  
> > +/*
> > + * \brief Reset the request
> > + *
> > + * Reset the request, to allow it to be reused and requeued without
> > + * destruction. All contents of the request will be reset, so any references
> > + * previously returned by the request will become invalid.
> > + */
> 
> Would it make sens to split this function in two? One that resets the 
> status_, cancelled_, metadata_ and any output controls and one that 
> clears the buffers.
> 
> The reset of output parameters could be called by Camera::queueRequest() 
> and the clearBuffers() would be called optionally by applications.
> 
> That way applications would be able to both reuse a request by either by 
> keeping all buffer associations or clearing them and repopulating them 
> with new buffers. Also by clearing output parameters in the framework 
> applications will be unable to get it wrong.

Oh interesting idea.

Okay, I did a reset() that's called in Camera::queueRequest(), that
resets status_, cancelled_, metadata_, controls_, and validator_. Then a
clearBuffers() that nukes the buffer map. And a reAddBuffers() that
takes the current buffer map, and reattaches the request to the buffers
(otherwise we get a segfault on buffer->request_ since completeBuffer()
nulls buffer->request_ and there's no addBuffer() to reattach it).

And so the apps that:
- still use transient request (android, gstreamer)
  - can just delete it at the end of the completion handler
- the apps the want to reuse the buffers (cam)
  - can just call request->reAddBuffers() at the end of the completion
    handler
- and apps that need to change up their buffers (qcam, v4l2)
  - can do so at the end of the completion handler, with no special
    calls

See v3 for details :)


Thanks,

Paul

> > +void Request::reset()
> > +{
> > +	bufferMap_.clear();
> > +	pending_.clear();
> > +
> > +	status_ = RequestPending;
> > +	cancelled_ = false;
> > +
> > +	delete metadata_;
> > +	delete controls_;
> > +	delete validator_;
> > +
> > +	validator_ = new CameraControlValidator(camera_);
> > +	controls_ = new ControlList(controls::controls, validator_);
> > +	metadata_ = new ControlList(controls::controls);
> > +}
> > +
> >  /**
> >   * \fn Request::controls()
> >   * \brief Retrieve the request's ControlList
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index 985743f3..c2f8b175 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
> >  int MainWindow::startCapture()
> >  {
> >  	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > -	std::vector<Request *> requests;
> >  	int ret;
> >  
> >  	/* Verify roles are supported. */
> > @@ -486,7 +485,7 @@ int MainWindow::startCapture()
> >  	while (!freeBuffers_[vfStream_].isEmpty()) {
> >  		FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
> >  
> > -		Request *request = camera_->createRequest();
> > +		std::unique_ptr<Request> request = camera_->createRequest();
> >  		if (!request) {
> >  			qWarning() << "Can't create request";
> >  			ret = -ENOMEM;
> > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> >  			goto error;
> >  		}
> >  
> > -		requests.push_back(request);
> > +		requests_.push_back(std::move(request));
> >  	}
> >  
> >  	/* Start the title timer and the camera. */
> > @@ -518,8 +517,8 @@ int MainWindow::startCapture()
> >  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> >  
> >  	/* Queue all requests. */
> > -	for (Request *request : requests) {
> > -		ret = camera_->queueRequest(request);
> > +	for (std::unique_ptr<Request> &request : requests_) {
> > +		ret = camera_->queueRequest(request.get());
> >  		if (ret < 0) {
> >  			qWarning() << "Can't queue request";
> >  			goto error_disconnect;
> > @@ -535,8 +534,7 @@ error_disconnect:
> >  	camera_->stop();
> >  
> >  error:
> > -	for (Request *request : requests)
> > -		delete request;
> > +	requests_.clear();
> >  
> >  	for (auto &iter : mappedBuffers_) {
> >  		const MappedBuffer &buffer = iter.second;
> > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
> >  	}
> >  	mappedBuffers_.clear();
> >  
> > +	requests_.clear();
> > +
> >  	delete allocator_;
> >  
> >  	isCapturing_ = false;
> > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
> >  	 */
> >  	{
> >  		QMutexLocker locker(&mutex_);
> > -		doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > +		doneQueue_.enqueue(request);
> >  	}
> >  
> >  	QCoreApplication::postEvent(this, new CaptureEvent);
> > @@ -714,8 +714,7 @@ void MainWindow::processCapture()
> >  	 * if stopCapture() has been called while a CaptureEvent was posted but
> >  	 * not processed yet. Return immediately in that case.
> >  	 */
> > -	CaptureRequest request;
> > -
> > +	Request *request;
> >  	{
> >  		QMutexLocker locker(&mutex_);
> >  		if (doneQueue_.isEmpty())
> > @@ -725,11 +724,19 @@ void MainWindow::processCapture()
> >  	}
> >  
> >  	/* Process buffers. */
> > -	if (request.buffers_.count(vfStream_))
> > -		processViewfinder(request.buffers_[vfStream_]);
> > +	if (request->buffers().count(vfStream_)) {
> > +		{
> > +			QMutexLocker locker(&mutex_);
> > +			freeQueue_.enqueue(request);
> > +		}
> > +		Request::BufferMap buffers = request->buffers();
> > +		processViewfinder(buffers[vfStream_]);
> > +	}
> >  
> > -	if (request.buffers_.count(rawStream_))
> > -		processRaw(request.buffers_[rawStream_], request.metadata_);
> > +	if (request->buffers().count(rawStream_)) {
> > +		Request::BufferMap buffers = request->buffers();
> > +		processRaw(buffers[rawStream_], request->metadata());
> > +	}
> >  }
> >  
> >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> >  
> >  void MainWindow::queueRequest(FrameBuffer *buffer)
> >  {
> > -	Request *request = camera_->createRequest();
> > -	if (!request) {
> > -		qWarning() << "Can't create request";
> > -		return;
> > +	Request *request;
> > +	{
> > +		QMutexLocker locker(&mutex_);
> > +		if (freeQueue_.isEmpty())
> > +			return;
> > +
> > +		request = freeQueue_.dequeue();
> >  	}
> >  
> > +	request->reset();
> > +
> >  	request->addBuffer(vfStream_, buffer);
> >  
> >  	if (captureRaw_) {
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 5c61a4df..64bcfebc 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -8,6 +8,7 @@
> >  #define __QCAM_MAIN_WINDOW_H__
> >  
> >  #include <memory>
> > +#include <vector>
> >  
> >  #include <QElapsedTimer>
> >  #include <QIcon>
> > @@ -22,6 +23,7 @@
> >  #include <libcamera/camera_manager.h>
> >  #include <libcamera/controls.h>
> >  #include <libcamera/framebuffer_allocator.h>
> > +#include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >  
> >  #include "../cam/stream_options.h"
> > @@ -41,23 +43,6 @@ enum {
> >  	OptStream = 's',
> >  };
> >  
> > -class CaptureRequest
> > -{
> > -public:
> > -	CaptureRequest()
> > -	{
> > -	}
> > -
> > -	CaptureRequest(const Request::BufferMap &buffers,
> > -		       const ControlList &metadata)
> > -		: buffers_(buffers), metadata_(metadata)
> > -	{
> > -	}
> > -
> > -	Request::BufferMap buffers_;
> > -	ControlList metadata_;
> > -};
> > -
> >  class MainWindow : public QMainWindow
> >  {
> >  	Q_OBJECT
> > @@ -128,13 +113,16 @@ private:
> >  	Stream *vfStream_;
> >  	Stream *rawStream_;
> >  	std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> > -	QQueue<CaptureRequest> doneQueue_;
> > -	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> > +	QQueue<Request *> doneQueue_;
> > +	QQueue<Request *> freeQueue_;
> > +	QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
> >  
> >  	uint64_t lastBufferTime_;
> >  	QElapsedTimer frameRateInterval_;
> >  	uint32_t previousFrames_;
> >  	uint32_t framesCaptured_;
> > +
> > +	std::vector<std::unique_ptr<Request>> requests_;
> >  };
> >  
> >  #endif /* __QCAM_MAIN_WINDOW__ */
> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > index 3565f369..0d59502a 100644
> > --- a/src/v4l2/v4l2_camera.cpp
> > +++ b/src/v4l2/v4l2_camera.cpp
> > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
> >  
> >  void V4L2Camera::close()
> >  {
> > +	requestPool_.clear();
> > +
> >  	delete bufferAllocator_;
> >  	bufferAllocator_ = nullptr;
> >  
> > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> >  	return 0;
> >  }
> >  
> > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> > +int V4L2Camera::allocBuffers(unsigned int count)
> >  {
> >  	Stream *stream = config_->at(0).stream();
> >  
> > -	return bufferAllocator_->allocate(stream);
> > +	int ret = bufferAllocator_->allocate(stream);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	for (unsigned int i = 0; i < count; i++) {
> > +		std::unique_ptr<Request> request = camera_->createRequest(i);
> > +		if (!request) {
> > +			requestPool_.clear();
> > +			return -ENOMEM;
> > +		}
> > +		requestPool_.push_back(std::move(request));
> > +	}
> > +
> > +	return ret;
> >  }
> >  
> >  void V4L2Camera::freeBuffers()
> >  {
> >  	pendingRequests_.clear();
> > +	requestPool_.clear();
> >  
> >  	Stream *stream = config_->at(0).stream();
> >  	bufferAllocator_->free(stream);
> > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
> >  
> >  	isRunning_ = true;
> >  
> > -	for (std::unique_ptr<Request> &req : pendingRequests_) {
> > +	for (Request *req : pendingRequests_) {
> >  		/* \todo What should we do if this returns -EINVAL? */
> > -		ret = camera_->queueRequest(req.release());
> > +		ret = camera_->queueRequest(req);
> >  		if (ret < 0)
> >  			return ret == -EACCES ? -EBUSY : ret;
> >  	}
> > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
> >  
> >  int V4L2Camera::qbuf(unsigned int index)
> >  {
> > -	std::unique_ptr<Request> request =
> > -		std::unique_ptr<Request>(camera_->createRequest(index));
> > -	if (!request) {
> > -		LOG(V4L2Compat, Error) << "Can't create request";
> > -		return -ENOMEM;
> > +	if (index >= requestPool_.size()) {
> > +		LOG(V4L2Compat, Error) << "Invalid index";
> > +		return -EINVAL;
> >  	}
> > +	Request *request = requestPool_[index].get();
> > +	request->reset();
> >  
> >  	Stream *stream = config_->at(0).stream();
> >  	FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
> >  	}
> >  
> >  	if (!isRunning_) {
> > -		pendingRequests_.push_back(std::move(request));
> > +		pendingRequests_.push_back(request);
> >  		return 0;
> >  	}
> >  
> > -	ret = camera_->queueRequest(request.release());
> > +	ret = camera_->queueRequest(request);
> >  	if (ret < 0) {
> >  		LOG(V4L2Compat, Error) << "Can't queue request";
> >  		return ret == -EACCES ? -EBUSY : ret;
> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > index 1fc5ebef..a6c35a2e 100644
> > --- a/src/v4l2/v4l2_camera.h
> > +++ b/src/v4l2/v4l2_camera.h
> > @@ -76,7 +76,9 @@ private:
> >  	std::mutex bufferLock_;
> >  	FrameBufferAllocator *bufferAllocator_;
> >  
> > -	std::deque<std::unique_ptr<Request>> pendingRequests_;
> > +	std::vector<std::unique_ptr<Request>> requestPool_;
> > +
> > +	std::deque<Request *> pendingRequests_;
> >  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
> >  
> >  	int efd_;
> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > index 64e96264..9cfdb388 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -50,7 +50,7 @@ protected:
> >  		if (request->status() != Request::RequestComplete)
> >  			return;
> >  
> > -		const Request::BufferMap &buffers = request->buffers();
> > +		const Request::BufferMap buffers = request->buffers();
> >  
> >  		completeRequestsCount_++;
> >  
> > @@ -58,7 +58,7 @@ protected:
> >  		const Stream *stream = buffers.begin()->first;
> >  		FrameBuffer *buffer = buffers.begin()->second;
> >  
> > -		request = camera_->createRequest();
> > +		request->reset();
> >  		request->addBuffer(stream, buffer);
> >  		camera_->queueRequest(request);
> >  	}
> > @@ -98,9 +98,8 @@ protected:
> >  		if (ret != TestPass)
> >  			return ret;
> >  
> > -		std::vector<Request *> requests;
> >  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> > -			Request *request = camera_->createRequest();
> > +			std::unique_ptr<Request> request = camera_->createRequest();
> >  			if (!request) {
> >  				std::cout << "Failed to create request" << std::endl;
> >  				return TestFail;
> > @@ -111,7 +110,7 @@ protected:
> >  				return TestFail;
> >  			}
> >  
> > -			requests.push_back(request);
> > +			requests_.push_back(std::move(request));
> >  		}
> >  
> >  		completeRequestsCount_ = 0;
> > @@ -125,8 +124,8 @@ protected:
> >  			return TestFail;
> >  		}
> >  
> > -		for (Request *request : requests) {
> > -			if (camera_->queueRequest(request)) {
> > +		for (std::unique_ptr<Request> &request : requests_) {
> > +			if (camera_->queueRequest(request.get())) {
> >  				std::cout << "Failed to queue request" << std::endl;
> >  				return TestFail;
> >  			}
> > @@ -160,6 +159,8 @@ protected:
> >  	}
> >  
> >  private:
> > +	std::vector<std::unique_ptr<Request>> requests_;
> > +
> >  	unsigned int completeBuffersCount_;
> >  	unsigned int completeRequestsCount_;
> >  	std::unique_ptr<CameraConfiguration> config_;
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index 51bbd258..5c86c490 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -44,7 +44,7 @@ protected:
> >  		if (request->status() != Request::RequestComplete)
> >  			return;
> >  
> > -		const Request::BufferMap &buffers = request->buffers();
> > +		const Request::BufferMap buffers = request->buffers();
> >  
> >  		completeRequestsCount_++;
> >  
> > @@ -52,7 +52,7 @@ protected:
> >  		const Stream *stream = buffers.begin()->first;
> >  		FrameBuffer *buffer = buffers.begin()->second;
> >  
> > -		request = camera_->createRequest();
> > +		request->reset();
> >  		request->addBuffer(stream, buffer);
> >  		camera_->queueRequest(request);
> >  	}
> > @@ -98,9 +98,8 @@ protected:
> >  		if (ret < 0)
> >  			return TestFail;
> >  
> > -		std::vector<Request *> requests;
> >  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > -			Request *request = camera_->createRequest();
> > +			std::unique_ptr<Request> request = camera_->createRequest();
> >  			if (!request) {
> >  				cout << "Failed to create request" << endl;
> >  				return TestFail;
> > @@ -111,7 +110,7 @@ protected:
> >  				return TestFail;
> >  			}
> >  
> > -			requests.push_back(request);
> > +			requests_.push_back(std::move(request));
> >  		}
> >  
> >  		completeRequestsCount_ = 0;
> > @@ -125,8 +124,8 @@ protected:
> >  			return TestFail;
> >  		}
> >  
> > -		for (Request *request : requests) {
> > -			if (camera_->queueRequest(request)) {
> > +		for (std::unique_ptr<Request> &request : requests_) {
> > +			if (camera_->queueRequest(request.get())) {
> >  				cout << "Failed to queue request" << endl;
> >  				return TestFail;
> >  			}
> > @@ -161,6 +160,8 @@ protected:
> >  		return TestPass;
> >  	}
> >  
> > +	std::vector<std::unique_ptr<Request>> requests_;
> > +
> >  	std::unique_ptr<CameraConfiguration> config_;
> >  	FrameBufferAllocator *allocator_;
> >  };
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index 28faeb91..e63ab298 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -101,13 +101,10 @@ protected:
> >  			return TestFail;
> >  
> >  		/* Test operations which should pass. */
> > -		Request *request2 = camera_->createRequest();
> > +		std::unique_ptr<Request> request2 = camera_->createRequest();
> >  		if (!request2)
> >  			return TestFail;
> >  
> > -		/* Never handed to hardware so need to manually delete it. */
> > -		delete request2;
> > -
> >  		/* Test valid state transitions, end in Running state. */
> >  		if (camera_->release())
> >  			return TestFail;
> > @@ -146,7 +143,7 @@ protected:
> >  			return TestFail;
> >  
> >  		/* Test operations which should pass. */
> > -		Request *request = camera_->createRequest();
> > +		std::unique_ptr<Request> request = camera_->createRequest();
> >  		if (!request)
> >  			return TestFail;
> >  
> > @@ -154,7 +151,7 @@ protected:
> >  		if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
> >  			return TestFail;
> >  
> > -		if (camera_->queueRequest(request))
> > +		if (camera_->queueRequest(request.get()))
> >  			return TestFail;
> >  
> >  		/* Test valid state transitions, end in Available state. */
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

On Wed, Sep 30, 2020 at 06:55:07PM +0900, paul.elder@ideasonboard.com wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your work.
> > 
> > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:
> > > Allow reuse of the Request object by implementing a reset() function.
> > > This means that the applications now have the responsibility of freeing
> > > the Request objects, so make all libcamera users (cam, qcam,
> > > v4l2-compat, gstreamer, android) do so.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v2:
> > > - clear controls_ and metadata_ and validator_ in Request::reset()
> > > - use unique_ptr on application side, prior to queueRequest, and use
> > >   regular pointer for completion handler
> > > - make qcam's reuse request nicer
> > > - update Camera::queueRequest() and Camera::createRequest() documentation
> > > - add documentation for Request::reset()
> > > - make v4l2-compat reuse request
> > > - make gstreamer and android use the new createRequest API, though they
> > >   do not actually reuse the requests
> > >   - I haven't yet tested android
> > > ---
> > >  include/libcamera/camera.h        |  2 +-
> > >  include/libcamera/request.h       |  2 ++
> > >  src/android/camera_device.cpp     |  8 +++++-
> > >  src/cam/capture.cpp               | 19 ++++++------
> > >  src/cam/capture.h                 |  3 ++
> > >  src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
> > >  src/libcamera/camera.cpp          | 15 ++++------
> > >  src/libcamera/request.cpp         | 24 ++++++++++++++++
> > >  src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
> > >  src/qcam/main_window.h            | 26 +++++------------
> > >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
> > >  src/v4l2/v4l2_camera.h            |  4 ++-
> > >  test/camera/buffer_import.cpp     | 15 +++++-----
> > >  test/camera/capture.cpp           | 15 +++++-----
> > >  test/camera/statemachine.cpp      |  9 ++----
> > >  15 files changed, 141 insertions(+), 93 deletions(-)
> > > 
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 272c12c3..e4036b63 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -93,7 +93,7 @@ public:
> > >  	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > >  	int configure(CameraConfiguration *config);
> > >  
> > > -	Request *createRequest(uint64_t cookie = 0);
> > > +	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > 
> > I like this.
> 
> :)
> 
> > >  	int queueRequest(Request *request);
> > >  
> > >  	int start();
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index 5976ac50..9f615a24 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -38,6 +38,8 @@ public:
> > >  	Request &operator=(const Request &) = delete;
> > >  	~Request();
> > >  
> > > +	void reset();
> > > +
> > >  	ControlList &controls() { return *controls_; }
> > >  	ControlList &metadata() { return *metadata_; }
> > >  	const BufferMap &buffers() const { return bufferMap_; }
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 70d77a17..7f288617 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> > >  					     camera3Request->num_output_buffers);
> > >  
> > > -	Request *request =
> > > +	std::unique_ptr<Request> r =
> > >  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > > +	Request *request = r.release();
> > > +	if (!request) {
> > > +		LOG(HAL, Error) << "Failed to create request";
> > > +		return -ENOMEM;
> > > +	}
> > 
> > I don't like this ;-)
> 
> :(
> 
> > Would it not make sens to leverage the unique_ptr?
> 
> From what I could tell, android doesn't requeue the request in the
> request completion handler (CameraDevice::requestComplete()), which
> means that android uses the request transiently, like how request was
> meant to be used before (no reuse). So I kept it that way.
> 
> Or do you mean that I should let the unique pointer die on its own by
> letting it go out of scope? Wouldn't that free the request its pointing
> to, and segfault somewhere between this and the completion handler?
> 
> > 
> > >  
> > >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > >  		CameraStream *cameraStream =
> > > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
> > >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > >  
> > >  	delete descriptor;
> > > +	delete request;
> > >  }
> > >  
> > >  std::string CameraDevice::logPrefix() const
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 5510c009..5b9238e2 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
> > >  		writer_ = nullptr;
> > >  	}
> > >  
> > > +	requests_.clear();
> > > +
> > >  	delete allocator;
> > >  
> > >  	return ret;
> > > @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  	 * example pushing a button. For now run all streams all the time.
> > >  	 */
> > >  
> > > -	std::vector<Request *> requests;
> > >  	for (unsigned int i = 0; i < nbuffers; i++) {
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request) {
> > >  			std::cerr << "Can't create request" << std::endl;
> > >  			return -ENOMEM;
> > > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  				writer_->mapBuffer(buffer.get());
> > >  		}
> > >  
> > > -		requests.push_back(request);
> > > +		requests_.push_back(std::move(request));
> > >  	}
> > >  
> > >  	ret = camera_->start();
> > > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  		return ret;
> > >  	}
> > >  
> > > -	for (Request *request : requests) {
> > > -		ret = camera_->queueRequest(request);
> > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > +		ret = camera_->queueRequest(request.get());
> > 
> > Would it not make sens to take this all the way and have 
> > Camera::queueRequest() take a unique pointer that is returned in 
> > requestComplete(). Then the application could store the unique ptr and 
> > reuse it or let it expire and have the request deleted?
> 
> The problem with queueRequest() taking a unique pointer is that the
> camera then owns the request... but it can't give ownership back to the
> application since the completion handler signal can have multiple slots.
> We can't give all of them a unique pointer, but if we give them a
> regular pointer, then whose responsibility is it for freeing the
> request? So we make the application keep ownership of the request. If it
> relinquishes ownership then it can free it at the end of the completion
> handler (like android and gstreamer does now).
> 
> > >  		if (ret < 0) {
> > >  			std::cerr << "Can't queue request" << std::endl;
> > >  			camera_->stop();
> > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
> > >  	if (request->status() == Request::RequestCancelled)
> > >  		return;
> > >  
> > > -	const Request::BufferMap &buffers = request->buffers();
> > > +	const Request::BufferMap buffers = request->buffers();
> > >  
> > >  	/*
> > >  	 * Compute the frame rate. The timestamp is arbitrarily retrieved from
> > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)
> > >  	 * Create a new request and populate it with one buffer for each
> > >  	 * stream.
> > >  	 */
> > > -	request = camera_->createRequest();
> > > -	if (!request) {
> > > -		std::cerr << "Can't create request" << std::endl;
> > > -		return;
> > > -	}
> > > +	request->reset();
> > >  
> > >  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > >  		const Stream *stream = it->first;
> > > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > > index 0aebdac9..45e5e8a9 100644
> > > --- a/src/cam/capture.h
> > > +++ b/src/cam/capture.h
> > > @@ -9,6 +9,7 @@
> > >  
> > >  #include <memory>
> > >  #include <stdint.h>
> > > +#include <vector>
> > >  
> > >  #include <libcamera/buffer.h>
> > >  #include <libcamera/camera.h>
> > > @@ -43,6 +44,8 @@ private:
> > >  	EventLoop *loop_;
> > >  	unsigned int captureCount_;
> > >  	unsigned int captureLimit_;
> > > +
> > > +	std::vector<std::unique_ptr<libcamera::Request>> requests_;
> > >  };
> > >  
> > >  #endif /* __CAM_CAPTURE_H__ */
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 1bfc2e2f..ca223df6 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
> > >  		if (item.second)
> > >  			gst_buffer_unref(item.second);
> > >  	}
> > > +
> > > +	delete request_;
> > >  }
> > >  
> > >  void RequestWrap::attachBuffer(GstBuffer *buffer)
> > > @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > >  	GstLibcameraSrcState *state = self->state;
> > >  
> > > -	Request *request = state->cam_->createRequest();
> > > +	std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> > > +	Request *request = request_unique.release();
> > >  	auto wrap = std::make_unique<RequestWrap>(request);
> > >  	for (GstPad *srcpad : state->srcpads_) {
> > >  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  			 * queueing this one due to lack of buffers.
> > >  			 */
> > >  			delete request;
> > > -			request = nullptr;
> > >  			break;
> > >  		}
> > >  
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index ae16a64a..8c2bbc76 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
> > >   * handler, and is completely opaque to libcamera.
> > >   *
> > >   * The ownership of the returned request is passed to the caller, which is
> > > - * responsible for either queueing the request or deleting it.
> > > + * responsible for either queueing the request or deleting it. The caller
> > > + * continues to own the request after queueing it, so deleting the request
> > > + * is the responsibility of the application after the request completes.
> > >   *
> > >   * \context This function is \threadsafe. It may only be called when the camera
> > >   * is in the Configured or Running state as defined in \ref camera_operation.
> > >   *
> > >   * \return A pointer to the newly created request, or nullptr on error
> > >   */
> > > -Request *Camera::createRequest(uint64_t cookie)
> > > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > >  {
> > >  	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > >  				      Private::CameraRunning);
> > >  	if (ret < 0)
> > >  		return nullptr;
> > >  
> > > -	return new Request(this, cookie);
> > > +	return std::make_unique<Request>(this, cookie);
> > >  }
> > >  
> > >  /**
> > > @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
> > >   * Once the request has been queued, the camera will notify its completion
> > >   * through the \ref requestCompleted signal.
> > >   *
> > > - * Ownership of the request is transferred to the camera. It will be deleted
> > > - * automatically after it completes.
> > > - *
> > >   * \context This function is \threadsafe. It may only be called when the camera
> > >   * is in the Running state as defined in \ref camera_operation.
> > >   *
> > > @@ -974,13 +973,11 @@ int Camera::stop()
> > >   * \param[in] request The request that has completed
> > >   *
> > >   * This function is called by the pipeline handler to notify the camera that
> > > - * the request has completed. It emits the requestCompleted signal and deletes
> > > - * the request.
> > > + * the request has completed. It emits the requestCompleted signal.
> > >   */
> > >  void Camera::requestComplete(Request *request)
> > >  {
> > >  	requestCompleted.emit(request);
> > > -	delete request;
> > >  }
> > >  
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 60b30692..dd3040fe 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -85,6 +85,30 @@ Request::~Request()
> > >  	delete validator_;
> > >  }
> > >  
> > > +/*
> > > + * \brief Reset the request
> > > + *
> > > + * Reset the request, to allow it to be reused and requeued without
> > > + * destruction. All contents of the request will be reset, so any references
> > > + * previously returned by the request will become invalid.
> > > + */
> > 
> > Would it make sens to split this function in two? One that resets the 
> > status_, cancelled_, metadata_ and any output controls and one that 
> > clears the buffers.
> > 
> > The reset of output parameters could be called by Camera::queueRequest() 
> > and the clearBuffers() would be called optionally by applications.
> > 
> > That way applications would be able to both reuse a request by either by 
> > keeping all buffer associations or clearing them and repopulating them 
> > with new buffers. Also by clearing output parameters in the framework 
> > applications will be unable to get it wrong.
> 
> Oh interesting idea.
> 
> Okay, I did a reset() that's called in Camera::queueRequest(), that
> resets status_, cancelled_, metadata_, controls_, and validator_. Then a
> clearBuffers() that nukes the buffer map. And a reAddBuffers() that
> takes the current buffer map, and reattaches the request to the buffers
> (otherwise we get a segfault on buffer->request_ since completeBuffer()
> nulls buffer->request_ and there's no addBuffer() to reattach it).
> 
> And so the apps that:
> - still use transient request (android, gstreamer)
>   - can just delete it at the end of the completion handler
> - the apps the want to reuse the buffers (cam)
>   - can just call request->reAddBuffers() at the end of the completion
>     handler
> - and apps that need to change up their buffers (qcam, v4l2)
>   - can do so at the end of the completion handler, with no special
>     calls

For this last category, clearBuffers() is required before changing the
buffers.


Paul

> 
> See v3 for details :)
> 
> 
> Thanks,
> 
> Paul
> 
> > > +void Request::reset()
> > > +{
> > > +	bufferMap_.clear();
> > > +	pending_.clear();
> > > +
> > > +	status_ = RequestPending;
> > > +	cancelled_ = false;
> > > +
> > > +	delete metadata_;
> > > +	delete controls_;
> > > +	delete validator_;
> > > +
> > > +	validator_ = new CameraControlValidator(camera_);
> > > +	controls_ = new ControlList(controls::controls, validator_);
> > > +	metadata_ = new ControlList(controls::controls);
> > > +}
> > > +
> > >  /**
> > >   * \fn Request::controls()
> > >   * \brief Retrieve the request's ControlList
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 985743f3..c2f8b175 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
> > >  int MainWindow::startCapture()
> > >  {
> > >  	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > -	std::vector<Request *> requests;
> > >  	int ret;
> > >  
> > >  	/* Verify roles are supported. */
> > > @@ -486,7 +485,7 @@ int MainWindow::startCapture()
> > >  	while (!freeBuffers_[vfStream_].isEmpty()) {
> > >  		FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
> > >  
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request) {
> > >  			qWarning() << "Can't create request";
> > >  			ret = -ENOMEM;
> > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> > >  			goto error;
> > >  		}
> > >  
> > > -		requests.push_back(request);
> > > +		requests_.push_back(std::move(request));
> > >  	}
> > >  
> > >  	/* Start the title timer and the camera. */
> > > @@ -518,8 +517,8 @@ int MainWindow::startCapture()
> > >  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > >  
> > >  	/* Queue all requests. */
> > > -	for (Request *request : requests) {
> > > -		ret = camera_->queueRequest(request);
> > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > +		ret = camera_->queueRequest(request.get());
> > >  		if (ret < 0) {
> > >  			qWarning() << "Can't queue request";
> > >  			goto error_disconnect;
> > > @@ -535,8 +534,7 @@ error_disconnect:
> > >  	camera_->stop();
> > >  
> > >  error:
> > > -	for (Request *request : requests)
> > > -		delete request;
> > > +	requests_.clear();
> > >  
> > >  	for (auto &iter : mappedBuffers_) {
> > >  		const MappedBuffer &buffer = iter.second;
> > > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
> > >  	}
> > >  	mappedBuffers_.clear();
> > >  
> > > +	requests_.clear();
> > > +
> > >  	delete allocator_;
> > >  
> > >  	isCapturing_ = false;
> > > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
> > >  	 */
> > >  	{
> > >  		QMutexLocker locker(&mutex_);
> > > -		doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > > +		doneQueue_.enqueue(request);
> > >  	}
> > >  
> > >  	QCoreApplication::postEvent(this, new CaptureEvent);
> > > @@ -714,8 +714,7 @@ void MainWindow::processCapture()
> > >  	 * if stopCapture() has been called while a CaptureEvent was posted but
> > >  	 * not processed yet. Return immediately in that case.
> > >  	 */
> > > -	CaptureRequest request;
> > > -
> > > +	Request *request;
> > >  	{
> > >  		QMutexLocker locker(&mutex_);
> > >  		if (doneQueue_.isEmpty())
> > > @@ -725,11 +724,19 @@ void MainWindow::processCapture()
> > >  	}
> > >  
> > >  	/* Process buffers. */
> > > -	if (request.buffers_.count(vfStream_))
> > > -		processViewfinder(request.buffers_[vfStream_]);
> > > +	if (request->buffers().count(vfStream_)) {
> > > +		{
> > > +			QMutexLocker locker(&mutex_);
> > > +			freeQueue_.enqueue(request);
> > > +		}
> > > +		Request::BufferMap buffers = request->buffers();
> > > +		processViewfinder(buffers[vfStream_]);
> > > +	}
> > >  
> > > -	if (request.buffers_.count(rawStream_))
> > > -		processRaw(request.buffers_[rawStream_], request.metadata_);
> > > +	if (request->buffers().count(rawStream_)) {
> > > +		Request::BufferMap buffers = request->buffers();
> > > +		processRaw(buffers[rawStream_], request->metadata());
> > > +	}
> > >  }
> > >  
> > >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > >  
> > >  void MainWindow::queueRequest(FrameBuffer *buffer)
> > >  {
> > > -	Request *request = camera_->createRequest();
> > > -	if (!request) {
> > > -		qWarning() << "Can't create request";
> > > -		return;
> > > +	Request *request;
> > > +	{
> > > +		QMutexLocker locker(&mutex_);
> > > +		if (freeQueue_.isEmpty())
> > > +			return;
> > > +
> > > +		request = freeQueue_.dequeue();
> > >  	}
> > >  
> > > +	request->reset();
> > > +
> > >  	request->addBuffer(vfStream_, buffer);
> > >  
> > >  	if (captureRaw_) {
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 5c61a4df..64bcfebc 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -8,6 +8,7 @@
> > >  #define __QCAM_MAIN_WINDOW_H__
> > >  
> > >  #include <memory>
> > > +#include <vector>
> > >  
> > >  #include <QElapsedTimer>
> > >  #include <QIcon>
> > > @@ -22,6 +23,7 @@
> > >  #include <libcamera/camera_manager.h>
> > >  #include <libcamera/controls.h>
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/request.h>
> > >  #include <libcamera/stream.h>
> > >  
> > >  #include "../cam/stream_options.h"
> > > @@ -41,23 +43,6 @@ enum {
> > >  	OptStream = 's',
> > >  };
> > >  
> > > -class CaptureRequest
> > > -{
> > > -public:
> > > -	CaptureRequest()
> > > -	{
> > > -	}
> > > -
> > > -	CaptureRequest(const Request::BufferMap &buffers,
> > > -		       const ControlList &metadata)
> > > -		: buffers_(buffers), metadata_(metadata)
> > > -	{
> > > -	}
> > > -
> > > -	Request::BufferMap buffers_;
> > > -	ControlList metadata_;
> > > -};
> > > -
> > >  class MainWindow : public QMainWindow
> > >  {
> > >  	Q_OBJECT
> > > @@ -128,13 +113,16 @@ private:
> > >  	Stream *vfStream_;
> > >  	Stream *rawStream_;
> > >  	std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> > > -	QQueue<CaptureRequest> doneQueue_;
> > > -	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> > > +	QQueue<Request *> doneQueue_;
> > > +	QQueue<Request *> freeQueue_;
> > > +	QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
> > >  
> > >  	uint64_t lastBufferTime_;
> > >  	QElapsedTimer frameRateInterval_;
> > >  	uint32_t previousFrames_;
> > >  	uint32_t framesCaptured_;
> > > +
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > >  };
> > >  
> > >  #endif /* __QCAM_MAIN_WINDOW__ */
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 3565f369..0d59502a 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
> > >  
> > >  void V4L2Camera::close()
> > >  {
> > > +	requestPool_.clear();
> > > +
> > >  	delete bufferAllocator_;
> > >  	bufferAllocator_ = nullptr;
> > >  
> > > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> > >  	return 0;
> > >  }
> > >  
> > > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> > > +int V4L2Camera::allocBuffers(unsigned int count)
> > >  {
> > >  	Stream *stream = config_->at(0).stream();
> > >  
> > > -	return bufferAllocator_->allocate(stream);
> > > +	int ret = bufferAllocator_->allocate(stream);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	for (unsigned int i = 0; i < count; i++) {
> > > +		std::unique_ptr<Request> request = camera_->createRequest(i);
> > > +		if (!request) {
> > > +			requestPool_.clear();
> > > +			return -ENOMEM;
> > > +		}
> > > +		requestPool_.push_back(std::move(request));
> > > +	}
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  void V4L2Camera::freeBuffers()
> > >  {
> > >  	pendingRequests_.clear();
> > > +	requestPool_.clear();
> > >  
> > >  	Stream *stream = config_->at(0).stream();
> > >  	bufferAllocator_->free(stream);
> > > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
> > >  
> > >  	isRunning_ = true;
> > >  
> > > -	for (std::unique_ptr<Request> &req : pendingRequests_) {
> > > +	for (Request *req : pendingRequests_) {
> > >  		/* \todo What should we do if this returns -EINVAL? */
> > > -		ret = camera_->queueRequest(req.release());
> > > +		ret = camera_->queueRequest(req);
> > >  		if (ret < 0)
> > >  			return ret == -EACCES ? -EBUSY : ret;
> > >  	}
> > > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
> > >  
> > >  int V4L2Camera::qbuf(unsigned int index)
> > >  {
> > > -	std::unique_ptr<Request> request =
> > > -		std::unique_ptr<Request>(camera_->createRequest(index));
> > > -	if (!request) {
> > > -		LOG(V4L2Compat, Error) << "Can't create request";
> > > -		return -ENOMEM;
> > > +	if (index >= requestPool_.size()) {
> > > +		LOG(V4L2Compat, Error) << "Invalid index";
> > > +		return -EINVAL;
> > >  	}
> > > +	Request *request = requestPool_[index].get();
> > > +	request->reset();
> > >  
> > >  	Stream *stream = config_->at(0).stream();
> > >  	FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> > > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
> > >  	}
> > >  
> > >  	if (!isRunning_) {
> > > -		pendingRequests_.push_back(std::move(request));
> > > +		pendingRequests_.push_back(request);
> > >  		return 0;
> > >  	}
> > >  
> > > -	ret = camera_->queueRequest(request.release());
> > > +	ret = camera_->queueRequest(request);
> > >  	if (ret < 0) {
> > >  		LOG(V4L2Compat, Error) << "Can't queue request";
> > >  		return ret == -EACCES ? -EBUSY : ret;
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index 1fc5ebef..a6c35a2e 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -76,7 +76,9 @@ private:
> > >  	std::mutex bufferLock_;
> > >  	FrameBufferAllocator *bufferAllocator_;
> > >  
> > > -	std::deque<std::unique_ptr<Request>> pendingRequests_;
> > > +	std::vector<std::unique_ptr<Request>> requestPool_;
> > > +
> > > +	std::deque<Request *> pendingRequests_;
> > >  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
> > >  
> > >  	int efd_;
> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > index 64e96264..9cfdb388 100644
> > > --- a/test/camera/buffer_import.cpp
> > > +++ b/test/camera/buffer_import.cpp
> > > @@ -50,7 +50,7 @@ protected:
> > >  		if (request->status() != Request::RequestComplete)
> > >  			return;
> > >  
> > > -		const Request::BufferMap &buffers = request->buffers();
> > > +		const Request::BufferMap buffers = request->buffers();
> > >  
> > >  		completeRequestsCount_++;
> > >  
> > > @@ -58,7 +58,7 @@ protected:
> > >  		const Stream *stream = buffers.begin()->first;
> > >  		FrameBuffer *buffer = buffers.begin()->second;
> > >  
> > > -		request = camera_->createRequest();
> > > +		request->reset();
> > >  		request->addBuffer(stream, buffer);
> > >  		camera_->queueRequest(request);
> > >  	}
> > > @@ -98,9 +98,8 @@ protected:
> > >  		if (ret != TestPass)
> > >  			return ret;
> > >  
> > > -		std::vector<Request *> requests;
> > >  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> > > -			Request *request = camera_->createRequest();
> > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > >  			if (!request) {
> > >  				std::cout << "Failed to create request" << std::endl;
> > >  				return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > >  				return TestFail;
> > >  			}
> > >  
> > > -			requests.push_back(request);
> > > +			requests_.push_back(std::move(request));
> > >  		}
> > >  
> > >  		completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >  
> > > -		for (Request *request : requests) {
> > > -			if (camera_->queueRequest(request)) {
> > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > +			if (camera_->queueRequest(request.get())) {
> > >  				std::cout << "Failed to queue request" << std::endl;
> > >  				return TestFail;
> > >  			}
> > > @@ -160,6 +159,8 @@ protected:
> > >  	}
> > >  
> > >  private:
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > >  	unsigned int completeBuffersCount_;
> > >  	unsigned int completeRequestsCount_;
> > >  	std::unique_ptr<CameraConfiguration> config_;
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 51bbd258..5c86c490 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -44,7 +44,7 @@ protected:
> > >  		if (request->status() != Request::RequestComplete)
> > >  			return;
> > >  
> > > -		const Request::BufferMap &buffers = request->buffers();
> > > +		const Request::BufferMap buffers = request->buffers();
> > >  
> > >  		completeRequestsCount_++;
> > >  
> > > @@ -52,7 +52,7 @@ protected:
> > >  		const Stream *stream = buffers.begin()->first;
> > >  		FrameBuffer *buffer = buffers.begin()->second;
> > >  
> > > -		request = camera_->createRequest();
> > > +		request->reset();
> > >  		request->addBuffer(stream, buffer);
> > >  		camera_->queueRequest(request);
> > >  	}
> > > @@ -98,9 +98,8 @@ protected:
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > > -		std::vector<Request *> requests;
> > >  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > > -			Request *request = camera_->createRequest();
> > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > >  			if (!request) {
> > >  				cout << "Failed to create request" << endl;
> > >  				return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > >  				return TestFail;
> > >  			}
> > >  
> > > -			requests.push_back(request);
> > > +			requests_.push_back(std::move(request));
> > >  		}
> > >  
> > >  		completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >  
> > > -		for (Request *request : requests) {
> > > -			if (camera_->queueRequest(request)) {
> > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > +			if (camera_->queueRequest(request.get())) {
> > >  				cout << "Failed to queue request" << endl;
> > >  				return TestFail;
> > >  			}
> > > @@ -161,6 +160,8 @@ protected:
> > >  		return TestPass;
> > >  	}
> > >  
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > >  	std::unique_ptr<CameraConfiguration> config_;
> > >  	FrameBufferAllocator *allocator_;
> > >  };
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 28faeb91..e63ab298 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -101,13 +101,10 @@ protected:
> > >  			return TestFail;
> > >  
> > >  		/* Test operations which should pass. */
> > > -		Request *request2 = camera_->createRequest();
> > > +		std::unique_ptr<Request> request2 = camera_->createRequest();
> > >  		if (!request2)
> > >  			return TestFail;
> > >  
> > > -		/* Never handed to hardware so need to manually delete it. */
> > > -		delete request2;
> > > -
> > >  		/* Test valid state transitions, end in Running state. */
> > >  		if (camera_->release())
> > >  			return TestFail;
> > > @@ -146,7 +143,7 @@ protected:
> > >  			return TestFail;
> > >  
> > >  		/* Test operations which should pass. */
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request)
> > >  			return TestFail;
> > >  
> > > @@ -154,7 +151,7 @@ protected:
> > >  		if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
> > >  			return TestFail;
> > >  
> > > -		if (camera_->queueRequest(request))
> > > +		if (camera_->queueRequest(request.get()))
> > >  			return TestFail;
> > >  
> > >  		/* Test valid state transitions, end in Available state. */
> > > -- 
> > > 2.27.0
> > > 
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

I know you posted a v3 but I wanted to continue one discussion point and 
to keep the context I reply here. Everything applies to v3 non the less.

On 2020-09-30 18:55:07 +0900, paul.elder@ideasonboard.com wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your work.
> > 
> > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:
> > > Allow reuse of the Request object by implementing a reset() function.
> > > This means that the applications now have the responsibility of freeing
> > > the Request objects, so make all libcamera users (cam, qcam,
> > > v4l2-compat, gstreamer, android) do so.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > Changes in v2:
> > > - clear controls_ and metadata_ and validator_ in Request::reset()
> > > - use unique_ptr on application side, prior to queueRequest, and use
> > >   regular pointer for completion handler
> > > - make qcam's reuse request nicer
> > > - update Camera::queueRequest() and Camera::createRequest() documentation
> > > - add documentation for Request::reset()
> > > - make v4l2-compat reuse request
> > > - make gstreamer and android use the new createRequest API, though they
> > >   do not actually reuse the requests
> > >   - I haven't yet tested android
> > > ---
> > >  include/libcamera/camera.h        |  2 +-
> > >  include/libcamera/request.h       |  2 ++
> > >  src/android/camera_device.cpp     |  8 +++++-
> > >  src/cam/capture.cpp               | 19 ++++++------
> > >  src/cam/capture.h                 |  3 ++
> > >  src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
> > >  src/libcamera/camera.cpp          | 15 ++++------
> > >  src/libcamera/request.cpp         | 24 ++++++++++++++++
> > >  src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
> > >  src/qcam/main_window.h            | 26 +++++------------
> > >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
> > >  src/v4l2/v4l2_camera.h            |  4 ++-
> > >  test/camera/buffer_import.cpp     | 15 +++++-----
> > >  test/camera/capture.cpp           | 15 +++++-----
> > >  test/camera/statemachine.cpp      |  9 ++----
> > >  15 files changed, 141 insertions(+), 93 deletions(-)
> > > 
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 272c12c3..e4036b63 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -93,7 +93,7 @@ public:
> > >  	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > >  	int configure(CameraConfiguration *config);
> > >  
> > > -	Request *createRequest(uint64_t cookie = 0);
> > > +	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > 
> > I like this.
> 
> :)
> 
> > >  	int queueRequest(Request *request);
> > >  
> > >  	int start();
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index 5976ac50..9f615a24 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -38,6 +38,8 @@ public:
> > >  	Request &operator=(const Request &) = delete;
> > >  	~Request();
> > >  
> > > +	void reset();
> > > +
> > >  	ControlList &controls() { return *controls_; }
> > >  	ControlList &metadata() { return *metadata_; }
> > >  	const BufferMap &buffers() const { return bufferMap_; }
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 70d77a17..7f288617 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> > >  					     camera3Request->num_output_buffers);
> > >  
> > > -	Request *request =
> > > +	std::unique_ptr<Request> r =
> > >  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > > +	Request *request = r.release();
> > > +	if (!request) {
> > > +		LOG(HAL, Error) << "Failed to create request";
> > > +		return -ENOMEM;
> > > +	}
> > 
> > I don't like this ;-)
> 
> :(
> 
> > Would it not make sens to leverage the unique_ptr?
> 
> From what I could tell, android doesn't requeue the request in the
> request completion handler (CameraDevice::requestComplete()), which
> means that android uses the request transiently, like how request was
> meant to be used before (no reuse). So I kept it that way.
> 
> Or do you mean that I should let the unique pointer die on its own by
> letting it go out of scope? Wouldn't that free the request its pointing
> to, and segfault somewhere between this and the completion handler?

I meant letting the unique pointer go out of scope after the 
requestComplete handler have completed. Reading your reply bellow I 
understand this connects into that discussion so I will leave this one 
for now.

> 
> > 
> > >  
> > >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > >  		CameraStream *cameraStream =
> > > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
> > >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > >  
> > >  	delete descriptor;
> > > +	delete request;
> > >  }
> > >  
> > >  std::string CameraDevice::logPrefix() const
> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > index 5510c009..5b9238e2 100644
> > > --- a/src/cam/capture.cpp
> > > +++ b/src/cam/capture.cpp
> > > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
> > >  		writer_ = nullptr;
> > >  	}
> > >  
> > > +	requests_.clear();
> > > +
> > >  	delete allocator;
> > >  
> > >  	return ret;
> > > @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  	 * example pushing a button. For now run all streams all the time.
> > >  	 */
> > >  
> > > -	std::vector<Request *> requests;
> > >  	for (unsigned int i = 0; i < nbuffers; i++) {
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request) {
> > >  			std::cerr << "Can't create request" << std::endl;
> > >  			return -ENOMEM;
> > > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  				writer_->mapBuffer(buffer.get());
> > >  		}
> > >  
> > > -		requests.push_back(request);
> > > +		requests_.push_back(std::move(request));
> > >  	}
> > >  
> > >  	ret = camera_->start();
> > > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > >  		return ret;
> > >  	}
> > >  
> > > -	for (Request *request : requests) {
> > > -		ret = camera_->queueRequest(request);
> > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > +		ret = camera_->queueRequest(request.get());
> > 
> > Would it not make sens to take this all the way and have 
> > Camera::queueRequest() take a unique pointer that is returned in 
> > requestComplete(). Then the application could store the unique ptr and 
> > reuse it or let it expire and have the request deleted?
> 
> The problem with queueRequest() taking a unique pointer is that the
> camera then owns the request... but it can't give ownership back to the
> application since the completion handler signal can have multiple slots.
> We can't give all of them a unique pointer, but if we give them a
> regular pointer, then whose responsibility is it for freeing the
> request? So we make the application keep ownership of the request. If it
> relinquishes ownership then it can free it at the end of the completion
> handler (like android and gstreamer does now).

We have touched upon this limitation before, maybe we can solve the 
issue by finding a way to limit the requestComplete() callback to a 
single slot? I think it's such a neat idea to have the ownership of a 
Request be handed between application and libcamera in this fashion.

Would it be possible to extend our singal/slot to express a special type 
of single subscriber signal?

> 
> > >  		if (ret < 0) {
> > >  			std::cerr << "Can't queue request" << std::endl;
> > >  			camera_->stop();
> > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
> > >  	if (request->status() == Request::RequestCancelled)
> > >  		return;
> > >  
> > > -	const Request::BufferMap &buffers = request->buffers();
> > > +	const Request::BufferMap buffers = request->buffers();
> > >  
> > >  	/*
> > >  	 * Compute the frame rate. The timestamp is arbitrarily retrieved from
> > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)
> > >  	 * Create a new request and populate it with one buffer for each
> > >  	 * stream.
> > >  	 */
> > > -	request = camera_->createRequest();
> > > -	if (!request) {
> > > -		std::cerr << "Can't create request" << std::endl;
> > > -		return;
> > > -	}
> > > +	request->reset();
> > >  
> > >  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > >  		const Stream *stream = it->first;
> > > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > > index 0aebdac9..45e5e8a9 100644
> > > --- a/src/cam/capture.h
> > > +++ b/src/cam/capture.h
> > > @@ -9,6 +9,7 @@
> > >  
> > >  #include <memory>
> > >  #include <stdint.h>
> > > +#include <vector>
> > >  
> > >  #include <libcamera/buffer.h>
> > >  #include <libcamera/camera.h>
> > > @@ -43,6 +44,8 @@ private:
> > >  	EventLoop *loop_;
> > >  	unsigned int captureCount_;
> > >  	unsigned int captureLimit_;
> > > +
> > > +	std::vector<std::unique_ptr<libcamera::Request>> requests_;
> > >  };
> > >  
> > >  #endif /* __CAM_CAPTURE_H__ */
> > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > index 1bfc2e2f..ca223df6 100644
> > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
> > >  		if (item.second)
> > >  			gst_buffer_unref(item.second);
> > >  	}
> > > +
> > > +	delete request_;
> > >  }
> > >  
> > >  void RequestWrap::attachBuffer(GstBuffer *buffer)
> > > @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > >  	GstLibcameraSrcState *state = self->state;
> > >  
> > > -	Request *request = state->cam_->createRequest();
> > > +	std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> > > +	Request *request = request_unique.release();
> > >  	auto wrap = std::make_unique<RequestWrap>(request);
> > >  	for (GstPad *srcpad : state->srcpads_) {
> > >  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
> > >  			 * queueing this one due to lack of buffers.
> > >  			 */
> > >  			delete request;
> > > -			request = nullptr;
> > >  			break;
> > >  		}
> > >  
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index ae16a64a..8c2bbc76 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
> > >   * handler, and is completely opaque to libcamera.
> > >   *
> > >   * The ownership of the returned request is passed to the caller, which is
> > > - * responsible for either queueing the request or deleting it.
> > > + * responsible for either queueing the request or deleting it. The caller
> > > + * continues to own the request after queueing it, so deleting the request
> > > + * is the responsibility of the application after the request completes.
> > >   *
> > >   * \context This function is \threadsafe. It may only be called when the camera
> > >   * is in the Configured or Running state as defined in \ref camera_operation.
> > >   *
> > >   * \return A pointer to the newly created request, or nullptr on error
> > >   */
> > > -Request *Camera::createRequest(uint64_t cookie)
> > > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > >  {
> > >  	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > >  				      Private::CameraRunning);
> > >  	if (ret < 0)
> > >  		return nullptr;
> > >  
> > > -	return new Request(this, cookie);
> > > +	return std::make_unique<Request>(this, cookie);
> > >  }
> > >  
> > >  /**
> > > @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
> > >   * Once the request has been queued, the camera will notify its completion
> > >   * through the \ref requestCompleted signal.
> > >   *
> > > - * Ownership of the request is transferred to the camera. It will be deleted
> > > - * automatically after it completes.
> > > - *
> > >   * \context This function is \threadsafe. It may only be called when the camera
> > >   * is in the Running state as defined in \ref camera_operation.
> > >   *
> > > @@ -974,13 +973,11 @@ int Camera::stop()
> > >   * \param[in] request The request that has completed
> > >   *
> > >   * This function is called by the pipeline handler to notify the camera that
> > > - * the request has completed. It emits the requestCompleted signal and deletes
> > > - * the request.
> > > + * the request has completed. It emits the requestCompleted signal.
> > >   */
> > >  void Camera::requestComplete(Request *request)
> > >  {
> > >  	requestCompleted.emit(request);
> > > -	delete request;
> > >  }
> > >  
> > >  } /* namespace libcamera */
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 60b30692..dd3040fe 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -85,6 +85,30 @@ Request::~Request()
> > >  	delete validator_;
> > >  }
> > >  
> > > +/*
> > > + * \brief Reset the request
> > > + *
> > > + * Reset the request, to allow it to be reused and requeued without
> > > + * destruction. All contents of the request will be reset, so any references
> > > + * previously returned by the request will become invalid.
> > > + */
> > 
> > Would it make sens to split this function in two? One that resets the 
> > status_, cancelled_, metadata_ and any output controls and one that 
> > clears the buffers.
> > 
> > The reset of output parameters could be called by Camera::queueRequest() 
> > and the clearBuffers() would be called optionally by applications.
> > 
> > That way applications would be able to both reuse a request by either by 
> > keeping all buffer associations or clearing them and repopulating them 
> > with new buffers. Also by clearing output parameters in the framework 
> > applications will be unable to get it wrong.
> 
> Oh interesting idea.
> 
> Okay, I did a reset() that's called in Camera::queueRequest(), that
> resets status_, cancelled_, metadata_, controls_, and validator_. Then a
> clearBuffers() that nukes the buffer map. And a reAddBuffers() that
> takes the current buffer map, and reattaches the request to the buffers
> (otherwise we get a segfault on buffer->request_ since completeBuffer()
> nulls buffer->request_ and there's no addBuffer() to reattach it).
> 
> And so the apps that:
> - still use transient request (android, gstreamer)
>   - can just delete it at the end of the completion handler
> - the apps the want to reuse the buffers (cam)
>   - can just call request->reAddBuffers() at the end of the completion
>     handler
> - and apps that need to change up their buffers (qcam, v4l2)
>   - can do so at the end of the completion handler, with no special
>     calls
> 
> See v3 for details :)

Nice!

> 
> 
> Thanks,
> 
> Paul
> 
> > > +void Request::reset()
> > > +{
> > > +	bufferMap_.clear();
> > > +	pending_.clear();
> > > +
> > > +	status_ = RequestPending;
> > > +	cancelled_ = false;
> > > +
> > > +	delete metadata_;
> > > +	delete controls_;
> > > +	delete validator_;
> > > +
> > > +	validator_ = new CameraControlValidator(camera_);
> > > +	controls_ = new ControlList(controls::controls, validator_);
> > > +	metadata_ = new ControlList(controls::controls);
> > > +}
> > > +
> > >  /**
> > >   * \fn Request::controls()
> > >   * \brief Retrieve the request's ControlList
> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > index 985743f3..c2f8b175 100644
> > > --- a/src/qcam/main_window.cpp
> > > +++ b/src/qcam/main_window.cpp
> > > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
> > >  int MainWindow::startCapture()
> > >  {
> > >  	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > -	std::vector<Request *> requests;
> > >  	int ret;
> > >  
> > >  	/* Verify roles are supported. */
> > > @@ -486,7 +485,7 @@ int MainWindow::startCapture()
> > >  	while (!freeBuffers_[vfStream_].isEmpty()) {
> > >  		FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
> > >  
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request) {
> > >  			qWarning() << "Can't create request";
> > >  			ret = -ENOMEM;
> > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> > >  			goto error;
> > >  		}
> > >  
> > > -		requests.push_back(request);
> > > +		requests_.push_back(std::move(request));
> > >  	}
> > >  
> > >  	/* Start the title timer and the camera. */
> > > @@ -518,8 +517,8 @@ int MainWindow::startCapture()
> > >  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > >  
> > >  	/* Queue all requests. */
> > > -	for (Request *request : requests) {
> > > -		ret = camera_->queueRequest(request);
> > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > +		ret = camera_->queueRequest(request.get());
> > >  		if (ret < 0) {
> > >  			qWarning() << "Can't queue request";
> > >  			goto error_disconnect;
> > > @@ -535,8 +534,7 @@ error_disconnect:
> > >  	camera_->stop();
> > >  
> > >  error:
> > > -	for (Request *request : requests)
> > > -		delete request;
> > > +	requests_.clear();
> > >  
> > >  	for (auto &iter : mappedBuffers_) {
> > >  		const MappedBuffer &buffer = iter.second;
> > > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
> > >  	}
> > >  	mappedBuffers_.clear();
> > >  
> > > +	requests_.clear();
> > > +
> > >  	delete allocator_;
> > >  
> > >  	isCapturing_ = false;
> > > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
> > >  	 */
> > >  	{
> > >  		QMutexLocker locker(&mutex_);
> > > -		doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > > +		doneQueue_.enqueue(request);
> > >  	}
> > >  
> > >  	QCoreApplication::postEvent(this, new CaptureEvent);
> > > @@ -714,8 +714,7 @@ void MainWindow::processCapture()
> > >  	 * if stopCapture() has been called while a CaptureEvent was posted but
> > >  	 * not processed yet. Return immediately in that case.
> > >  	 */
> > > -	CaptureRequest request;
> > > -
> > > +	Request *request;
> > >  	{
> > >  		QMutexLocker locker(&mutex_);
> > >  		if (doneQueue_.isEmpty())
> > > @@ -725,11 +724,19 @@ void MainWindow::processCapture()
> > >  	}
> > >  
> > >  	/* Process buffers. */
> > > -	if (request.buffers_.count(vfStream_))
> > > -		processViewfinder(request.buffers_[vfStream_]);
> > > +	if (request->buffers().count(vfStream_)) {
> > > +		{
> > > +			QMutexLocker locker(&mutex_);
> > > +			freeQueue_.enqueue(request);
> > > +		}
> > > +		Request::BufferMap buffers = request->buffers();
> > > +		processViewfinder(buffers[vfStream_]);
> > > +	}
> > >  
> > > -	if (request.buffers_.count(rawStream_))
> > > -		processRaw(request.buffers_[rawStream_], request.metadata_);
> > > +	if (request->buffers().count(rawStream_)) {
> > > +		Request::BufferMap buffers = request->buffers();
> > > +		processRaw(buffers[rawStream_], request->metadata());
> > > +	}
> > >  }
> > >  
> > >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > >  
> > >  void MainWindow::queueRequest(FrameBuffer *buffer)
> > >  {
> > > -	Request *request = camera_->createRequest();
> > > -	if (!request) {
> > > -		qWarning() << "Can't create request";
> > > -		return;
> > > +	Request *request;
> > > +	{
> > > +		QMutexLocker locker(&mutex_);
> > > +		if (freeQueue_.isEmpty())
> > > +			return;
> > > +
> > > +		request = freeQueue_.dequeue();
> > >  	}
> > >  
> > > +	request->reset();
> > > +
> > >  	request->addBuffer(vfStream_, buffer);
> > >  
> > >  	if (captureRaw_) {
> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > index 5c61a4df..64bcfebc 100644
> > > --- a/src/qcam/main_window.h
> > > +++ b/src/qcam/main_window.h
> > > @@ -8,6 +8,7 @@
> > >  #define __QCAM_MAIN_WINDOW_H__
> > >  
> > >  #include <memory>
> > > +#include <vector>
> > >  
> > >  #include <QElapsedTimer>
> > >  #include <QIcon>
> > > @@ -22,6 +23,7 @@
> > >  #include <libcamera/camera_manager.h>
> > >  #include <libcamera/controls.h>
> > >  #include <libcamera/framebuffer_allocator.h>
> > > +#include <libcamera/request.h>
> > >  #include <libcamera/stream.h>
> > >  
> > >  #include "../cam/stream_options.h"
> > > @@ -41,23 +43,6 @@ enum {
> > >  	OptStream = 's',
> > >  };
> > >  
> > > -class CaptureRequest
> > > -{
> > > -public:
> > > -	CaptureRequest()
> > > -	{
> > > -	}
> > > -
> > > -	CaptureRequest(const Request::BufferMap &buffers,
> > > -		       const ControlList &metadata)
> > > -		: buffers_(buffers), metadata_(metadata)
> > > -	{
> > > -	}
> > > -
> > > -	Request::BufferMap buffers_;
> > > -	ControlList metadata_;
> > > -};
> > > -
> > >  class MainWindow : public QMainWindow
> > >  {
> > >  	Q_OBJECT
> > > @@ -128,13 +113,16 @@ private:
> > >  	Stream *vfStream_;
> > >  	Stream *rawStream_;
> > >  	std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> > > -	QQueue<CaptureRequest> doneQueue_;
> > > -	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> > > +	QQueue<Request *> doneQueue_;
> > > +	QQueue<Request *> freeQueue_;
> > > +	QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
> > >  
> > >  	uint64_t lastBufferTime_;
> > >  	QElapsedTimer frameRateInterval_;
> > >  	uint32_t previousFrames_;
> > >  	uint32_t framesCaptured_;
> > > +
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > >  };
> > >  
> > >  #endif /* __QCAM_MAIN_WINDOW__ */
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index 3565f369..0d59502a 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
> > >  
> > >  void V4L2Camera::close()
> > >  {
> > > +	requestPool_.clear();
> > > +
> > >  	delete bufferAllocator_;
> > >  	bufferAllocator_ = nullptr;
> > >  
> > > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> > >  	return 0;
> > >  }
> > >  
> > > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> > > +int V4L2Camera::allocBuffers(unsigned int count)
> > >  {
> > >  	Stream *stream = config_->at(0).stream();
> > >  
> > > -	return bufferAllocator_->allocate(stream);
> > > +	int ret = bufferAllocator_->allocate(stream);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	for (unsigned int i = 0; i < count; i++) {
> > > +		std::unique_ptr<Request> request = camera_->createRequest(i);
> > > +		if (!request) {
> > > +			requestPool_.clear();
> > > +			return -ENOMEM;
> > > +		}
> > > +		requestPool_.push_back(std::move(request));
> > > +	}
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  void V4L2Camera::freeBuffers()
> > >  {
> > >  	pendingRequests_.clear();
> > > +	requestPool_.clear();
> > >  
> > >  	Stream *stream = config_->at(0).stream();
> > >  	bufferAllocator_->free(stream);
> > > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
> > >  
> > >  	isRunning_ = true;
> > >  
> > > -	for (std::unique_ptr<Request> &req : pendingRequests_) {
> > > +	for (Request *req : pendingRequests_) {
> > >  		/* \todo What should we do if this returns -EINVAL? */
> > > -		ret = camera_->queueRequest(req.release());
> > > +		ret = camera_->queueRequest(req);
> > >  		if (ret < 0)
> > >  			return ret == -EACCES ? -EBUSY : ret;
> > >  	}
> > > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
> > >  
> > >  int V4L2Camera::qbuf(unsigned int index)
> > >  {
> > > -	std::unique_ptr<Request> request =
> > > -		std::unique_ptr<Request>(camera_->createRequest(index));
> > > -	if (!request) {
> > > -		LOG(V4L2Compat, Error) << "Can't create request";
> > > -		return -ENOMEM;
> > > +	if (index >= requestPool_.size()) {
> > > +		LOG(V4L2Compat, Error) << "Invalid index";
> > > +		return -EINVAL;
> > >  	}
> > > +	Request *request = requestPool_[index].get();
> > > +	request->reset();
> > >  
> > >  	Stream *stream = config_->at(0).stream();
> > >  	FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> > > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
> > >  	}
> > >  
> > >  	if (!isRunning_) {
> > > -		pendingRequests_.push_back(std::move(request));
> > > +		pendingRequests_.push_back(request);
> > >  		return 0;
> > >  	}
> > >  
> > > -	ret = camera_->queueRequest(request.release());
> > > +	ret = camera_->queueRequest(request);
> > >  	if (ret < 0) {
> > >  		LOG(V4L2Compat, Error) << "Can't queue request";
> > >  		return ret == -EACCES ? -EBUSY : ret;
> > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > index 1fc5ebef..a6c35a2e 100644
> > > --- a/src/v4l2/v4l2_camera.h
> > > +++ b/src/v4l2/v4l2_camera.h
> > > @@ -76,7 +76,9 @@ private:
> > >  	std::mutex bufferLock_;
> > >  	FrameBufferAllocator *bufferAllocator_;
> > >  
> > > -	std::deque<std::unique_ptr<Request>> pendingRequests_;
> > > +	std::vector<std::unique_ptr<Request>> requestPool_;
> > > +
> > > +	std::deque<Request *> pendingRequests_;
> > >  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
> > >  
> > >  	int efd_;
> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > index 64e96264..9cfdb388 100644
> > > --- a/test/camera/buffer_import.cpp
> > > +++ b/test/camera/buffer_import.cpp
> > > @@ -50,7 +50,7 @@ protected:
> > >  		if (request->status() != Request::RequestComplete)
> > >  			return;
> > >  
> > > -		const Request::BufferMap &buffers = request->buffers();
> > > +		const Request::BufferMap buffers = request->buffers();
> > >  
> > >  		completeRequestsCount_++;
> > >  
> > > @@ -58,7 +58,7 @@ protected:
> > >  		const Stream *stream = buffers.begin()->first;
> > >  		FrameBuffer *buffer = buffers.begin()->second;
> > >  
> > > -		request = camera_->createRequest();
> > > +		request->reset();
> > >  		request->addBuffer(stream, buffer);
> > >  		camera_->queueRequest(request);
> > >  	}
> > > @@ -98,9 +98,8 @@ protected:
> > >  		if (ret != TestPass)
> > >  			return ret;
> > >  
> > > -		std::vector<Request *> requests;
> > >  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> > > -			Request *request = camera_->createRequest();
> > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > >  			if (!request) {
> > >  				std::cout << "Failed to create request" << std::endl;
> > >  				return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > >  				return TestFail;
> > >  			}
> > >  
> > > -			requests.push_back(request);
> > > +			requests_.push_back(std::move(request));
> > >  		}
> > >  
> > >  		completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >  
> > > -		for (Request *request : requests) {
> > > -			if (camera_->queueRequest(request)) {
> > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > +			if (camera_->queueRequest(request.get())) {
> > >  				std::cout << "Failed to queue request" << std::endl;
> > >  				return TestFail;
> > >  			}
> > > @@ -160,6 +159,8 @@ protected:
> > >  	}
> > >  
> > >  private:
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > >  	unsigned int completeBuffersCount_;
> > >  	unsigned int completeRequestsCount_;
> > >  	std::unique_ptr<CameraConfiguration> config_;
> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > index 51bbd258..5c86c490 100644
> > > --- a/test/camera/capture.cpp
> > > +++ b/test/camera/capture.cpp
> > > @@ -44,7 +44,7 @@ protected:
> > >  		if (request->status() != Request::RequestComplete)
> > >  			return;
> > >  
> > > -		const Request::BufferMap &buffers = request->buffers();
> > > +		const Request::BufferMap buffers = request->buffers();
> > >  
> > >  		completeRequestsCount_++;
> > >  
> > > @@ -52,7 +52,7 @@ protected:
> > >  		const Stream *stream = buffers.begin()->first;
> > >  		FrameBuffer *buffer = buffers.begin()->second;
> > >  
> > > -		request = camera_->createRequest();
> > > +		request->reset();
> > >  		request->addBuffer(stream, buffer);
> > >  		camera_->queueRequest(request);
> > >  	}
> > > @@ -98,9 +98,8 @@ protected:
> > >  		if (ret < 0)
> > >  			return TestFail;
> > >  
> > > -		std::vector<Request *> requests;
> > >  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > > -			Request *request = camera_->createRequest();
> > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > >  			if (!request) {
> > >  				cout << "Failed to create request" << endl;
> > >  				return TestFail;
> > > @@ -111,7 +110,7 @@ protected:
> > >  				return TestFail;
> > >  			}
> > >  
> > > -			requests.push_back(request);
> > > +			requests_.push_back(std::move(request));
> > >  		}
> > >  
> > >  		completeRequestsCount_ = 0;
> > > @@ -125,8 +124,8 @@ protected:
> > >  			return TestFail;
> > >  		}
> > >  
> > > -		for (Request *request : requests) {
> > > -			if (camera_->queueRequest(request)) {
> > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > +			if (camera_->queueRequest(request.get())) {
> > >  				cout << "Failed to queue request" << endl;
> > >  				return TestFail;
> > >  			}
> > > @@ -161,6 +160,8 @@ protected:
> > >  		return TestPass;
> > >  	}
> > >  
> > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > +
> > >  	std::unique_ptr<CameraConfiguration> config_;
> > >  	FrameBufferAllocator *allocator_;
> > >  };
> > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > index 28faeb91..e63ab298 100644
> > > --- a/test/camera/statemachine.cpp
> > > +++ b/test/camera/statemachine.cpp
> > > @@ -101,13 +101,10 @@ protected:
> > >  			return TestFail;
> > >  
> > >  		/* Test operations which should pass. */
> > > -		Request *request2 = camera_->createRequest();
> > > +		std::unique_ptr<Request> request2 = camera_->createRequest();
> > >  		if (!request2)
> > >  			return TestFail;
> > >  
> > > -		/* Never handed to hardware so need to manually delete it. */
> > > -		delete request2;
> > > -
> > >  		/* Test valid state transitions, end in Running state. */
> > >  		if (camera_->release())
> > >  			return TestFail;
> > > @@ -146,7 +143,7 @@ protected:
> > >  			return TestFail;
> > >  
> > >  		/* Test operations which should pass. */
> > > -		Request *request = camera_->createRequest();
> > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > >  		if (!request)
> > >  			return TestFail;
> > >  
> > > @@ -154,7 +151,7 @@ protected:
> > >  		if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
> > >  			return TestFail;
> > >  
> > > -		if (camera_->queueRequest(request))
> > > +		if (camera_->queueRequest(request.get()))
> > >  			return TestFail;
> > >  
> > >  		/* Test valid state transitions, end in Available state. */
> > > -- 
> > > 2.27.0
> > > 
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Wed, Sep 30, 2020 at 12:33:18PM +0200, Niklas Söderlund wrote:
> Hi Paul,
> 
> I know you posted a v3 but I wanted to continue one discussion point and 
> to keep the context I reply here. Everything applies to v3 non the less.
> 
> On 2020-09-30 18:55:07 +0900, paul.elder@ideasonboard.com wrote:
> > Hi Niklas,
> > 
> > Thank you for the review.
> > 
> > On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:
> > > Hi Paul,
> > > 
> > > Thanks for your work.
> > > 
> > > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:
> > > > Allow reuse of the Request object by implementing a reset() function.
> > > > This means that the applications now have the responsibility of freeing
> > > > the Request objects, so make all libcamera users (cam, qcam,
> > > > v4l2-compat, gstreamer, android) do so.
> > > > 
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > 
> > > > ---
> > > > Changes in v2:
> > > > - clear controls_ and metadata_ and validator_ in Request::reset()
> > > > - use unique_ptr on application side, prior to queueRequest, and use
> > > >   regular pointer for completion handler
> > > > - make qcam's reuse request nicer
> > > > - update Camera::queueRequest() and Camera::createRequest() documentation
> > > > - add documentation for Request::reset()
> > > > - make v4l2-compat reuse request
> > > > - make gstreamer and android use the new createRequest API, though they
> > > >   do not actually reuse the requests
> > > >   - I haven't yet tested android
> > > > ---
> > > >  include/libcamera/camera.h        |  2 +-
> > > >  include/libcamera/request.h       |  2 ++
> > > >  src/android/camera_device.cpp     |  8 +++++-
> > > >  src/cam/capture.cpp               | 19 ++++++------
> > > >  src/cam/capture.h                 |  3 ++
> > > >  src/gstreamer/gstlibcamerasrc.cpp |  6 ++--
> > > >  src/libcamera/camera.cpp          | 15 ++++------
> > > >  src/libcamera/request.cpp         | 24 ++++++++++++++++
> > > >  src/qcam/main_window.cpp          | 48 +++++++++++++++++++------------
> > > >  src/qcam/main_window.h            | 26 +++++------------
> > > >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++-------
> > > >  src/v4l2/v4l2_camera.h            |  4 ++-
> > > >  test/camera/buffer_import.cpp     | 15 +++++-----
> > > >  test/camera/capture.cpp           | 15 +++++-----
> > > >  test/camera/statemachine.cpp      |  9 ++----
> > > >  15 files changed, 141 insertions(+), 93 deletions(-)
> > > > 
> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > > index 272c12c3..e4036b63 100644
> > > > --- a/include/libcamera/camera.h
> > > > +++ b/include/libcamera/camera.h
> > > > @@ -93,7 +93,7 @@ public:
> > > >  	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > > >  	int configure(CameraConfiguration *config);
> > > >  
> > > > -	Request *createRequest(uint64_t cookie = 0);
> > > > +	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > > 
> > > I like this.
> > 
> > :)
> > 
> > > >  	int queueRequest(Request *request);
> > > >  
> > > >  	int start();
> > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > > index 5976ac50..9f615a24 100644
> > > > --- a/include/libcamera/request.h
> > > > +++ b/include/libcamera/request.h
> > > > @@ -38,6 +38,8 @@ public:
> > > >  	Request &operator=(const Request &) = delete;
> > > >  	~Request();
> > > >  
> > > > +	void reset();
> > > > +
> > > >  	ControlList &controls() { return *controls_; }
> > > >  	ControlList &metadata() { return *metadata_; }
> > > >  	const BufferMap &buffers() const { return bufferMap_; }
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index 70d77a17..7f288617 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -1395,8 +1395,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > > >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> > > >  					     camera3Request->num_output_buffers);
> > > >  
> > > > -	Request *request =
> > > > +	std::unique_ptr<Request> r =
> > > >  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > > > +	Request *request = r.release();
> > > > +	if (!request) {
> > > > +		LOG(HAL, Error) << "Failed to create request";
> > > > +		return -ENOMEM;
> > > > +	}
> > > 
> > > I don't like this ;-)
> > 
> > :(
> > 
> > > Would it not make sens to leverage the unique_ptr?
> > 
> > From what I could tell, android doesn't requeue the request in the
> > request completion handler (CameraDevice::requestComplete()), which
> > means that android uses the request transiently, like how request was
> > meant to be used before (no reuse). So I kept it that way.
> > 
> > Or do you mean that I should let the unique pointer die on its own by
> > letting it go out of scope? Wouldn't that free the request its pointing
> > to, and segfault somewhere between this and the completion handler?
> 
> I meant letting the unique pointer go out of scope after the 
> requestComplete handler have completed. Reading your reply bellow I 
> understand this connects into that discussion so I will leave this one 
> for now.
> 
> > 
> > > 
> > > >  
> > > >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> > > >  		CameraStream *cameraStream =
> > > > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)
> > > >  	callbacks_->process_capture_result(callbacks_, &captureResult);
> > > >  
> > > >  	delete descriptor;
> > > > +	delete request;
> > > >  }
> > > >  
> > > >  std::string CameraDevice::logPrefix() const
> > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp
> > > > index 5510c009..5b9238e2 100644
> > > > --- a/src/cam/capture.cpp
> > > > +++ b/src/cam/capture.cpp
> > > > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)
> > > >  		writer_ = nullptr;
> > > >  	}
> > > >  
> > > > +	requests_.clear();
> > > > +
> > > >  	delete allocator;
> > > >  
> > > >  	return ret;
> > > > @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > >  	 * example pushing a button. For now run all streams all the time.
> > > >  	 */
> > > >  
> > > > -	std::vector<Request *> requests;
> > > >  	for (unsigned int i = 0; i < nbuffers; i++) {
> > > > -		Request *request = camera_->createRequest();
> > > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > > >  		if (!request) {
> > > >  			std::cerr << "Can't create request" << std::endl;
> > > >  			return -ENOMEM;
> > > > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > >  				writer_->mapBuffer(buffer.get());
> > > >  		}
> > > >  
> > > > -		requests.push_back(request);
> > > > +		requests_.push_back(std::move(request));
> > > >  	}
> > > >  
> > > >  	ret = camera_->start();
> > > > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)
> > > >  		return ret;
> > > >  	}
> > > >  
> > > > -	for (Request *request : requests) {
> > > > -		ret = camera_->queueRequest(request);
> > > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > > +		ret = camera_->queueRequest(request.get());
> > > 
> > > Would it not make sens to take this all the way and have 
> > > Camera::queueRequest() take a unique pointer that is returned in 
> > > requestComplete(). Then the application could store the unique ptr and 
> > > reuse it or let it expire and have the request deleted?
> > 
> > The problem with queueRequest() taking a unique pointer is that the
> > camera then owns the request... but it can't give ownership back to the
> > application since the completion handler signal can have multiple slots.
> > We can't give all of them a unique pointer, but if we give them a
> > regular pointer, then whose responsibility is it for freeing the
> > request? So we make the application keep ownership of the request. If it
> > relinquishes ownership then it can free it at the end of the completion
> > handler (like android and gstreamer does now).
> 
> We have touched upon this limitation before, maybe we can solve the 
> issue by finding a way to limit the requestComplete() callback to a 
> single slot? I think it's such a neat idea to have the ownership of a 
> Request be handed between application and libcamera in this fashion.

If I remember correctly, the limitation is because BoundMethodMember
needs to make a copy of the parameters to the slot, and that doesn't
work well with unique_ptr.

> Would it be possible to extend our singal/slot to express a special type 
> of single subscriber signal?

...maybe? Like class SignalSingle : public SignalBase which keeps a
counter of how many slots are connected, and limit it to 1? We'd still
have to extend BoundMethodMember though, to use std::move instead of
std::copy...

Conclusion: not now :)


Paul

> > 
> > > >  		if (ret < 0) {
> > > >  			std::cerr << "Can't queue request" << std::endl;
> > > >  			camera_->stop();
> > > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)
> > > >  	if (request->status() == Request::RequestCancelled)
> > > >  		return;
> > > >  
> > > > -	const Request::BufferMap &buffers = request->buffers();
> > > > +	const Request::BufferMap buffers = request->buffers();
> > > >  
> > > >  	/*
> > > >  	 * Compute the frame rate. The timestamp is arbitrarily retrieved from
> > > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)
> > > >  	 * Create a new request and populate it with one buffer for each
> > > >  	 * stream.
> > > >  	 */
> > > > -	request = camera_->createRequest();
> > > > -	if (!request) {
> > > > -		std::cerr << "Can't create request" << std::endl;
> > > > -		return;
> > > > -	}
> > > > +	request->reset();
> > > >  
> > > >  	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > > >  		const Stream *stream = it->first;
> > > > diff --git a/src/cam/capture.h b/src/cam/capture.h
> > > > index 0aebdac9..45e5e8a9 100644
> > > > --- a/src/cam/capture.h
> > > > +++ b/src/cam/capture.h
> > > > @@ -9,6 +9,7 @@
> > > >  
> > > >  #include <memory>
> > > >  #include <stdint.h>
> > > > +#include <vector>
> > > >  
> > > >  #include <libcamera/buffer.h>
> > > >  #include <libcamera/camera.h>
> > > > @@ -43,6 +44,8 @@ private:
> > > >  	EventLoop *loop_;
> > > >  	unsigned int captureCount_;
> > > >  	unsigned int captureLimit_;
> > > > +
> > > > +	std::vector<std::unique_ptr<libcamera::Request>> requests_;
> > > >  };
> > > >  
> > > >  #endif /* __CAM_CAPTURE_H__ */
> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > > index 1bfc2e2f..ca223df6 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()
> > > >  		if (item.second)
> > > >  			gst_buffer_unref(item.second);
> > > >  	}
> > > > +
> > > > +	delete request_;
> > > >  }
> > > >  
> > > >  void RequestWrap::attachBuffer(GstBuffer *buffer)
> > > > @@ -266,7 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)
> > > >  	GstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);
> > > >  	GstLibcameraSrcState *state = self->state;
> > > >  
> > > > -	Request *request = state->cam_->createRequest();
> > > > +	std::unique_ptr<Request> request_unique = state->cam_->createRequest();
> > > > +	Request *request = request_unique.release();
> > > >  	auto wrap = std::make_unique<RequestWrap>(request);
> > > >  	for (GstPad *srcpad : state->srcpads_) {
> > > >  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> > > > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)
> > > >  			 * queueing this one due to lack of buffers.
> > > >  			 */
> > > >  			delete request;
> > > > -			request = nullptr;
> > > >  			break;
> > > >  		}
> > > >  
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index ae16a64a..8c2bbc76 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -833,21 +833,23 @@ int Camera::configure(CameraConfiguration *config)
> > > >   * handler, and is completely opaque to libcamera.
> > > >   *
> > > >   * The ownership of the returned request is passed to the caller, which is
> > > > - * responsible for either queueing the request or deleting it.
> > > > + * responsible for either queueing the request or deleting it. The caller
> > > > + * continues to own the request after queueing it, so deleting the request
> > > > + * is the responsibility of the application after the request completes.
> > > >   *
> > > >   * \context This function is \threadsafe. It may only be called when the camera
> > > >   * is in the Configured or Running state as defined in \ref camera_operation.
> > > >   *
> > > >   * \return A pointer to the newly created request, or nullptr on error
> > > >   */
> > > > -Request *Camera::createRequest(uint64_t cookie)
> > > > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)
> > > >  {
> > > >  	int ret = p_->isAccessAllowed(Private::CameraConfigured,
> > > >  				      Private::CameraRunning);
> > > >  	if (ret < 0)
> > > >  		return nullptr;
> > > >  
> > > > -	return new Request(this, cookie);
> > > > +	return std::make_unique<Request>(this, cookie);
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -863,9 +865,6 @@ Request *Camera::createRequest(uint64_t cookie)
> > > >   * Once the request has been queued, the camera will notify its completion
> > > >   * through the \ref requestCompleted signal.
> > > >   *
> > > > - * Ownership of the request is transferred to the camera. It will be deleted
> > > > - * automatically after it completes.
> > > > - *
> > > >   * \context This function is \threadsafe. It may only be called when the camera
> > > >   * is in the Running state as defined in \ref camera_operation.
> > > >   *
> > > > @@ -974,13 +973,11 @@ int Camera::stop()
> > > >   * \param[in] request The request that has completed
> > > >   *
> > > >   * This function is called by the pipeline handler to notify the camera that
> > > > - * the request has completed. It emits the requestCompleted signal and deletes
> > > > - * the request.
> > > > + * the request has completed. It emits the requestCompleted signal.
> > > >   */
> > > >  void Camera::requestComplete(Request *request)
> > > >  {
> > > >  	requestCompleted.emit(request);
> > > > -	delete request;
> > > >  }
> > > >  
> > > >  } /* namespace libcamera */
> > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > > index 60b30692..dd3040fe 100644
> > > > --- a/src/libcamera/request.cpp
> > > > +++ b/src/libcamera/request.cpp
> > > > @@ -85,6 +85,30 @@ Request::~Request()
> > > >  	delete validator_;
> > > >  }
> > > >  
> > > > +/*
> > > > + * \brief Reset the request
> > > > + *
> > > > + * Reset the request, to allow it to be reused and requeued without
> > > > + * destruction. All contents of the request will be reset, so any references
> > > > + * previously returned by the request will become invalid.
> > > > + */
> > > 
> > > Would it make sens to split this function in two? One that resets the 
> > > status_, cancelled_, metadata_ and any output controls and one that 
> > > clears the buffers.
> > > 
> > > The reset of output parameters could be called by Camera::queueRequest() 
> > > and the clearBuffers() would be called optionally by applications.
> > > 
> > > That way applications would be able to both reuse a request by either by 
> > > keeping all buffer associations or clearing them and repopulating them 
> > > with new buffers. Also by clearing output parameters in the framework 
> > > applications will be unable to get it wrong.
> > 
> > Oh interesting idea.
> > 
> > Okay, I did a reset() that's called in Camera::queueRequest(), that
> > resets status_, cancelled_, metadata_, controls_, and validator_. Then a
> > clearBuffers() that nukes the buffer map. And a reAddBuffers() that
> > takes the current buffer map, and reattaches the request to the buffers
> > (otherwise we get a segfault on buffer->request_ since completeBuffer()
> > nulls buffer->request_ and there's no addBuffer() to reattach it).
> > 
> > And so the apps that:
> > - still use transient request (android, gstreamer)
> >   - can just delete it at the end of the completion handler
> > - the apps the want to reuse the buffers (cam)
> >   - can just call request->reAddBuffers() at the end of the completion
> >     handler
> > - and apps that need to change up their buffers (qcam, v4l2)
> >   - can do so at the end of the completion handler, with no special
> >     calls
> > 
> > See v3 for details :)
> 
> Nice!
> 
> > 
> > 
> > Thanks,
> > 
> > Paul
> > 
> > > > +void Request::reset()
> > > > +{
> > > > +	bufferMap_.clear();
> > > > +	pending_.clear();
> > > > +
> > > > +	status_ = RequestPending;
> > > > +	cancelled_ = false;
> > > > +
> > > > +	delete metadata_;
> > > > +	delete controls_;
> > > > +	delete validator_;
> > > > +
> > > > +	validator_ = new CameraControlValidator(camera_);
> > > > +	controls_ = new ControlList(controls::controls, validator_);
> > > > +	metadata_ = new ControlList(controls::controls);
> > > > +}
> > > > +
> > > >  /**
> > > >   * \fn Request::controls()
> > > >   * \brief Retrieve the request's ControlList
> > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > > index 985743f3..c2f8b175 100644
> > > > --- a/src/qcam/main_window.cpp
> > > > +++ b/src/qcam/main_window.cpp
> > > > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)
> > > >  int MainWindow::startCapture()
> > > >  {
> > > >  	StreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);
> > > > -	std::vector<Request *> requests;
> > > >  	int ret;
> > > >  
> > > >  	/* Verify roles are supported. */
> > > > @@ -486,7 +485,7 @@ int MainWindow::startCapture()
> > > >  	while (!freeBuffers_[vfStream_].isEmpty()) {
> > > >  		FrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();
> > > >  
> > > > -		Request *request = camera_->createRequest();
> > > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > > >  		if (!request) {
> > > >  			qWarning() << "Can't create request";
> > > >  			ret = -ENOMEM;
> > > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()
> > > >  			goto error;
> > > >  		}
> > > >  
> > > > -		requests.push_back(request);
> > > > +		requests_.push_back(std::move(request));
> > > >  	}
> > > >  
> > > >  	/* Start the title timer and the camera. */
> > > > @@ -518,8 +517,8 @@ int MainWindow::startCapture()
> > > >  	camera_->requestCompleted.connect(this, &MainWindow::requestComplete);
> > > >  
> > > >  	/* Queue all requests. */
> > > > -	for (Request *request : requests) {
> > > > -		ret = camera_->queueRequest(request);
> > > > +	for (std::unique_ptr<Request> &request : requests_) {
> > > > +		ret = camera_->queueRequest(request.get());
> > > >  		if (ret < 0) {
> > > >  			qWarning() << "Can't queue request";
> > > >  			goto error_disconnect;
> > > > @@ -535,8 +534,7 @@ error_disconnect:
> > > >  	camera_->stop();
> > > >  
> > > >  error:
> > > > -	for (Request *request : requests)
> > > > -		delete request;
> > > > +	requests_.clear();
> > > >  
> > > >  	for (auto &iter : mappedBuffers_) {
> > > >  		const MappedBuffer &buffer = iter.second;
> > > > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()
> > > >  	}
> > > >  	mappedBuffers_.clear();
> > > >  
> > > > +	requests_.clear();
> > > > +
> > > >  	delete allocator_;
> > > >  
> > > >  	isCapturing_ = false;
> > > > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)
> > > >  	 */
> > > >  	{
> > > >  		QMutexLocker locker(&mutex_);
> > > > -		doneQueue_.enqueue({ request->buffers(), request->metadata() });
> > > > +		doneQueue_.enqueue(request);
> > > >  	}
> > > >  
> > > >  	QCoreApplication::postEvent(this, new CaptureEvent);
> > > > @@ -714,8 +714,7 @@ void MainWindow::processCapture()
> > > >  	 * if stopCapture() has been called while a CaptureEvent was posted but
> > > >  	 * not processed yet. Return immediately in that case.
> > > >  	 */
> > > > -	CaptureRequest request;
> > > > -
> > > > +	Request *request;
> > > >  	{
> > > >  		QMutexLocker locker(&mutex_);
> > > >  		if (doneQueue_.isEmpty())
> > > > @@ -725,11 +724,19 @@ void MainWindow::processCapture()
> > > >  	}
> > > >  
> > > >  	/* Process buffers. */
> > > > -	if (request.buffers_.count(vfStream_))
> > > > -		processViewfinder(request.buffers_[vfStream_]);
> > > > +	if (request->buffers().count(vfStream_)) {
> > > > +		{
> > > > +			QMutexLocker locker(&mutex_);
> > > > +			freeQueue_.enqueue(request);
> > > > +		}
> > > > +		Request::BufferMap buffers = request->buffers();
> > > > +		processViewfinder(buffers[vfStream_]);
> > > > +	}
> > > >  
> > > > -	if (request.buffers_.count(rawStream_))
> > > > -		processRaw(request.buffers_[rawStream_], request.metadata_);
> > > > +	if (request->buffers().count(rawStream_)) {
> > > > +		Request::BufferMap buffers = request->buffers();
> > > > +		processRaw(buffers[rawStream_], request->metadata());
> > > > +	}
> > > >  }
> > > >  
> > > >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > > @@ -754,12 +761,17 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > >  
> > > >  void MainWindow::queueRequest(FrameBuffer *buffer)
> > > >  {
> > > > -	Request *request = camera_->createRequest();
> > > > -	if (!request) {
> > > > -		qWarning() << "Can't create request";
> > > > -		return;
> > > > +	Request *request;
> > > > +	{
> > > > +		QMutexLocker locker(&mutex_);
> > > > +		if (freeQueue_.isEmpty())
> > > > +			return;
> > > > +
> > > > +		request = freeQueue_.dequeue();
> > > >  	}
> > > >  
> > > > +	request->reset();
> > > > +
> > > >  	request->addBuffer(vfStream_, buffer);
> > > >  
> > > >  	if (captureRaw_) {
> > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > > > index 5c61a4df..64bcfebc 100644
> > > > --- a/src/qcam/main_window.h
> > > > +++ b/src/qcam/main_window.h
> > > > @@ -8,6 +8,7 @@
> > > >  #define __QCAM_MAIN_WINDOW_H__
> > > >  
> > > >  #include <memory>
> > > > +#include <vector>
> > > >  
> > > >  #include <QElapsedTimer>
> > > >  #include <QIcon>
> > > > @@ -22,6 +23,7 @@
> > > >  #include <libcamera/camera_manager.h>
> > > >  #include <libcamera/controls.h>
> > > >  #include <libcamera/framebuffer_allocator.h>
> > > > +#include <libcamera/request.h>
> > > >  #include <libcamera/stream.h>
> > > >  
> > > >  #include "../cam/stream_options.h"
> > > > @@ -41,23 +43,6 @@ enum {
> > > >  	OptStream = 's',
> > > >  };
> > > >  
> > > > -class CaptureRequest
> > > > -{
> > > > -public:
> > > > -	CaptureRequest()
> > > > -	{
> > > > -	}
> > > > -
> > > > -	CaptureRequest(const Request::BufferMap &buffers,
> > > > -		       const ControlList &metadata)
> > > > -		: buffers_(buffers), metadata_(metadata)
> > > > -	{
> > > > -	}
> > > > -
> > > > -	Request::BufferMap buffers_;
> > > > -	ControlList metadata_;
> > > > -};
> > > > -
> > > >  class MainWindow : public QMainWindow
> > > >  {
> > > >  	Q_OBJECT
> > > > @@ -128,13 +113,16 @@ private:
> > > >  	Stream *vfStream_;
> > > >  	Stream *rawStream_;
> > > >  	std::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;
> > > > -	QQueue<CaptureRequest> doneQueue_;
> > > > -	QMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */
> > > > +	QQueue<Request *> doneQueue_;
> > > > +	QQueue<Request *> freeQueue_;
> > > > +	QMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */
> > > >  
> > > >  	uint64_t lastBufferTime_;
> > > >  	QElapsedTimer frameRateInterval_;
> > > >  	uint32_t previousFrames_;
> > > >  	uint32_t framesCaptured_;
> > > > +
> > > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > >  };
> > > >  
> > > >  #endif /* __QCAM_MAIN_WINDOW__ */
> > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > > index 3565f369..0d59502a 100644
> > > > --- a/src/v4l2/v4l2_camera.cpp
> > > > +++ b/src/v4l2/v4l2_camera.cpp
> > > > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)
> > > >  
> > > >  void V4L2Camera::close()
> > > >  {
> > > > +	requestPool_.clear();
> > > > +
> > > >  	delete bufferAllocator_;
> > > >  	bufferAllocator_ = nullptr;
> > > >  
> > > > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)
> > > > +int V4L2Camera::allocBuffers(unsigned int count)
> > > >  {
> > > >  	Stream *stream = config_->at(0).stream();
> > > >  
> > > > -	return bufferAllocator_->allocate(stream);
> > > > +	int ret = bufferAllocator_->allocate(stream);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	for (unsigned int i = 0; i < count; i++) {
> > > > +		std::unique_ptr<Request> request = camera_->createRequest(i);
> > > > +		if (!request) {
> > > > +			requestPool_.clear();
> > > > +			return -ENOMEM;
> > > > +		}
> > > > +		requestPool_.push_back(std::move(request));
> > > > +	}
> > > > +
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  void V4L2Camera::freeBuffers()
> > > >  {
> > > >  	pendingRequests_.clear();
> > > > +	requestPool_.clear();
> > > >  
> > > >  	Stream *stream = config_->at(0).stream();
> > > >  	bufferAllocator_->free(stream);
> > > > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()
> > > >  
> > > >  	isRunning_ = true;
> > > >  
> > > > -	for (std::unique_ptr<Request> &req : pendingRequests_) {
> > > > +	for (Request *req : pendingRequests_) {
> > > >  		/* \todo What should we do if this returns -EINVAL? */
> > > > -		ret = camera_->queueRequest(req.release());
> > > > +		ret = camera_->queueRequest(req);
> > > >  		if (ret < 0)
> > > >  			return ret == -EACCES ? -EBUSY : ret;
> > > >  	}
> > > > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()
> > > >  
> > > >  int V4L2Camera::qbuf(unsigned int index)
> > > >  {
> > > > -	std::unique_ptr<Request> request =
> > > > -		std::unique_ptr<Request>(camera_->createRequest(index));
> > > > -	if (!request) {
> > > > -		LOG(V4L2Compat, Error) << "Can't create request";
> > > > -		return -ENOMEM;
> > > > +	if (index >= requestPool_.size()) {
> > > > +		LOG(V4L2Compat, Error) << "Invalid index";
> > > > +		return -EINVAL;
> > > >  	}
> > > > +	Request *request = requestPool_[index].get();
> > > > +	request->reset();
> > > >  
> > > >  	Stream *stream = config_->at(0).stream();
> > > >  	FrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();
> > > > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)
> > > >  	}
> > > >  
> > > >  	if (!isRunning_) {
> > > > -		pendingRequests_.push_back(std::move(request));
> > > > +		pendingRequests_.push_back(request);
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > -	ret = camera_->queueRequest(request.release());
> > > > +	ret = camera_->queueRequest(request);
> > > >  	if (ret < 0) {
> > > >  		LOG(V4L2Compat, Error) << "Can't queue request";
> > > >  		return ret == -EACCES ? -EBUSY : ret;
> > > > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h
> > > > index 1fc5ebef..a6c35a2e 100644
> > > > --- a/src/v4l2/v4l2_camera.h
> > > > +++ b/src/v4l2/v4l2_camera.h
> > > > @@ -76,7 +76,9 @@ private:
> > > >  	std::mutex bufferLock_;
> > > >  	FrameBufferAllocator *bufferAllocator_;
> > > >  
> > > > -	std::deque<std::unique_ptr<Request>> pendingRequests_;
> > > > +	std::vector<std::unique_ptr<Request>> requestPool_;
> > > > +
> > > > +	std::deque<Request *> pendingRequests_;
> > > >  	std::deque<std::unique_ptr<Buffer>> completedBuffers_;
> > > >  
> > > >  	int efd_;
> > > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp
> > > > index 64e96264..9cfdb388 100644
> > > > --- a/test/camera/buffer_import.cpp
> > > > +++ b/test/camera/buffer_import.cpp
> > > > @@ -50,7 +50,7 @@ protected:
> > > >  		if (request->status() != Request::RequestComplete)
> > > >  			return;
> > > >  
> > > > -		const Request::BufferMap &buffers = request->buffers();
> > > > +		const Request::BufferMap buffers = request->buffers();
> > > >  
> > > >  		completeRequestsCount_++;
> > > >  
> > > > @@ -58,7 +58,7 @@ protected:
> > > >  		const Stream *stream = buffers.begin()->first;
> > > >  		FrameBuffer *buffer = buffers.begin()->second;
> > > >  
> > > > -		request = camera_->createRequest();
> > > > +		request->reset();
> > > >  		request->addBuffer(stream, buffer);
> > > >  		camera_->queueRequest(request);
> > > >  	}
> > > > @@ -98,9 +98,8 @@ protected:
> > > >  		if (ret != TestPass)
> > > >  			return ret;
> > > >  
> > > > -		std::vector<Request *> requests;
> > > >  		for (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {
> > > > -			Request *request = camera_->createRequest();
> > > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > > >  			if (!request) {
> > > >  				std::cout << "Failed to create request" << std::endl;
> > > >  				return TestFail;
> > > > @@ -111,7 +110,7 @@ protected:
> > > >  				return TestFail;
> > > >  			}
> > > >  
> > > > -			requests.push_back(request);
> > > > +			requests_.push_back(std::move(request));
> > > >  		}
> > > >  
> > > >  		completeRequestsCount_ = 0;
> > > > @@ -125,8 +124,8 @@ protected:
> > > >  			return TestFail;
> > > >  		}
> > > >  
> > > > -		for (Request *request : requests) {
> > > > -			if (camera_->queueRequest(request)) {
> > > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > > +			if (camera_->queueRequest(request.get())) {
> > > >  				std::cout << "Failed to queue request" << std::endl;
> > > >  				return TestFail;
> > > >  			}
> > > > @@ -160,6 +159,8 @@ protected:
> > > >  	}
> > > >  
> > > >  private:
> > > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > > +
> > > >  	unsigned int completeBuffersCount_;
> > > >  	unsigned int completeRequestsCount_;
> > > >  	std::unique_ptr<CameraConfiguration> config_;
> > > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > > > index 51bbd258..5c86c490 100644
> > > > --- a/test/camera/capture.cpp
> > > > +++ b/test/camera/capture.cpp
> > > > @@ -44,7 +44,7 @@ protected:
> > > >  		if (request->status() != Request::RequestComplete)
> > > >  			return;
> > > >  
> > > > -		const Request::BufferMap &buffers = request->buffers();
> > > > +		const Request::BufferMap buffers = request->buffers();
> > > >  
> > > >  		completeRequestsCount_++;
> > > >  
> > > > @@ -52,7 +52,7 @@ protected:
> > > >  		const Stream *stream = buffers.begin()->first;
> > > >  		FrameBuffer *buffer = buffers.begin()->second;
> > > >  
> > > > -		request = camera_->createRequest();
> > > > +		request->reset();
> > > >  		request->addBuffer(stream, buffer);
> > > >  		camera_->queueRequest(request);
> > > >  	}
> > > > @@ -98,9 +98,8 @@ protected:
> > > >  		if (ret < 0)
> > > >  			return TestFail;
> > > >  
> > > > -		std::vector<Request *> requests;
> > > >  		for (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {
> > > > -			Request *request = camera_->createRequest();
> > > > +			std::unique_ptr<Request> request = camera_->createRequest();
> > > >  			if (!request) {
> > > >  				cout << "Failed to create request" << endl;
> > > >  				return TestFail;
> > > > @@ -111,7 +110,7 @@ protected:
> > > >  				return TestFail;
> > > >  			}
> > > >  
> > > > -			requests.push_back(request);
> > > > +			requests_.push_back(std::move(request));
> > > >  		}
> > > >  
> > > >  		completeRequestsCount_ = 0;
> > > > @@ -125,8 +124,8 @@ protected:
> > > >  			return TestFail;
> > > >  		}
> > > >  
> > > > -		for (Request *request : requests) {
> > > > -			if (camera_->queueRequest(request)) {
> > > > +		for (std::unique_ptr<Request> &request : requests_) {
> > > > +			if (camera_->queueRequest(request.get())) {
> > > >  				cout << "Failed to queue request" << endl;
> > > >  				return TestFail;
> > > >  			}
> > > > @@ -161,6 +160,8 @@ protected:
> > > >  		return TestPass;
> > > >  	}
> > > >  
> > > > +	std::vector<std::unique_ptr<Request>> requests_;
> > > > +
> > > >  	std::unique_ptr<CameraConfiguration> config_;
> > > >  	FrameBufferAllocator *allocator_;
> > > >  };
> > > > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > > > index 28faeb91..e63ab298 100644
> > > > --- a/test/camera/statemachine.cpp
> > > > +++ b/test/camera/statemachine.cpp
> > > > @@ -101,13 +101,10 @@ protected:
> > > >  			return TestFail;
> > > >  
> > > >  		/* Test operations which should pass. */
> > > > -		Request *request2 = camera_->createRequest();
> > > > +		std::unique_ptr<Request> request2 = camera_->createRequest();
> > > >  		if (!request2)
> > > >  			return TestFail;
> > > >  
> > > > -		/* Never handed to hardware so need to manually delete it. */
> > > > -		delete request2;
> > > > -
> > > >  		/* Test valid state transitions, end in Running state. */
> > > >  		if (camera_->release())
> > > >  			return TestFail;
> > > > @@ -146,7 +143,7 @@ protected:
> > > >  			return TestFail;
> > > >  
> > > >  		/* Test operations which should pass. */
> > > > -		Request *request = camera_->createRequest();
> > > > +		std::unique_ptr<Request> request = camera_->createRequest();
> > > >  		if (!request)
> > > >  			return TestFail;
> > > >  
> > > > @@ -154,7 +151,7 @@ protected:
> > > >  		if (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))
> > > >  			return TestFail;
> > > >  
> > > > -		if (camera_->queueRequest(request))
> > > > +		if (camera_->queueRequest(request.get()))
> > > >  			return TestFail;
> > > >  
> > > >  		/* Test valid state transitions, end in Available state. */
> > > > -- 
> > > > 2.27.0
> > > > 
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel@lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel