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

9908 diff mbox series

Allow reuse of the Request object by implementing reset(). This means
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 v4:
- no more implicit calls of anything that we added in this patch
- make reset() take a reuseBuffers boolean parameters
  - use transient request - delete request
  - reuse request, reset buffers - reset()
  - reuse request, reuse buffesr - reset(true)
- update apps and tests and documentation accordingly

Changes in v3:
- reset() is called in Camera::queueRequest()
- apps that use transient request (android, gstreamer) delete the
  request at the end of the completion handler
- apps that want to reuse the buffers (cam) use reAddBuffers()
- apps that need to change the buffers (qcam, v4l2) use clearBuffers()
- update the documentation accordingly

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
---
 include/libcamera/camera.h        |  2 +-
 include/libcamera/request.h       |  2 ++
 src/android/camera_device.cpp     | 14 +++++++---
 src/cam/capture.cpp               | 29 +++++--------------
 src/cam/capture.h                 |  3 ++
 src/gstreamer/gstlibcamerasrc.cpp | 14 ++++++----
 src/libcamera/camera.cpp          | 14 ++++------
 src/libcamera/request.cpp         | 35 +++++++++++++++++++++++
 src/qcam/main_window.cpp          | 46 +++++++++++++++++++------------
 src/qcam/main_window.h            | 26 +++++------------
 src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++--------
 src/v4l2/v4l2_camera.h            |  4 ++-
 test/camera/buffer_import.cpp     | 13 +++++----
 test/camera/capture.cpp           | 13 +++++----
 test/camera/statemachine.cpp      |  9 ++----
 15 files changed, 154 insertions(+), 108 deletions(-)

Hi Paul,

Thank you for the patch.

On Fri, Oct 02, 2020 at 09:20:43PM +0900, Paul Elder wrote:
> Allow reuse of the Request object by implementing reset(). This means
> 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 v4:
> - no more implicit calls of anything that we added in this patch
> - make reset() take a reuseBuffers boolean parameters
>   - use transient request - delete request
>   - reuse request, reset buffers - reset()
>   - reuse request, reuse buffesr - reset(true)
> - update apps and tests and documentation accordingly
> 
> Changes in v3:
> - reset() is called in Camera::queueRequest()
> - apps that use transient request (android, gstreamer) delete the
>   request at the end of the completion handler
> - apps that want to reuse the buffers (cam) use reAddBuffers()
> - apps that need to change the buffers (qcam, v4l2) use clearBuffers()
> - update the documentation accordingly
> 
> 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
> ---
>  include/libcamera/camera.h        |  2 +-
>  include/libcamera/request.h       |  2 ++
>  src/android/camera_device.cpp     | 14 +++++++---
>  src/cam/capture.cpp               | 29 +++++--------------
>  src/cam/capture.h                 |  3 ++
>  src/gstreamer/gstlibcamerasrc.cpp | 14 ++++++----
>  src/libcamera/camera.cpp          | 14 ++++------
>  src/libcamera/request.cpp         | 35 +++++++++++++++++++++++
>  src/qcam/main_window.cpp          | 46 +++++++++++++++++++------------
>  src/qcam/main_window.h            | 26 +++++------------
>  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++--------
>  src/v4l2/v4l2_camera.h            |  4 ++-
>  test/camera/buffer_import.cpp     | 13 +++++----
>  test/camera/capture.cpp           | 13 +++++----
>  test/camera/statemachine.cpp      |  9 ++----
>  15 files changed, 154 insertions(+), 108 deletions(-)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a2ee4e7e..79ff8d6b 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -96,7 +96,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);
>  	int queueRequest(Request *request);
>  
>  	int start();
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 5976ac50..70d06933 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -38,6 +38,8 @@ public:
>  	Request &operator=(const Request &) = delete;
>  	~Request();
>  
> +	void reset(bool reuseBuffers = false);
> +
>  	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 751699cd..052c9292 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1395,8 +1395,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		new Camera3RequestDescriptor(camera3Request->frame_number,
>  					     camera3Request->num_output_buffers);
>  
> -	Request *request =
> +	std::unique_ptr<Request> request =
>  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> +	if (!request) {
> +		LOG(HAL, Error) << "Failed to create request";
> +		return -ENOMEM;
> +	}
>  
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
>  		CameraStream *cameraStream =
> @@ -1422,7 +1426,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
>  		if (!buffer) {
>  			LOG(HAL, Error) << "Failed to create buffer";
> -			delete request;
>  			delete descriptor;
>  			return -ENOMEM;
>  		}
> @@ -1434,14 +1437,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  		request->addBuffer(stream, buffer);
>  	}
>  
> -	int ret = camera_->queueRequest(request);
> +	int ret = camera_->queueRequest(request.get());
>  	if (ret) {
>  		LOG(HAL, Error) << "Failed to queue request";
> -		delete request;
>  		delete descriptor;
>  		return ret;
>  	}
>  
> +	/* The request will be deleted in the completion handler. */
> +	request.release();
> +
>  	return 0;
>  }
>  
> @@ -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..e6aaf79f 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());
>  		if (ret < 0) {
>  			std::cerr << "Can't queue request" << std::endl;
>  			camera_->stop();
> @@ -202,22 +203,6 @@ void Capture::requestComplete(Request *request)
>  		return;
>  	}
>  
> -	/*
> -	 * 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;
> -	}
> -
> -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> -		const Stream *stream = it->first;
> -		FrameBuffer *buffer = it->second;
> -
> -		request->addBuffer(stream, buffer);
> -	}
> -
> +	request->reset(true);
>  	camera_->queueRequest(request);
>  }
> 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..f25b6420 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,8 +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();
> -	auto wrap = std::make_unique<RequestWrap>(request);
> +	std::unique_ptr<Request> request = state->cam_->createRequest();
> +	auto wrap = std::make_unique<RequestWrap>(request.get());
>  	for (GstPad *srcpad : state->srcpads_) {
>  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
>  		GstBuffer *buffer;
> @@ -280,8 +282,7 @@ gst_libcamera_src_task_run(gpointer user_data)
>  			 * RequestWrap does not take ownership, and we won't be
>  			 * queueing this one due to lack of buffers.
>  			 */
> -			delete request;
> -			request = nullptr;
> +			request.reset(nullptr);

nullptr is the default, so you could write

			request.reset();

>  			break;
>  		}
>  
> @@ -291,8 +292,11 @@ gst_libcamera_src_task_run(gpointer user_data)
>  	if (request) {
>  		GLibLocker lock(GST_OBJECT(self));
>  		GST_TRACE_OBJECT(self, "Requesting buffers");
> -		state->cam_->queueRequest(request);
> +		state->cam_->queueRequest(request.get());
>  		state->requests_.push(std::move(wrap));
> +
> +		/* The request will be deleted in the completion handler. */
> +		request.release();
>  	}
>  
>  	GstFlowReturn ret = GST_FLOW_OK;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index fb76077f..2639a415 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -847,21 +847,22 @@ 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 deleting it. The request may be deleted in the completion
> + * handler, or reused after clearing its buffers with Request::clearBuffers().

There's no clearBuffers() anymore :-)

>   *
>   * \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);
>  }
>  
>  /**
> @@ -877,9 +878,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.
>   *
> @@ -988,13 +986,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..f19995d3 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -85,6 +85,41 @@ Request::~Request()
>  	delete validator_;
>  }
>  
> +/*
> + * \brief Reset the request
> + * \param[in] reuseBuffers Flag to indicate whether or not to reuse the buffers
> + *
> + * Reset the status and controls associated with the request, to allow it to
> + * be reused and requeued without destruction. This function should be called
> + * prior to queueing the request to the camera, in lieu of constructing a new
> + * request. If the application wishes to reuse the buffers that were previously
> + * added to the request via addBuffer(), then \a reuseBuffers should be set to
> + * true.
> + */
> +void Request::reset(bool reuseBuffers)

Boolean parameters are not very nice when their usage isn't implied by
the function name. When reading code,

	request->reset(true);

isn't very explicit. An enum parameter would be better:

	request->reset(Request::ReuseBuffers);

I even wonder if we should call the function Request::reuse() instead of
reset(). With requests stored in unique_ptr,

	std::unique_ptr<Request> request = ...;

	request->reset();
	request.reset();

could lead to confusion.

> +{
> +	pending_.clear();
> +	if (reuseBuffers) {
> +		for (auto pair : bufferMap_) {
> +			pair.second->request_ = this;
> +			pending_.insert(pair.second);
> +		}
> +	} else {
> +		bufferMap_.clear();
> +	}
> +
> +	status_ = RequestPending;
> +	cancelled_ = false;
> +
> +	delete metadata_;
> +	delete controls_;
> +	delete validator_;
> +
> +	validator_ = new CameraControlValidator(camera_);

Do we need to recreate the validator, as camera_ is the same ?

> +	controls_ = new ControlList(controls::controls, validator_);
> +	metadata_ = new ControlList(controls::controls);

Maybe just

	controls_.clear();
	metadata_.clear();

instead of deleting them ?

> +}
> +
>  /**
>   * \fn Request::controls()
>   * \brief Retrieve the request's ControlList
> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> index ecb9dd66..5192cbb8 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())
> @@ -724,12 +723,19 @@ void MainWindow::processCapture()
>  		request = doneQueue_.dequeue();
>  	}
>  
> +	Request::BufferMap buffers = request->buffers();

Do you need to copy the map, can't it be a reference ?

> +
>  	/* Process buffers. */
> -	if (request.buffers_.count(vfStream_))
> -		processViewfinder(request.buffers_[vfStream_]);
> +	if (request->buffers().count(vfStream_))
> +		processViewfinder(buffers[vfStream_]);
>  
> -	if (request.buffers_.count(rawStream_))
> -		processRaw(request.buffers_[rawStream_], request.metadata_);
> +	if (request->buffers().count(rawStream_))
> +		processRaw(buffers[rawStream_], request->metadata());
> +
> +	{
> +		QMutexLocker locker(&mutex_);
> +		freeQueue_.enqueue(request);
> +	}

You don't need a specific scope for this,

	QMutexLocker locker(&mutex_);
	freeQueue_.enqueue(request);

will do as it's at the end of the function.

>  }
>  
>  void MainWindow::processViewfinder(FrameBuffer *buffer)
> @@ -754,12 +760,16 @@ 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())
> +			qWarning() << "No free request";
> +
> +		request = freeQueue_.dequeue();
>  	}
>  
> +	request->reset();

Could this be moved just before freeQueue_.enqueue(request) (to be
precise before creating the mutex locker) ?

>  	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();

Can reset() be moved at the end of V4L2Camera::requestComplete() ?

>  
>  	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..ea9fbc78 100644
> --- a/test/camera/buffer_import.cpp
> +++ b/test/camera/buffer_import.cpp
> @@ -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..ffe29cb9 100644
> --- a/test/camera/capture.cpp
> +++ b/test/camera/capture.cpp
> @@ -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. */

Hi Laurent,

Thank you for the review.

On Fri, Oct 02, 2020 at 04:03:45PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Oct 02, 2020 at 09:20:43PM +0900, Paul Elder wrote:
> > Allow reuse of the Request object by implementing reset(). This means
> > 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 v4:
> > - no more implicit calls of anything that we added in this patch
> > - make reset() take a reuseBuffers boolean parameters
> >   - use transient request - delete request
> >   - reuse request, reset buffers - reset()
> >   - reuse request, reuse buffesr - reset(true)
> > - update apps and tests and documentation accordingly
> > 
> > Changes in v3:
> > - reset() is called in Camera::queueRequest()
> > - apps that use transient request (android, gstreamer) delete the
> >   request at the end of the completion handler
> > - apps that want to reuse the buffers (cam) use reAddBuffers()
> > - apps that need to change the buffers (qcam, v4l2) use clearBuffers()
> > - update the documentation accordingly
> > 
> > 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
> > ---
> >  include/libcamera/camera.h        |  2 +-
> >  include/libcamera/request.h       |  2 ++
> >  src/android/camera_device.cpp     | 14 +++++++---
> >  src/cam/capture.cpp               | 29 +++++--------------
> >  src/cam/capture.h                 |  3 ++
> >  src/gstreamer/gstlibcamerasrc.cpp | 14 ++++++----
> >  src/libcamera/camera.cpp          | 14 ++++------
> >  src/libcamera/request.cpp         | 35 +++++++++++++++++++++++
> >  src/qcam/main_window.cpp          | 46 +++++++++++++++++++------------
> >  src/qcam/main_window.h            | 26 +++++------------
> >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++--------
> >  src/v4l2/v4l2_camera.h            |  4 ++-
> >  test/camera/buffer_import.cpp     | 13 +++++----
> >  test/camera/capture.cpp           | 13 +++++----
> >  test/camera/statemachine.cpp      |  9 ++----
> >  15 files changed, 154 insertions(+), 108 deletions(-)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index a2ee4e7e..79ff8d6b 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -96,7 +96,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);
> >  	int queueRequest(Request *request);
> >  
> >  	int start();
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 5976ac50..70d06933 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -38,6 +38,8 @@ public:
> >  	Request &operator=(const Request &) = delete;
> >  	~Request();
> >  
> > +	void reset(bool reuseBuffers = false);
> > +
> >  	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 751699cd..052c9292 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1395,8 +1395,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> >  					     camera3Request->num_output_buffers);
> >  
> > -	Request *request =
> > +	std::unique_ptr<Request> request =
> >  		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > +	if (!request) {
> > +		LOG(HAL, Error) << "Failed to create request";
> > +		return -ENOMEM;
> > +	}
> >  
> >  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> >  		CameraStream *cameraStream =
> > @@ -1422,7 +1426,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		FrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);
> >  		if (!buffer) {
> >  			LOG(HAL, Error) << "Failed to create buffer";
> > -			delete request;
> >  			delete descriptor;
> >  			return -ENOMEM;
> >  		}
> > @@ -1434,14 +1437,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		request->addBuffer(stream, buffer);
> >  	}
> >  
> > -	int ret = camera_->queueRequest(request);
> > +	int ret = camera_->queueRequest(request.get());
> >  	if (ret) {
> >  		LOG(HAL, Error) << "Failed to queue request";
> > -		delete request;
> >  		delete descriptor;
> >  		return ret;
> >  	}
> >  
> > +	/* The request will be deleted in the completion handler. */
> > +	request.release();
> > +
> >  	return 0;
> >  }
> >  
> > @@ -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..e6aaf79f 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());
> >  		if (ret < 0) {
> >  			std::cerr << "Can't queue request" << std::endl;
> >  			camera_->stop();
> > @@ -202,22 +203,6 @@ void Capture::requestComplete(Request *request)
> >  		return;
> >  	}
> >  
> > -	/*
> > -	 * 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;
> > -	}
> > -
> > -	for (auto it = buffers.begin(); it != buffers.end(); ++it) {
> > -		const Stream *stream = it->first;
> > -		FrameBuffer *buffer = it->second;
> > -
> > -		request->addBuffer(stream, buffer);
> > -	}
> > -
> > +	request->reset(true);
> >  	camera_->queueRequest(request);
> >  }
> > 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..f25b6420 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,8 +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();
> > -	auto wrap = std::make_unique<RequestWrap>(request);
> > +	std::unique_ptr<Request> request = state->cam_->createRequest();
> > +	auto wrap = std::make_unique<RequestWrap>(request.get());
> >  	for (GstPad *srcpad : state->srcpads_) {
> >  		GstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);
> >  		GstBuffer *buffer;
> > @@ -280,8 +282,7 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  			 * RequestWrap does not take ownership, and we won't be
> >  			 * queueing this one due to lack of buffers.
> >  			 */
> > -			delete request;
> > -			request = nullptr;
> > +			request.reset(nullptr);
> 
> nullptr is the default, so you could write
> 
> 			request.reset();

ack

> >  			break;
> >  		}
> >  
> > @@ -291,8 +292,11 @@ gst_libcamera_src_task_run(gpointer user_data)
> >  	if (request) {
> >  		GLibLocker lock(GST_OBJECT(self));
> >  		GST_TRACE_OBJECT(self, "Requesting buffers");
> > -		state->cam_->queueRequest(request);
> > +		state->cam_->queueRequest(request.get());
> >  		state->requests_.push(std::move(wrap));
> > +
> > +		/* The request will be deleted in the completion handler. */
> > +		request.release();
> >  	}
> >  
> >  	GstFlowReturn ret = GST_FLOW_OK;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index fb76077f..2639a415 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -847,21 +847,22 @@ 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 deleting it. The request may be deleted in the completion
> > + * handler, or reused after clearing its buffers with Request::clearBuffers().
> 
> There's no clearBuffers() anymore :-)

Oops :p

> >   *
> >   * \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);
> >  }
> >  
> >  /**
> > @@ -877,9 +878,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.
> >   *
> > @@ -988,13 +986,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..f19995d3 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -85,6 +85,41 @@ Request::~Request()
> >  	delete validator_;
> >  }
> >  
> > +/*
> > + * \brief Reset the request
> > + * \param[in] reuseBuffers Flag to indicate whether or not to reuse the buffers
> > + *
> > + * Reset the status and controls associated with the request, to allow it to
> > + * be reused and requeued without destruction. This function should be called
> > + * prior to queueing the request to the camera, in lieu of constructing a new
> > + * request. If the application wishes to reuse the buffers that were previously
> > + * added to the request via addBuffer(), then \a reuseBuffers should be set to
> > + * true.
> > + */
> > +void Request::reset(bool reuseBuffers)
> 
> Boolean parameters are not very nice when their usage isn't implied by
> the function name. When reading code,
> 
> 	request->reset(true);
> 
> isn't very explicit. An enum parameter would be better:
> 
> 	request->reset(Request::ReuseBuffers);

Ah, I see.

> I even wonder if we should call the function Request::reuse() instead of
> reset(). With requests stored in unique_ptr,
> 
> 	std::unique_ptr<Request> request = ...;
> 
> 	request->reset();
> 	request.reset();
> 
> could lead to confusion.

Ooh... that's true. I think reset is a better verb, but to avoid that
confusion maybe reuse would be fine.

> > +{
> > +	pending_.clear();
> > +	if (reuseBuffers) {
> > +		for (auto pair : bufferMap_) {
> > +			pair.second->request_ = this;
> > +			pending_.insert(pair.second);
> > +		}
> > +	} else {
> > +		bufferMap_.clear();
> > +	}
> > +
> > +	status_ = RequestPending;
> > +	cancelled_ = false;
> > +
> > +	delete metadata_;
> > +	delete controls_;
> > +	delete validator_;
> > +
> > +	validator_ = new CameraControlValidator(camera_);
> 
> Do we need to recreate the validator, as camera_ is the same ?
> 
> > +	controls_ = new ControlList(controls::controls, validator_);
> > +	metadata_ = new ControlList(controls::controls);
> 
> Maybe just
> 
> 	controls_.clear();
> 	metadata_.clear();
> 
> instead of deleting them ?

Oh, okay. I didn't realize that was good enough.

> > +}
> > +
> >  /**
> >   * \fn Request::controls()
> >   * \brief Retrieve the request's ControlList
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index ecb9dd66..5192cbb8 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())
> > @@ -724,12 +723,19 @@ void MainWindow::processCapture()
> >  		request = doneQueue_.dequeue();
> >  	}
> >  
> > +	Request::BufferMap buffers = request->buffers();
> 
> Do you need to copy the map, can't it be a reference ?

Here, no, because processViewfinder and processRaw both want non-const
but the reference returned by request->buffers() is const.

> > +
> >  	/* Process buffers. */
> > -	if (request.buffers_.count(vfStream_))
> > -		processViewfinder(request.buffers_[vfStream_]);
> > +	if (request->buffers().count(vfStream_))
> > +		processViewfinder(buffers[vfStream_]);
> >  
> > -	if (request.buffers_.count(rawStream_))
> > -		processRaw(request.buffers_[rawStream_], request.metadata_);
> > +	if (request->buffers().count(rawStream_))
> > +		processRaw(buffers[rawStream_], request->metadata());
> > +
> > +	{
> > +		QMutexLocker locker(&mutex_);
> > +		freeQueue_.enqueue(request);
> > +	}
> 
> You don't need a specific scope for this,
> 
> 	QMutexLocker locker(&mutex_);
> 	freeQueue_.enqueue(request);
> 
> will do as it's at the end of the function.

ack

> >  }
> >  
> >  void MainWindow::processViewfinder(FrameBuffer *buffer)
> > @@ -754,12 +760,16 @@ 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())
> > +			qWarning() << "No free request";
> > +
> > +		request = freeQueue_.dequeue();
> >  	}
> >  
> > +	request->reset();
> 
> Could this be moved just before freeQueue_.enqueue(request) (to be
> precise before creating the mutex locker) ?

ack

> >  	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();
> 
> Can reset() be moved at the end of V4L2Camera::requestComplete() ?

I think so. I was thinking that reset() replaces createRequest,
therefore it should be here instead.

> >  
> >  	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..ea9fbc78 100644
> > --- a/test/camera/buffer_import.cpp
> > +++ b/test/camera/buffer_import.cpp
> > @@ -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..ffe29cb9 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -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. */


Thanks,

Paul