[{"id":12938,"web_url":"https://patchwork.libcamera.org/comment/12938/","msgid":"<20201002130345.GB3933@pendragon.ideasonboard.com>","date":"2020-10-02T13:03:45","subject":"Re: [libcamera-devel] [PATCH v4] libcamera, android, cam, gstreamer,\n\tqcam, v4l2: Reuse Request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Fri, Oct 02, 2020 at 09:20:43PM +0900, Paul Elder wrote:\n> Allow reuse of the Request object by implementing reset(). This means\n> the applications now have the responsibility of freeing the Request\n> objects, so make all libcamera users (cam, qcam, v4l2-compat, gstreamer,\n> android) do so.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> \n> ---\n> Changes in v4:\n> - no more implicit calls of anything that we added in this patch\n> - make reset() take a reuseBuffers boolean parameters\n>   - use transient request - delete request\n>   - reuse request, reset buffers - reset()\n>   - reuse request, reuse buffesr - reset(true)\n> - update apps and tests and documentation accordingly\n> \n> Changes in v3:\n> - reset() is called in Camera::queueRequest()\n> - apps that use transient request (android, gstreamer) delete the\n>   request at the end of the completion handler\n> - apps that want to reuse the buffers (cam) use reAddBuffers()\n> - apps that need to change the buffers (qcam, v4l2) use clearBuffers()\n> - update the documentation accordingly\n> \n> Changes in v2:\n> - clear controls_ and metadata_ and validator_ in Request::reset()\n> - use unique_ptr on application side, prior to queueRequest, and use\n>   regular pointer for completion handler\n> - make qcam's reuse request nicer\n> - update Camera::queueRequest() and Camera::createRequest() documentation\n> - add documentation for Request::reset()\n> - make v4l2-compat reuse request\n> - make gstreamer and android use the new createRequest API, though they\n>   do not actually reuse the requests\n> ---\n>  include/libcamera/camera.h        |  2 +-\n>  include/libcamera/request.h       |  2 ++\n>  src/android/camera_device.cpp     | 14 +++++++---\n>  src/cam/capture.cpp               | 29 +++++--------------\n>  src/cam/capture.h                 |  3 ++\n>  src/gstreamer/gstlibcamerasrc.cpp | 14 ++++++----\n>  src/libcamera/camera.cpp          | 14 ++++------\n>  src/libcamera/request.cpp         | 35 +++++++++++++++++++++++\n>  src/qcam/main_window.cpp          | 46 +++++++++++++++++++------------\n>  src/qcam/main_window.h            | 26 +++++------------\n>  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++--------\n>  src/v4l2/v4l2_camera.h            |  4 ++-\n>  test/camera/buffer_import.cpp     | 13 +++++----\n>  test/camera/capture.cpp           | 13 +++++----\n>  test/camera/statemachine.cpp      |  9 ++----\n>  15 files changed, 154 insertions(+), 108 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index a2ee4e7e..79ff8d6b 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -96,7 +96,7 @@ public:\n>  \tstd::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});\n>  \tint configure(CameraConfiguration *config);\n>  \n> -\tRequest *createRequest(uint64_t cookie = 0);\n> +\tstd::unique_ptr<Request> createRequest(uint64_t cookie = 0);\n>  \tint queueRequest(Request *request);\n>  \n>  \tint start();\n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 5976ac50..70d06933 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -38,6 +38,8 @@ public:\n>  \tRequest &operator=(const Request &) = delete;\n>  \t~Request();\n>  \n> +\tvoid reset(bool reuseBuffers = false);\n> +\n>  \tControlList &controls() { return *controls_; }\n>  \tControlList &metadata() { return *metadata_; }\n>  \tconst BufferMap &buffers() const { return bufferMap_; }\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 751699cd..052c9292 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1395,8 +1395,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\tnew Camera3RequestDescriptor(camera3Request->frame_number,\n>  \t\t\t\t\t     camera3Request->num_output_buffers);\n>  \n> -\tRequest *request =\n> +\tstd::unique_ptr<Request> request =\n>  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n> +\tif (!request) {\n> +\t\tLOG(HAL, Error) << \"Failed to create request\";\n> +\t\treturn -ENOMEM;\n> +\t}\n>  \n>  \tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n>  \t\tCameraStream *cameraStream =\n> @@ -1422,7 +1426,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\tFrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n>  \t\tif (!buffer) {\n>  \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> -\t\t\tdelete request;\n>  \t\t\tdelete descriptor;\n>  \t\t\treturn -ENOMEM;\n>  \t\t}\n> @@ -1434,14 +1437,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>  \t\trequest->addBuffer(stream, buffer);\n>  \t}\n>  \n> -\tint ret = camera_->queueRequest(request);\n> +\tint ret = camera_->queueRequest(request.get());\n>  \tif (ret) {\n>  \t\tLOG(HAL, Error) << \"Failed to queue request\";\n> -\t\tdelete request;\n>  \t\tdelete descriptor;\n>  \t\treturn ret;\n>  \t}\n>  \n> +\t/* The request will be deleted in the completion handler. */\n> +\trequest.release();\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)\n>  \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>  \n>  \tdelete descriptor;\n> +\tdelete request;\n>  }\n>  \n>  std::string CameraDevice::logPrefix() const\n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 5510c009..e6aaf79f 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)\n>  \t\twriter_ = nullptr;\n>  \t}\n>  \n> +\trequests_.clear();\n> +\n>  \tdelete allocator;\n>  \n>  \treturn ret;\n> @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)\n>  \t * example pushing a button. For now run all streams all the time.\n>  \t */\n>  \n> -\tstd::vector<Request *> requests;\n>  \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> -\t\tRequest *request = camera_->createRequest();\n> +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n>  \t\tif (!request) {\n>  \t\t\tstd::cerr << \"Can't create request\" << std::endl;\n>  \t\t\treturn -ENOMEM;\n> @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n>  \t\t\t\twriter_->mapBuffer(buffer.get());\n>  \t\t}\n>  \n> -\t\trequests.push_back(request);\n> +\t\trequests_.push_back(std::move(request));\n>  \t}\n>  \n>  \tret = camera_->start();\n> @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)\n>  \t\treturn ret;\n>  \t}\n>  \n> -\tfor (Request *request : requests) {\n> -\t\tret = camera_->queueRequest(request);\n> +\tfor (std::unique_ptr<Request> &request : requests_) {\n> +\t\tret = camera_->queueRequest(request.get());\n>  \t\tif (ret < 0) {\n>  \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n>  \t\t\tcamera_->stop();\n> @@ -202,22 +203,6 @@ void Capture::requestComplete(Request *request)\n>  \t\treturn;\n>  \t}\n>  \n> -\t/*\n> -\t * Create a new request and populate it with one buffer for each\n> -\t * stream.\n> -\t */\n> -\trequest = camera_->createRequest();\n> -\tif (!request) {\n> -\t\tstd::cerr << \"Can't create request\" << std::endl;\n> -\t\treturn;\n> -\t}\n> -\n> -\tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> -\t\tconst Stream *stream = it->first;\n> -\t\tFrameBuffer *buffer = it->second;\n> -\n> -\t\trequest->addBuffer(stream, buffer);\n> -\t}\n> -\n> +\trequest->reset(true);\n>  \tcamera_->queueRequest(request);\n>  }\n> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> index 0aebdac9..45e5e8a9 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -9,6 +9,7 @@\n>  \n>  #include <memory>\n>  #include <stdint.h>\n> +#include <vector>\n>  \n>  #include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n> @@ -43,6 +44,8 @@ private:\n>  \tEventLoop *loop_;\n>  \tunsigned int captureCount_;\n>  \tunsigned int captureLimit_;\n> +\n> +\tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n>  };\n>  \n>  #endif /* __CAM_CAPTURE_H__ */\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 1bfc2e2f..f25b6420 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()\n>  \t\tif (item.second)\n>  \t\t\tgst_buffer_unref(item.second);\n>  \t}\n> +\n> +\tdelete request_;\n>  }\n>  \n>  void RequestWrap::attachBuffer(GstBuffer *buffer)\n> @@ -266,8 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)\n>  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n>  \tGstLibcameraSrcState *state = self->state;\n>  \n> -\tRequest *request = state->cam_->createRequest();\n> -\tauto wrap = std::make_unique<RequestWrap>(request);\n> +\tstd::unique_ptr<Request> request = state->cam_->createRequest();\n> +\tauto wrap = std::make_unique<RequestWrap>(request.get());\n>  \tfor (GstPad *srcpad : state->srcpads_) {\n>  \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n>  \t\tGstBuffer *buffer;\n> @@ -280,8 +282,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n>  \t\t\t * RequestWrap does not take ownership, and we won't be\n>  \t\t\t * queueing this one due to lack of buffers.\n>  \t\t\t */\n> -\t\t\tdelete request;\n> -\t\t\trequest = nullptr;\n> +\t\t\trequest.reset(nullptr);\n\nnullptr is the default, so you could write\n\n\t\t\trequest.reset();\n\n>  \t\t\tbreak;\n>  \t\t}\n>  \n> @@ -291,8 +292,11 @@ gst_libcamera_src_task_run(gpointer user_data)\n>  \tif (request) {\n>  \t\tGLibLocker lock(GST_OBJECT(self));\n>  \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> -\t\tstate->cam_->queueRequest(request);\n> +\t\tstate->cam_->queueRequest(request.get());\n>  \t\tstate->requests_.push(std::move(wrap));\n> +\n> +\t\t/* The request will be deleted in the completion handler. */\n> +\t\trequest.release();\n>  \t}\n>  \n>  \tGstFlowReturn ret = GST_FLOW_OK;\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index fb76077f..2639a415 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -847,21 +847,22 @@ int Camera::configure(CameraConfiguration *config)\n>   * handler, and is completely opaque to libcamera.\n>   *\n>   * The ownership of the returned request is passed to the caller, which is\n> - * responsible for either queueing the request or deleting it.\n> + * responsible for deleting it. The request may be deleted in the completion\n> + * handler, or reused after clearing its buffers with Request::clearBuffers().\n\nThere's no clearBuffers() anymore :-)\n\n>   *\n>   * \\context This function is \\threadsafe. It may only be called when the camera\n>   * is in the Configured or Running state as defined in \\ref camera_operation.\n>   *\n>   * \\return A pointer to the newly created request, or nullptr on error\n>   */\n> -Request *Camera::createRequest(uint64_t cookie)\n> +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n>  {\n>  \tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n>  \t\t\t\t      Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn nullptr;\n>  \n> -\treturn new Request(this, cookie);\n> +\treturn std::make_unique<Request>(this, cookie);\n>  }\n>  \n>  /**\n> @@ -877,9 +878,6 @@ Request *Camera::createRequest(uint64_t cookie)\n>   * Once the request has been queued, the camera will notify its completion\n>   * through the \\ref requestCompleted signal.\n>   *\n> - * Ownership of the request is transferred to the camera. It will be deleted\n> - * automatically after it completes.\n> - *\n>   * \\context This function is \\threadsafe. It may only be called when the camera\n>   * is in the Running state as defined in \\ref camera_operation.\n>   *\n> @@ -988,13 +986,11 @@ int Camera::stop()\n>   * \\param[in] request The request that has completed\n>   *\n>   * This function is called by the pipeline handler to notify the camera that\n> - * the request has completed. It emits the requestCompleted signal and deletes\n> - * the request.\n> + * the request has completed. It emits the requestCompleted signal.\n>   */\n>  void Camera::requestComplete(Request *request)\n>  {\n>  \trequestCompleted.emit(request);\n> -\tdelete request;\n>  }\n>  \n>  } /* namespace libcamera */\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 60b30692..f19995d3 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -85,6 +85,41 @@ Request::~Request()\n>  \tdelete validator_;\n>  }\n>  \n> +/*\n> + * \\brief Reset the request\n> + * \\param[in] reuseBuffers Flag to indicate whether or not to reuse the buffers\n> + *\n> + * Reset the status and controls associated with the request, to allow it to\n> + * be reused and requeued without destruction. This function should be called\n> + * prior to queueing the request to the camera, in lieu of constructing a new\n> + * request. If the application wishes to reuse the buffers that were previously\n> + * added to the request via addBuffer(), then \\a reuseBuffers should be set to\n> + * true.\n> + */\n> +void Request::reset(bool reuseBuffers)\n\nBoolean parameters are not very nice when their usage isn't implied by\nthe function name. When reading code,\n\n\trequest->reset(true);\n\nisn't very explicit. An enum parameter would be better:\n\n\trequest->reset(Request::ReuseBuffers);\n\nI even wonder if we should call the function Request::reuse() instead of\nreset(). With requests stored in unique_ptr,\n\n\tstd::unique_ptr<Request> request = ...;\n\n\trequest->reset();\n\trequest.reset();\n\ncould lead to confusion.\n\n> +{\n> +\tpending_.clear();\n> +\tif (reuseBuffers) {\n> +\t\tfor (auto pair : bufferMap_) {\n> +\t\t\tpair.second->request_ = this;\n> +\t\t\tpending_.insert(pair.second);\n> +\t\t}\n> +\t} else {\n> +\t\tbufferMap_.clear();\n> +\t}\n> +\n> +\tstatus_ = RequestPending;\n> +\tcancelled_ = false;\n> +\n> +\tdelete metadata_;\n> +\tdelete controls_;\n> +\tdelete validator_;\n> +\n> +\tvalidator_ = new CameraControlValidator(camera_);\n\nDo we need to recreate the validator, as camera_ is the same ?\n\n> +\tcontrols_ = new ControlList(controls::controls, validator_);\n> +\tmetadata_ = new ControlList(controls::controls);\n\nMaybe just\n\n\tcontrols_.clear();\n\tmetadata_.clear();\n\ninstead of deleting them ?\n\n> +}\n> +\n>  /**\n>   * \\fn Request::controls()\n>   * \\brief Retrieve the request's ControlList\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index ecb9dd66..5192cbb8 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)\n>  int MainWindow::startCapture()\n>  {\n>  \tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> -\tstd::vector<Request *> requests;\n>  \tint ret;\n>  \n>  \t/* Verify roles are supported. */\n> @@ -486,7 +485,7 @@ int MainWindow::startCapture()\n>  \twhile (!freeBuffers_[vfStream_].isEmpty()) {\n>  \t\tFrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();\n>  \n> -\t\tRequest *request = camera_->createRequest();\n> +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n>  \t\tif (!request) {\n>  \t\t\tqWarning() << \"Can't create request\";\n>  \t\t\tret = -ENOMEM;\n> @@ -499,7 +498,7 @@ int MainWindow::startCapture()\n>  \t\t\tgoto error;\n>  \t\t}\n>  \n> -\t\trequests.push_back(request);\n> +\t\trequests_.push_back(std::move(request));\n>  \t}\n>  \n>  \t/* Start the title timer and the camera. */\n> @@ -518,8 +517,8 @@ int MainWindow::startCapture()\n>  \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>  \n>  \t/* Queue all requests. */\n> -\tfor (Request *request : requests) {\n> -\t\tret = camera_->queueRequest(request);\n> +\tfor (std::unique_ptr<Request> &request : requests_) {\n> +\t\tret = camera_->queueRequest(request.get());\n>  \t\tif (ret < 0) {\n>  \t\t\tqWarning() << \"Can't queue request\";\n>  \t\t\tgoto error_disconnect;\n> @@ -535,8 +534,7 @@ error_disconnect:\n>  \tcamera_->stop();\n>  \n>  error:\n> -\tfor (Request *request : requests)\n> -\t\tdelete request;\n> +\trequests_.clear();\n>  \n>  \tfor (auto &iter : mappedBuffers_) {\n>  \t\tconst MappedBuffer &buffer = iter.second;\n> @@ -580,6 +578,8 @@ void MainWindow::stopCapture()\n>  \t}\n>  \tmappedBuffers_.clear();\n>  \n> +\trequests_.clear();\n> +\n>  \tdelete allocator_;\n>  \n>  \tisCapturing_ = false;\n> @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)\n>  \t */\n>  \t{\n>  \t\tQMutexLocker locker(&mutex_);\n> -\t\tdoneQueue_.enqueue({ request->buffers(), request->metadata() });\n> +\t\tdoneQueue_.enqueue(request);\n>  \t}\n>  \n>  \tQCoreApplication::postEvent(this, new CaptureEvent);\n> @@ -714,8 +714,7 @@ void MainWindow::processCapture()\n>  \t * if stopCapture() has been called while a CaptureEvent was posted but\n>  \t * not processed yet. Return immediately in that case.\n>  \t */\n> -\tCaptureRequest request;\n> -\n> +\tRequest *request;\n>  \t{\n>  \t\tQMutexLocker locker(&mutex_);\n>  \t\tif (doneQueue_.isEmpty())\n> @@ -724,12 +723,19 @@ void MainWindow::processCapture()\n>  \t\trequest = doneQueue_.dequeue();\n>  \t}\n>  \n> +\tRequest::BufferMap buffers = request->buffers();\n\nDo you need to copy the map, can't it be a reference ?\n\n> +\n>  \t/* Process buffers. */\n> -\tif (request.buffers_.count(vfStream_))\n> -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> +\tif (request->buffers().count(vfStream_))\n> +\t\tprocessViewfinder(buffers[vfStream_]);\n>  \n> -\tif (request.buffers_.count(rawStream_))\n> -\t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> +\tif (request->buffers().count(rawStream_))\n> +\t\tprocessRaw(buffers[rawStream_], request->metadata());\n> +\n> +\t{\n> +\t\tQMutexLocker locker(&mutex_);\n> +\t\tfreeQueue_.enqueue(request);\n> +\t}\n\nYou don't need a specific scope for this,\n\n\tQMutexLocker locker(&mutex_);\n\tfreeQueue_.enqueue(request);\n\nwill do as it's at the end of the function.\n\n>  }\n>  \n>  void MainWindow::processViewfinder(FrameBuffer *buffer)\n> @@ -754,12 +760,16 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n>  \n>  void MainWindow::queueRequest(FrameBuffer *buffer)\n>  {\n> -\tRequest *request = camera_->createRequest();\n> -\tif (!request) {\n> -\t\tqWarning() << \"Can't create request\";\n> -\t\treturn;\n> +\tRequest *request;\n> +\t{\n> +\t\tQMutexLocker locker(&mutex_);\n> +\t\tif (freeQueue_.isEmpty())\n> +\t\t\tqWarning() << \"No free request\";\n> +\n> +\t\trequest = freeQueue_.dequeue();\n>  \t}\n>  \n> +\trequest->reset();\n\nCould this be moved just before freeQueue_.enqueue(request) (to be\nprecise before creating the mutex locker) ?\n\n>  \trequest->addBuffer(vfStream_, buffer);\n>  \n>  \tif (captureRaw_) {\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 5c61a4df..64bcfebc 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -8,6 +8,7 @@\n>  #define __QCAM_MAIN_WINDOW_H__\n>  \n>  #include <memory>\n> +#include <vector>\n>  \n>  #include <QElapsedTimer>\n>  #include <QIcon>\n> @@ -22,6 +23,7 @@\n>  #include <libcamera/camera_manager.h>\n>  #include <libcamera/controls.h>\n>  #include <libcamera/framebuffer_allocator.h>\n> +#include <libcamera/request.h>\n>  #include <libcamera/stream.h>\n>  \n>  #include \"../cam/stream_options.h\"\n> @@ -41,23 +43,6 @@ enum {\n>  \tOptStream = 's',\n>  };\n>  \n> -class CaptureRequest\n> -{\n> -public:\n> -\tCaptureRequest()\n> -\t{\n> -\t}\n> -\n> -\tCaptureRequest(const Request::BufferMap &buffers,\n> -\t\t       const ControlList &metadata)\n> -\t\t: buffers_(buffers), metadata_(metadata)\n> -\t{\n> -\t}\n> -\n> -\tRequest::BufferMap buffers_;\n> -\tControlList metadata_;\n> -};\n> -\n>  class MainWindow : public QMainWindow\n>  {\n>  \tQ_OBJECT\n> @@ -128,13 +113,16 @@ private:\n>  \tStream *vfStream_;\n>  \tStream *rawStream_;\n>  \tstd::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;\n> -\tQQueue<CaptureRequest> doneQueue_;\n> -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> +\tQQueue<Request *> doneQueue_;\n> +\tQQueue<Request *> freeQueue_;\n> +\tQMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */\n>  \n>  \tuint64_t lastBufferTime_;\n>  \tQElapsedTimer frameRateInterval_;\n>  \tuint32_t previousFrames_;\n>  \tuint32_t framesCaptured_;\n> +\n> +\tstd::vector<std::unique_ptr<Request>> requests_;\n>  };\n>  \n>  #endif /* __QCAM_MAIN_WINDOW__ */\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 3565f369..0d59502a 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)\n>  \n>  void V4L2Camera::close()\n>  {\n> +\trequestPool_.clear();\n> +\n>  \tdelete bufferAllocator_;\n>  \tbufferAllocator_ = nullptr;\n>  \n> @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,\n>  \treturn 0;\n>  }\n>  \n> -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)\n> +int V4L2Camera::allocBuffers(unsigned int count)\n>  {\n>  \tStream *stream = config_->at(0).stream();\n>  \n> -\treturn bufferAllocator_->allocate(stream);\n> +\tint ret = bufferAllocator_->allocate(stream);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\tfor (unsigned int i = 0; i < count; i++) {\n> +\t\tstd::unique_ptr<Request> request = camera_->createRequest(i);\n> +\t\tif (!request) {\n> +\t\t\trequestPool_.clear();\n> +\t\t\treturn -ENOMEM;\n> +\t\t}\n> +\t\trequestPool_.push_back(std::move(request));\n> +\t}\n> +\n> +\treturn ret;\n>  }\n>  \n>  void V4L2Camera::freeBuffers()\n>  {\n>  \tpendingRequests_.clear();\n> +\trequestPool_.clear();\n>  \n>  \tStream *stream = config_->at(0).stream();\n>  \tbufferAllocator_->free(stream);\n> @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()\n>  \n>  \tisRunning_ = true;\n>  \n> -\tfor (std::unique_ptr<Request> &req : pendingRequests_) {\n> +\tfor (Request *req : pendingRequests_) {\n>  \t\t/* \\todo What should we do if this returns -EINVAL? */\n> -\t\tret = camera_->queueRequest(req.release());\n> +\t\tret = camera_->queueRequest(req);\n>  \t\tif (ret < 0)\n>  \t\t\treturn ret == -EACCES ? -EBUSY : ret;\n>  \t}\n> @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()\n>  \n>  int V4L2Camera::qbuf(unsigned int index)\n>  {\n> -\tstd::unique_ptr<Request> request =\n> -\t\tstd::unique_ptr<Request>(camera_->createRequest(index));\n> -\tif (!request) {\n> -\t\tLOG(V4L2Compat, Error) << \"Can't create request\";\n> -\t\treturn -ENOMEM;\n> +\tif (index >= requestPool_.size()) {\n> +\t\tLOG(V4L2Compat, Error) << \"Invalid index\";\n> +\t\treturn -EINVAL;\n>  \t}\n> +\tRequest *request = requestPool_[index].get();\n> +\trequest->reset();\n\nCan reset() be moved at the end of V4L2Camera::requestComplete() ?\n\n>  \n>  \tStream *stream = config_->at(0).stream();\n>  \tFrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();\n> @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)\n>  \t}\n>  \n>  \tif (!isRunning_) {\n> -\t\tpendingRequests_.push_back(std::move(request));\n> +\t\tpendingRequests_.push_back(request);\n>  \t\treturn 0;\n>  \t}\n>  \n> -\tret = camera_->queueRequest(request.release());\n> +\tret = camera_->queueRequest(request);\n>  \tif (ret < 0) {\n>  \t\tLOG(V4L2Compat, Error) << \"Can't queue request\";\n>  \t\treturn ret == -EACCES ? -EBUSY : ret;\n> diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> index 1fc5ebef..a6c35a2e 100644\n> --- a/src/v4l2/v4l2_camera.h\n> +++ b/src/v4l2/v4l2_camera.h\n> @@ -76,7 +76,9 @@ private:\n>  \tstd::mutex bufferLock_;\n>  \tFrameBufferAllocator *bufferAllocator_;\n>  \n> -\tstd::deque<std::unique_ptr<Request>> pendingRequests_;\n> +\tstd::vector<std::unique_ptr<Request>> requestPool_;\n> +\n> +\tstd::deque<Request *> pendingRequests_;\n>  \tstd::deque<std::unique_ptr<Buffer>> completedBuffers_;\n>  \n>  \tint efd_;\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 64e96264..ea9fbc78 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -58,7 +58,7 @@ protected:\n>  \t\tconst Stream *stream = buffers.begin()->first;\n>  \t\tFrameBuffer *buffer = buffers.begin()->second;\n>  \n> -\t\trequest = camera_->createRequest();\n> +\t\trequest->reset();\n>  \t\trequest->addBuffer(stream, buffer);\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n> @@ -98,9 +98,8 @@ protected:\n>  \t\tif (ret != TestPass)\n>  \t\t\treturn ret;\n>  \n> -\t\tstd::vector<Request *> requests;\n>  \t\tfor (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {\n> -\t\t\tRequest *request = camera_->createRequest();\n> +\t\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n>  \t\t\tif (!request) {\n>  \t\t\t\tstd::cout << \"Failed to create request\" << std::endl;\n>  \t\t\t\treturn TestFail;\n> @@ -111,7 +110,7 @@ protected:\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n>  \n> -\t\t\trequests.push_back(request);\n> +\t\t\trequests_.push_back(std::move(request));\n>  \t\t}\n>  \n>  \t\tcompleteRequestsCount_ = 0;\n> @@ -125,8 +124,8 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tfor (Request *request : requests) {\n> -\t\t\tif (camera_->queueRequest(request)) {\n> +\t\tfor (std::unique_ptr<Request> &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n>  \t\t\t\tstd::cout << \"Failed to queue request\" << std::endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n> @@ -160,6 +159,8 @@ protected:\n>  \t}\n>  \n>  private:\n> +\tstd::vector<std::unique_ptr<Request>> requests_;\n> +\n>  \tunsigned int completeBuffersCount_;\n>  \tunsigned int completeRequestsCount_;\n>  \tstd::unique_ptr<CameraConfiguration> config_;\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 51bbd258..ffe29cb9 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -52,7 +52,7 @@ protected:\n>  \t\tconst Stream *stream = buffers.begin()->first;\n>  \t\tFrameBuffer *buffer = buffers.begin()->second;\n>  \n> -\t\trequest = camera_->createRequest();\n> +\t\trequest->reset();\n>  \t\trequest->addBuffer(stream, buffer);\n>  \t\tcamera_->queueRequest(request);\n>  \t}\n> @@ -98,9 +98,8 @@ protected:\n>  \t\tif (ret < 0)\n>  \t\t\treturn TestFail;\n>  \n> -\t\tstd::vector<Request *> requests;\n>  \t\tfor (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> -\t\t\tRequest *request = camera_->createRequest();\n> +\t\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n>  \t\t\tif (!request) {\n>  \t\t\t\tcout << \"Failed to create request\" << endl;\n>  \t\t\t\treturn TestFail;\n> @@ -111,7 +110,7 @@ protected:\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n>  \n> -\t\t\trequests.push_back(request);\n> +\t\t\trequests_.push_back(std::move(request));\n>  \t\t}\n>  \n>  \t\tcompleteRequestsCount_ = 0;\n> @@ -125,8 +124,8 @@ protected:\n>  \t\t\treturn TestFail;\n>  \t\t}\n>  \n> -\t\tfor (Request *request : requests) {\n> -\t\t\tif (camera_->queueRequest(request)) {\n> +\t\tfor (std::unique_ptr<Request> &request : requests_) {\n> +\t\t\tif (camera_->queueRequest(request.get())) {\n>  \t\t\t\tcout << \"Failed to queue request\" << endl;\n>  \t\t\t\treturn TestFail;\n>  \t\t\t}\n> @@ -161,6 +160,8 @@ protected:\n>  \t\treturn TestPass;\n>  \t}\n>  \n> +\tstd::vector<std::unique_ptr<Request>> requests_;\n> +\n>  \tstd::unique_ptr<CameraConfiguration> config_;\n>  \tFrameBufferAllocator *allocator_;\n>  };\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index 28faeb91..e63ab298 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -101,13 +101,10 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\t/* Test operations which should pass. */\n> -\t\tRequest *request2 = camera_->createRequest();\n> +\t\tstd::unique_ptr<Request> request2 = camera_->createRequest();\n>  \t\tif (!request2)\n>  \t\t\treturn TestFail;\n>  \n> -\t\t/* Never handed to hardware so need to manually delete it. */\n> -\t\tdelete request2;\n> -\n>  \t\t/* Test valid state transitions, end in Running state. */\n>  \t\tif (camera_->release())\n>  \t\t\treturn TestFail;\n> @@ -146,7 +143,7 @@ protected:\n>  \t\t\treturn TestFail;\n>  \n>  \t\t/* Test operations which should pass. */\n> -\t\tRequest *request = camera_->createRequest();\n> +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n>  \t\tif (!request)\n>  \t\t\treturn TestFail;\n>  \n> @@ -154,7 +151,7 @@ protected:\n>  \t\tif (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))\n>  \t\t\treturn TestFail;\n>  \n> -\t\tif (camera_->queueRequest(request))\n> +\t\tif (camera_->queueRequest(request.get()))\n>  \t\t\treturn TestFail;\n>  \n>  \t\t/* Test valid state transitions, end in Available state. */","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0EAF2C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Oct 2020 13:04:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8994F63B10;\n\tFri,  2 Oct 2020 15:04:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 51BF960361\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Oct 2020 15:04:23 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B17142A2;\n\tFri,  2 Oct 2020 15:04:22 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SQ6XJWPj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601643863;\n\tbh=f5sAczfRBaFAASxP7CUX590DuI3h6pTeoDk6kPpwVrI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SQ6XJWPjWYAEU2o/im6IkTSXedDhe+rEXQmvOmPxUrBRoWUjmAakxTtmlSy6RqezL\n\teQfJg2kPf6DVQ44NeyC4EvU69CI32Hk2txbI7rtsBGUN0vwj99Kx+PNi0XB5jrGwkx\n\tr0/h+U2NxnGogf0GE9kkEl1cNe9PUGA9LIPnLwcI=","Date":"Fri, 2 Oct 2020 16:03:45 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20201002130345.GB3933@pendragon.ideasonboard.com>","References":"<20201002122043.454058-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002122043.454058-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4] libcamera, android, cam, gstreamer,\n\tqcam, v4l2: Reuse Request","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12939,"web_url":"https://patchwork.libcamera.org/comment/12939/","msgid":"<20201002131226.GF45948@pyrite.rasen.tech>","date":"2020-10-02T13:12:26","subject":"Re: [libcamera-devel] [PATCH v4] libcamera, android, cam, gstreamer,\n\tqcam, v4l2: Reuse Request","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Fri, Oct 02, 2020 at 04:03:45PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Fri, Oct 02, 2020 at 09:20:43PM +0900, Paul Elder wrote:\n> > Allow reuse of the Request object by implementing reset(). This means\n> > the applications now have the responsibility of freeing the Request\n> > objects, so make all libcamera users (cam, qcam, v4l2-compat, gstreamer,\n> > android) do so.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > \n> > ---\n> > Changes in v4:\n> > - no more implicit calls of anything that we added in this patch\n> > - make reset() take a reuseBuffers boolean parameters\n> >   - use transient request - delete request\n> >   - reuse request, reset buffers - reset()\n> >   - reuse request, reuse buffesr - reset(true)\n> > - update apps and tests and documentation accordingly\n> > \n> > Changes in v3:\n> > - reset() is called in Camera::queueRequest()\n> > - apps that use transient request (android, gstreamer) delete the\n> >   request at the end of the completion handler\n> > - apps that want to reuse the buffers (cam) use reAddBuffers()\n> > - apps that need to change the buffers (qcam, v4l2) use clearBuffers()\n> > - update the documentation accordingly\n> > \n> > Changes in v2:\n> > - clear controls_ and metadata_ and validator_ in Request::reset()\n> > - use unique_ptr on application side, prior to queueRequest, and use\n> >   regular pointer for completion handler\n> > - make qcam's reuse request nicer\n> > - update Camera::queueRequest() and Camera::createRequest() documentation\n> > - add documentation for Request::reset()\n> > - make v4l2-compat reuse request\n> > - make gstreamer and android use the new createRequest API, though they\n> >   do not actually reuse the requests\n> > ---\n> >  include/libcamera/camera.h        |  2 +-\n> >  include/libcamera/request.h       |  2 ++\n> >  src/android/camera_device.cpp     | 14 +++++++---\n> >  src/cam/capture.cpp               | 29 +++++--------------\n> >  src/cam/capture.h                 |  3 ++\n> >  src/gstreamer/gstlibcamerasrc.cpp | 14 ++++++----\n> >  src/libcamera/camera.cpp          | 14 ++++------\n> >  src/libcamera/request.cpp         | 35 +++++++++++++++++++++++\n> >  src/qcam/main_window.cpp          | 46 +++++++++++++++++++------------\n> >  src/qcam/main_window.h            | 26 +++++------------\n> >  src/v4l2/v4l2_camera.cpp          | 38 +++++++++++++++++--------\n> >  src/v4l2/v4l2_camera.h            |  4 ++-\n> >  test/camera/buffer_import.cpp     | 13 +++++----\n> >  test/camera/capture.cpp           | 13 +++++----\n> >  test/camera/statemachine.cpp      |  9 ++----\n> >  15 files changed, 154 insertions(+), 108 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index a2ee4e7e..79ff8d6b 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -96,7 +96,7 @@ public:\n> >  \tstd::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});\n> >  \tint configure(CameraConfiguration *config);\n> >  \n> > -\tRequest *createRequest(uint64_t cookie = 0);\n> > +\tstd::unique_ptr<Request> createRequest(uint64_t cookie = 0);\n> >  \tint queueRequest(Request *request);\n> >  \n> >  \tint start();\n> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> > index 5976ac50..70d06933 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -38,6 +38,8 @@ public:\n> >  \tRequest &operator=(const Request &) = delete;\n> >  \t~Request();\n> >  \n> > +\tvoid reset(bool reuseBuffers = false);\n> > +\n> >  \tControlList &controls() { return *controls_; }\n> >  \tControlList &metadata() { return *metadata_; }\n> >  \tconst BufferMap &buffers() const { return bufferMap_; }\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 751699cd..052c9292 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1395,8 +1395,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >  \t\tnew Camera3RequestDescriptor(camera3Request->frame_number,\n> >  \t\t\t\t\t     camera3Request->num_output_buffers);\n> >  \n> > -\tRequest *request =\n> > +\tstd::unique_ptr<Request> request =\n> >  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n> > +\tif (!request) {\n> > +\t\tLOG(HAL, Error) << \"Failed to create request\";\n> > +\t\treturn -ENOMEM;\n> > +\t}\n> >  \n> >  \tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> >  \t\tCameraStream *cameraStream =\n> > @@ -1422,7 +1426,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >  \t\tFrameBuffer *buffer = createFrameBuffer(*camera3Buffers[i].buffer);\n> >  \t\tif (!buffer) {\n> >  \t\t\tLOG(HAL, Error) << \"Failed to create buffer\";\n> > -\t\t\tdelete request;\n> >  \t\t\tdelete descriptor;\n> >  \t\t\treturn -ENOMEM;\n> >  \t\t}\n> > @@ -1434,14 +1437,16 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >  \t\trequest->addBuffer(stream, buffer);\n> >  \t}\n> >  \n> > -\tint ret = camera_->queueRequest(request);\n> > +\tint ret = camera_->queueRequest(request.get());\n> >  \tif (ret) {\n> >  \t\tLOG(HAL, Error) << \"Failed to queue request\";\n> > -\t\tdelete request;\n> >  \t\tdelete descriptor;\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> > +\t/* The request will be deleted in the completion handler. */\n> > +\trequest.release();\n> > +\n> >  \treturn 0;\n> >  }\n> >  \n> > @@ -1593,6 +1598,7 @@ void CameraDevice::requestComplete(Request *request)\n> >  \tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> >  \n> >  \tdelete descriptor;\n> > +\tdelete request;\n> >  }\n> >  \n> >  std::string CameraDevice::logPrefix() const\n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 5510c009..e6aaf79f 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -65,6 +65,8 @@ int Capture::run(const OptionsParser::Options &options)\n> >  \t\twriter_ = nullptr;\n> >  \t}\n> >  \n> > +\trequests_.clear();\n> > +\n> >  \tdelete allocator;\n> >  \n> >  \treturn ret;\n> > @@ -92,9 +94,8 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> >  \t * example pushing a button. For now run all streams all the time.\n> >  \t */\n> >  \n> > -\tstd::vector<Request *> requests;\n> >  \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > -\t\tRequest *request = camera_->createRequest();\n> > +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> >  \t\tif (!request) {\n> >  \t\t\tstd::cerr << \"Can't create request\" << std::endl;\n> >  \t\t\treturn -ENOMEM;\n> > @@ -117,7 +118,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> >  \t\t\t\twriter_->mapBuffer(buffer.get());\n> >  \t\t}\n> >  \n> > -\t\trequests.push_back(request);\n> > +\t\trequests_.push_back(std::move(request));\n> >  \t}\n> >  \n> >  \tret = camera_->start();\n> > @@ -126,8 +127,8 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> >  \t\treturn ret;\n> >  \t}\n> >  \n> > -\tfor (Request *request : requests) {\n> > -\t\tret = camera_->queueRequest(request);\n> > +\tfor (std::unique_ptr<Request> &request : requests_) {\n> > +\t\tret = camera_->queueRequest(request.get());\n> >  \t\tif (ret < 0) {\n> >  \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> >  \t\t\tcamera_->stop();\n> > @@ -202,22 +203,6 @@ void Capture::requestComplete(Request *request)\n> >  \t\treturn;\n> >  \t}\n> >  \n> > -\t/*\n> > -\t * Create a new request and populate it with one buffer for each\n> > -\t * stream.\n> > -\t */\n> > -\trequest = camera_->createRequest();\n> > -\tif (!request) {\n> > -\t\tstd::cerr << \"Can't create request\" << std::endl;\n> > -\t\treturn;\n> > -\t}\n> > -\n> > -\tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > -\t\tconst Stream *stream = it->first;\n> > -\t\tFrameBuffer *buffer = it->second;\n> > -\n> > -\t\trequest->addBuffer(stream, buffer);\n> > -\t}\n> > -\n> > +\trequest->reset(true);\n> >  \tcamera_->queueRequest(request);\n> >  }\n> > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > index 0aebdac9..45e5e8a9 100644\n> > --- a/src/cam/capture.h\n> > +++ b/src/cam/capture.h\n> > @@ -9,6 +9,7 @@\n> >  \n> >  #include <memory>\n> >  #include <stdint.h>\n> > +#include <vector>\n> >  \n> >  #include <libcamera/buffer.h>\n> >  #include <libcamera/camera.h>\n> > @@ -43,6 +44,8 @@ private:\n> >  \tEventLoop *loop_;\n> >  \tunsigned int captureCount_;\n> >  \tunsigned int captureLimit_;\n> > +\n> > +\tstd::vector<std::unique_ptr<libcamera::Request>> requests_;\n> >  };\n> >  \n> >  #endif /* __CAM_CAPTURE_H__ */\n> > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> > index 1bfc2e2f..f25b6420 100644\n> > --- a/src/gstreamer/gstlibcamerasrc.cpp\n> > +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> > @@ -74,6 +74,8 @@ RequestWrap::~RequestWrap()\n> >  \t\tif (item.second)\n> >  \t\t\tgst_buffer_unref(item.second);\n> >  \t}\n> > +\n> > +\tdelete request_;\n> >  }\n> >  \n> >  void RequestWrap::attachBuffer(GstBuffer *buffer)\n> > @@ -266,8 +268,8 @@ gst_libcamera_src_task_run(gpointer user_data)\n> >  \tGstLibcameraSrc *self = GST_LIBCAMERA_SRC(user_data);\n> >  \tGstLibcameraSrcState *state = self->state;\n> >  \n> > -\tRequest *request = state->cam_->createRequest();\n> > -\tauto wrap = std::make_unique<RequestWrap>(request);\n> > +\tstd::unique_ptr<Request> request = state->cam_->createRequest();\n> > +\tauto wrap = std::make_unique<RequestWrap>(request.get());\n> >  \tfor (GstPad *srcpad : state->srcpads_) {\n> >  \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> >  \t\tGstBuffer *buffer;\n> > @@ -280,8 +282,7 @@ gst_libcamera_src_task_run(gpointer user_data)\n> >  \t\t\t * RequestWrap does not take ownership, and we won't be\n> >  \t\t\t * queueing this one due to lack of buffers.\n> >  \t\t\t */\n> > -\t\t\tdelete request;\n> > -\t\t\trequest = nullptr;\n> > +\t\t\trequest.reset(nullptr);\n> \n> nullptr is the default, so you could write\n> \n> \t\t\trequest.reset();\n\nack\n\n> >  \t\t\tbreak;\n> >  \t\t}\n> >  \n> > @@ -291,8 +292,11 @@ gst_libcamera_src_task_run(gpointer user_data)\n> >  \tif (request) {\n> >  \t\tGLibLocker lock(GST_OBJECT(self));\n> >  \t\tGST_TRACE_OBJECT(self, \"Requesting buffers\");\n> > -\t\tstate->cam_->queueRequest(request);\n> > +\t\tstate->cam_->queueRequest(request.get());\n> >  \t\tstate->requests_.push(std::move(wrap));\n> > +\n> > +\t\t/* The request will be deleted in the completion handler. */\n> > +\t\trequest.release();\n> >  \t}\n> >  \n> >  \tGstFlowReturn ret = GST_FLOW_OK;\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index fb76077f..2639a415 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -847,21 +847,22 @@ int Camera::configure(CameraConfiguration *config)\n> >   * handler, and is completely opaque to libcamera.\n> >   *\n> >   * The ownership of the returned request is passed to the caller, which is\n> > - * responsible for either queueing the request or deleting it.\n> > + * responsible for deleting it. The request may be deleted in the completion\n> > + * handler, or reused after clearing its buffers with Request::clearBuffers().\n> \n> There's no clearBuffers() anymore :-)\n\nOops :p\n\n> >   *\n> >   * \\context This function is \\threadsafe. It may only be called when the camera\n> >   * is in the Configured or Running state as defined in \\ref camera_operation.\n> >   *\n> >   * \\return A pointer to the newly created request, or nullptr on error\n> >   */\n> > -Request *Camera::createRequest(uint64_t cookie)\n> > +std::unique_ptr<Request> Camera::createRequest(uint64_t cookie)\n> >  {\n> >  \tint ret = p_->isAccessAllowed(Private::CameraConfigured,\n> >  \t\t\t\t      Private::CameraRunning);\n> >  \tif (ret < 0)\n> >  \t\treturn nullptr;\n> >  \n> > -\treturn new Request(this, cookie);\n> > +\treturn std::make_unique<Request>(this, cookie);\n> >  }\n> >  \n> >  /**\n> > @@ -877,9 +878,6 @@ Request *Camera::createRequest(uint64_t cookie)\n> >   * Once the request has been queued, the camera will notify its completion\n> >   * through the \\ref requestCompleted signal.\n> >   *\n> > - * Ownership of the request is transferred to the camera. It will be deleted\n> > - * automatically after it completes.\n> > - *\n> >   * \\context This function is \\threadsafe. It may only be called when the camera\n> >   * is in the Running state as defined in \\ref camera_operation.\n> >   *\n> > @@ -988,13 +986,11 @@ int Camera::stop()\n> >   * \\param[in] request The request that has completed\n> >   *\n> >   * This function is called by the pipeline handler to notify the camera that\n> > - * the request has completed. It emits the requestCompleted signal and deletes\n> > - * the request.\n> > + * the request has completed. It emits the requestCompleted signal.\n> >   */\n> >  void Camera::requestComplete(Request *request)\n> >  {\n> >  \trequestCompleted.emit(request);\n> > -\tdelete request;\n> >  }\n> >  \n> >  } /* namespace libcamera */\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 60b30692..f19995d3 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -85,6 +85,41 @@ Request::~Request()\n> >  \tdelete validator_;\n> >  }\n> >  \n> > +/*\n> > + * \\brief Reset the request\n> > + * \\param[in] reuseBuffers Flag to indicate whether or not to reuse the buffers\n> > + *\n> > + * Reset the status and controls associated with the request, to allow it to\n> > + * be reused and requeued without destruction. This function should be called\n> > + * prior to queueing the request to the camera, in lieu of constructing a new\n> > + * request. If the application wishes to reuse the buffers that were previously\n> > + * added to the request via addBuffer(), then \\a reuseBuffers should be set to\n> > + * true.\n> > + */\n> > +void Request::reset(bool reuseBuffers)\n> \n> Boolean parameters are not very nice when their usage isn't implied by\n> the function name. When reading code,\n> \n> \trequest->reset(true);\n> \n> isn't very explicit. An enum parameter would be better:\n> \n> \trequest->reset(Request::ReuseBuffers);\n\nAh, I see.\n\n> I even wonder if we should call the function Request::reuse() instead of\n> reset(). With requests stored in unique_ptr,\n> \n> \tstd::unique_ptr<Request> request = ...;\n> \n> \trequest->reset();\n> \trequest.reset();\n> \n> could lead to confusion.\n\nOoh... that's true. I think reset is a better verb, but to avoid that\nconfusion maybe reuse would be fine.\n\n> > +{\n> > +\tpending_.clear();\n> > +\tif (reuseBuffers) {\n> > +\t\tfor (auto pair : bufferMap_) {\n> > +\t\t\tpair.second->request_ = this;\n> > +\t\t\tpending_.insert(pair.second);\n> > +\t\t}\n> > +\t} else {\n> > +\t\tbufferMap_.clear();\n> > +\t}\n> > +\n> > +\tstatus_ = RequestPending;\n> > +\tcancelled_ = false;\n> > +\n> > +\tdelete metadata_;\n> > +\tdelete controls_;\n> > +\tdelete validator_;\n> > +\n> > +\tvalidator_ = new CameraControlValidator(camera_);\n> \n> Do we need to recreate the validator, as camera_ is the same ?\n> \n> > +\tcontrols_ = new ControlList(controls::controls, validator_);\n> > +\tmetadata_ = new ControlList(controls::controls);\n> \n> Maybe just\n> \n> \tcontrols_.clear();\n> \tmetadata_.clear();\n> \n> instead of deleting them ?\n\nOh, okay. I didn't realize that was good enough.\n\n> > +}\n> > +\n> >  /**\n> >   * \\fn Request::controls()\n> >   * \\brief Retrieve the request's ControlList\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index ecb9dd66..5192cbb8 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -367,7 +367,6 @@ void MainWindow::toggleCapture(bool start)\n> >  int MainWindow::startCapture()\n> >  {\n> >  \tStreamRoles roles = StreamKeyValueParser::roles(options_[OptStream]);\n> > -\tstd::vector<Request *> requests;\n> >  \tint ret;\n> >  \n> >  \t/* Verify roles are supported. */\n> > @@ -486,7 +485,7 @@ int MainWindow::startCapture()\n> >  \twhile (!freeBuffers_[vfStream_].isEmpty()) {\n> >  \t\tFrameBuffer *buffer = freeBuffers_[vfStream_].dequeue();\n> >  \n> > -\t\tRequest *request = camera_->createRequest();\n> > +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> >  \t\tif (!request) {\n> >  \t\t\tqWarning() << \"Can't create request\";\n> >  \t\t\tret = -ENOMEM;\n> > @@ -499,7 +498,7 @@ int MainWindow::startCapture()\n> >  \t\t\tgoto error;\n> >  \t\t}\n> >  \n> > -\t\trequests.push_back(request);\n> > +\t\trequests_.push_back(std::move(request));\n> >  \t}\n> >  \n> >  \t/* Start the title timer and the camera. */\n> > @@ -518,8 +517,8 @@ int MainWindow::startCapture()\n> >  \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> >  \n> >  \t/* Queue all requests. */\n> > -\tfor (Request *request : requests) {\n> > -\t\tret = camera_->queueRequest(request);\n> > +\tfor (std::unique_ptr<Request> &request : requests_) {\n> > +\t\tret = camera_->queueRequest(request.get());\n> >  \t\tif (ret < 0) {\n> >  \t\t\tqWarning() << \"Can't queue request\";\n> >  \t\t\tgoto error_disconnect;\n> > @@ -535,8 +534,7 @@ error_disconnect:\n> >  \tcamera_->stop();\n> >  \n> >  error:\n> > -\tfor (Request *request : requests)\n> > -\t\tdelete request;\n> > +\trequests_.clear();\n> >  \n> >  \tfor (auto &iter : mappedBuffers_) {\n> >  \t\tconst MappedBuffer &buffer = iter.second;\n> > @@ -580,6 +578,8 @@ void MainWindow::stopCapture()\n> >  \t}\n> >  \tmappedBuffers_.clear();\n> >  \n> > +\trequests_.clear();\n> > +\n> >  \tdelete allocator_;\n> >  \n> >  \tisCapturing_ = false;\n> > @@ -701,7 +701,7 @@ void MainWindow::requestComplete(Request *request)\n> >  \t */\n> >  \t{\n> >  \t\tQMutexLocker locker(&mutex_);\n> > -\t\tdoneQueue_.enqueue({ request->buffers(), request->metadata() });\n> > +\t\tdoneQueue_.enqueue(request);\n> >  \t}\n> >  \n> >  \tQCoreApplication::postEvent(this, new CaptureEvent);\n> > @@ -714,8 +714,7 @@ void MainWindow::processCapture()\n> >  \t * if stopCapture() has been called while a CaptureEvent was posted but\n> >  \t * not processed yet. Return immediately in that case.\n> >  \t */\n> > -\tCaptureRequest request;\n> > -\n> > +\tRequest *request;\n> >  \t{\n> >  \t\tQMutexLocker locker(&mutex_);\n> >  \t\tif (doneQueue_.isEmpty())\n> > @@ -724,12 +723,19 @@ void MainWindow::processCapture()\n> >  \t\trequest = doneQueue_.dequeue();\n> >  \t}\n> >  \n> > +\tRequest::BufferMap buffers = request->buffers();\n> \n> Do you need to copy the map, can't it be a reference ?\n\nHere, no, because processViewfinder and processRaw both want non-const\nbut the reference returned by request->buffers() is const.\n\n> > +\n> >  \t/* Process buffers. */\n> > -\tif (request.buffers_.count(vfStream_))\n> > -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> > +\tif (request->buffers().count(vfStream_))\n> > +\t\tprocessViewfinder(buffers[vfStream_]);\n> >  \n> > -\tif (request.buffers_.count(rawStream_))\n> > -\t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> > +\tif (request->buffers().count(rawStream_))\n> > +\t\tprocessRaw(buffers[rawStream_], request->metadata());\n> > +\n> > +\t{\n> > +\t\tQMutexLocker locker(&mutex_);\n> > +\t\tfreeQueue_.enqueue(request);\n> > +\t}\n> \n> You don't need a specific scope for this,\n> \n> \tQMutexLocker locker(&mutex_);\n> \tfreeQueue_.enqueue(request);\n> \n> will do as it's at the end of the function.\n\nack\n\n> >  }\n> >  \n> >  void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > @@ -754,12 +760,16 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> >  \n> >  void MainWindow::queueRequest(FrameBuffer *buffer)\n> >  {\n> > -\tRequest *request = camera_->createRequest();\n> > -\tif (!request) {\n> > -\t\tqWarning() << \"Can't create request\";\n> > -\t\treturn;\n> > +\tRequest *request;\n> > +\t{\n> > +\t\tQMutexLocker locker(&mutex_);\n> > +\t\tif (freeQueue_.isEmpty())\n> > +\t\t\tqWarning() << \"No free request\";\n> > +\n> > +\t\trequest = freeQueue_.dequeue();\n> >  \t}\n> >  \n> > +\trequest->reset();\n> \n> Could this be moved just before freeQueue_.enqueue(request) (to be\n> precise before creating the mutex locker) ?\n\nack\n\n> >  \trequest->addBuffer(vfStream_, buffer);\n> >  \n> >  \tif (captureRaw_) {\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 5c61a4df..64bcfebc 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -8,6 +8,7 @@\n> >  #define __QCAM_MAIN_WINDOW_H__\n> >  \n> >  #include <memory>\n> > +#include <vector>\n> >  \n> >  #include <QElapsedTimer>\n> >  #include <QIcon>\n> > @@ -22,6 +23,7 @@\n> >  #include <libcamera/camera_manager.h>\n> >  #include <libcamera/controls.h>\n> >  #include <libcamera/framebuffer_allocator.h>\n> > +#include <libcamera/request.h>\n> >  #include <libcamera/stream.h>\n> >  \n> >  #include \"../cam/stream_options.h\"\n> > @@ -41,23 +43,6 @@ enum {\n> >  \tOptStream = 's',\n> >  };\n> >  \n> > -class CaptureRequest\n> > -{\n> > -public:\n> > -\tCaptureRequest()\n> > -\t{\n> > -\t}\n> > -\n> > -\tCaptureRequest(const Request::BufferMap &buffers,\n> > -\t\t       const ControlList &metadata)\n> > -\t\t: buffers_(buffers), metadata_(metadata)\n> > -\t{\n> > -\t}\n> > -\n> > -\tRequest::BufferMap buffers_;\n> > -\tControlList metadata_;\n> > -};\n> > -\n> >  class MainWindow : public QMainWindow\n> >  {\n> >  \tQ_OBJECT\n> > @@ -128,13 +113,16 @@ private:\n> >  \tStream *vfStream_;\n> >  \tStream *rawStream_;\n> >  \tstd::map<const Stream *, QQueue<FrameBuffer *>> freeBuffers_;\n> > -\tQQueue<CaptureRequest> doneQueue_;\n> > -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> > +\tQQueue<Request *> doneQueue_;\n> > +\tQQueue<Request *> freeQueue_;\n> > +\tQMutex mutex_; /* Protects freeBuffers_, doneQueue_, and freeQueue_ */\n> >  \n> >  \tuint64_t lastBufferTime_;\n> >  \tQElapsedTimer frameRateInterval_;\n> >  \tuint32_t previousFrames_;\n> >  \tuint32_t framesCaptured_;\n> > +\n> > +\tstd::vector<std::unique_ptr<Request>> requests_;\n> >  };\n> >  \n> >  #endif /* __QCAM_MAIN_WINDOW__ */\n> > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> > index 3565f369..0d59502a 100644\n> > --- a/src/v4l2/v4l2_camera.cpp\n> > +++ b/src/v4l2/v4l2_camera.cpp\n> > @@ -49,6 +49,8 @@ int V4L2Camera::open(StreamConfiguration *streamConfig)\n> >  \n> >  void V4L2Camera::close()\n> >  {\n> > +\trequestPool_.clear();\n> > +\n> >  \tdelete bufferAllocator_;\n> >  \tbufferAllocator_ = nullptr;\n> >  \n> > @@ -154,16 +156,30 @@ int V4L2Camera::validateConfiguration(const PixelFormat &pixelFormat,\n> >  \treturn 0;\n> >  }\n> >  \n> > -int V4L2Camera::allocBuffers([[maybe_unused]] unsigned int count)\n> > +int V4L2Camera::allocBuffers(unsigned int count)\n> >  {\n> >  \tStream *stream = config_->at(0).stream();\n> >  \n> > -\treturn bufferAllocator_->allocate(stream);\n> > +\tint ret = bufferAllocator_->allocate(stream);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\tfor (unsigned int i = 0; i < count; i++) {\n> > +\t\tstd::unique_ptr<Request> request = camera_->createRequest(i);\n> > +\t\tif (!request) {\n> > +\t\t\trequestPool_.clear();\n> > +\t\t\treturn -ENOMEM;\n> > +\t\t}\n> > +\t\trequestPool_.push_back(std::move(request));\n> > +\t}\n> > +\n> > +\treturn ret;\n> >  }\n> >  \n> >  void V4L2Camera::freeBuffers()\n> >  {\n> >  \tpendingRequests_.clear();\n> > +\trequestPool_.clear();\n> >  \n> >  \tStream *stream = config_->at(0).stream();\n> >  \tbufferAllocator_->free(stream);\n> > @@ -192,9 +208,9 @@ int V4L2Camera::streamOn()\n> >  \n> >  \tisRunning_ = true;\n> >  \n> > -\tfor (std::unique_ptr<Request> &req : pendingRequests_) {\n> > +\tfor (Request *req : pendingRequests_) {\n> >  \t\t/* \\todo What should we do if this returns -EINVAL? */\n> > -\t\tret = camera_->queueRequest(req.release());\n> > +\t\tret = camera_->queueRequest(req);\n> >  \t\tif (ret < 0)\n> >  \t\t\treturn ret == -EACCES ? -EBUSY : ret;\n> >  \t}\n> > @@ -226,12 +242,12 @@ int V4L2Camera::streamOff()\n> >  \n> >  int V4L2Camera::qbuf(unsigned int index)\n> >  {\n> > -\tstd::unique_ptr<Request> request =\n> > -\t\tstd::unique_ptr<Request>(camera_->createRequest(index));\n> > -\tif (!request) {\n> > -\t\tLOG(V4L2Compat, Error) << \"Can't create request\";\n> > -\t\treturn -ENOMEM;\n> > +\tif (index >= requestPool_.size()) {\n> > +\t\tLOG(V4L2Compat, Error) << \"Invalid index\";\n> > +\t\treturn -EINVAL;\n> >  \t}\n> > +\tRequest *request = requestPool_[index].get();\n> > +\trequest->reset();\n> \n> Can reset() be moved at the end of V4L2Camera::requestComplete() ?\n\nI think so. I was thinking that reset() replaces createRequest,\ntherefore it should be here instead.\n\n> >  \n> >  \tStream *stream = config_->at(0).stream();\n> >  \tFrameBuffer *buffer = bufferAllocator_->buffers(stream)[index].get();\n> > @@ -242,11 +258,11 @@ int V4L2Camera::qbuf(unsigned int index)\n> >  \t}\n> >  \n> >  \tif (!isRunning_) {\n> > -\t\tpendingRequests_.push_back(std::move(request));\n> > +\t\tpendingRequests_.push_back(request);\n> >  \t\treturn 0;\n> >  \t}\n> >  \n> > -\tret = camera_->queueRequest(request.release());\n> > +\tret = camera_->queueRequest(request);\n> >  \tif (ret < 0) {\n> >  \t\tLOG(V4L2Compat, Error) << \"Can't queue request\";\n> >  \t\treturn ret == -EACCES ? -EBUSY : ret;\n> > diff --git a/src/v4l2/v4l2_camera.h b/src/v4l2/v4l2_camera.h\n> > index 1fc5ebef..a6c35a2e 100644\n> > --- a/src/v4l2/v4l2_camera.h\n> > +++ b/src/v4l2/v4l2_camera.h\n> > @@ -76,7 +76,9 @@ private:\n> >  \tstd::mutex bufferLock_;\n> >  \tFrameBufferAllocator *bufferAllocator_;\n> >  \n> > -\tstd::deque<std::unique_ptr<Request>> pendingRequests_;\n> > +\tstd::vector<std::unique_ptr<Request>> requestPool_;\n> > +\n> > +\tstd::deque<Request *> pendingRequests_;\n> >  \tstd::deque<std::unique_ptr<Buffer>> completedBuffers_;\n> >  \n> >  \tint efd_;\n> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > index 64e96264..ea9fbc78 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -58,7 +58,7 @@ protected:\n> >  \t\tconst Stream *stream = buffers.begin()->first;\n> >  \t\tFrameBuffer *buffer = buffers.begin()->second;\n> >  \n> > -\t\trequest = camera_->createRequest();\n> > +\t\trequest->reset();\n> >  \t\trequest->addBuffer(stream, buffer);\n> >  \t\tcamera_->queueRequest(request);\n> >  \t}\n> > @@ -98,9 +98,8 @@ protected:\n> >  \t\tif (ret != TestPass)\n> >  \t\t\treturn ret;\n> >  \n> > -\t\tstd::vector<Request *> requests;\n> >  \t\tfor (const std::unique_ptr<FrameBuffer> &buffer : source.buffers()) {\n> > -\t\t\tRequest *request = camera_->createRequest();\n> > +\t\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> >  \t\t\tif (!request) {\n> >  \t\t\t\tstd::cout << \"Failed to create request\" << std::endl;\n> >  \t\t\t\treturn TestFail;\n> > @@ -111,7 +110,7 @@ protected:\n> >  \t\t\t\treturn TestFail;\n> >  \t\t\t}\n> >  \n> > -\t\t\trequests.push_back(request);\n> > +\t\t\trequests_.push_back(std::move(request));\n> >  \t\t}\n> >  \n> >  \t\tcompleteRequestsCount_ = 0;\n> > @@ -125,8 +124,8 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > -\t\tfor (Request *request : requests) {\n> > -\t\t\tif (camera_->queueRequest(request)) {\n> > +\t\tfor (std::unique_ptr<Request> &request : requests_) {\n> > +\t\t\tif (camera_->queueRequest(request.get())) {\n> >  \t\t\t\tstd::cout << \"Failed to queue request\" << std::endl;\n> >  \t\t\t\treturn TestFail;\n> >  \t\t\t}\n> > @@ -160,6 +159,8 @@ protected:\n> >  \t}\n> >  \n> >  private:\n> > +\tstd::vector<std::unique_ptr<Request>> requests_;\n> > +\n> >  \tunsigned int completeBuffersCount_;\n> >  \tunsigned int completeRequestsCount_;\n> >  \tstd::unique_ptr<CameraConfiguration> config_;\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index 51bbd258..ffe29cb9 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -52,7 +52,7 @@ protected:\n> >  \t\tconst Stream *stream = buffers.begin()->first;\n> >  \t\tFrameBuffer *buffer = buffers.begin()->second;\n> >  \n> > -\t\trequest = camera_->createRequest();\n> > +\t\trequest->reset();\n> >  \t\trequest->addBuffer(stream, buffer);\n> >  \t\tcamera_->queueRequest(request);\n> >  \t}\n> > @@ -98,9 +98,8 @@ protected:\n> >  \t\tif (ret < 0)\n> >  \t\t\treturn TestFail;\n> >  \n> > -\t\tstd::vector<Request *> requests;\n> >  \t\tfor (const std::unique_ptr<FrameBuffer> &buffer : allocator_->buffers(stream)) {\n> > -\t\t\tRequest *request = camera_->createRequest();\n> > +\t\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> >  \t\t\tif (!request) {\n> >  \t\t\t\tcout << \"Failed to create request\" << endl;\n> >  \t\t\t\treturn TestFail;\n> > @@ -111,7 +110,7 @@ protected:\n> >  \t\t\t\treturn TestFail;\n> >  \t\t\t}\n> >  \n> > -\t\t\trequests.push_back(request);\n> > +\t\t\trequests_.push_back(std::move(request));\n> >  \t\t}\n> >  \n> >  \t\tcompleteRequestsCount_ = 0;\n> > @@ -125,8 +124,8 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >  \n> > -\t\tfor (Request *request : requests) {\n> > -\t\t\tif (camera_->queueRequest(request)) {\n> > +\t\tfor (std::unique_ptr<Request> &request : requests_) {\n> > +\t\t\tif (camera_->queueRequest(request.get())) {\n> >  \t\t\t\tcout << \"Failed to queue request\" << endl;\n> >  \t\t\t\treturn TestFail;\n> >  \t\t\t}\n> > @@ -161,6 +160,8 @@ protected:\n> >  \t\treturn TestPass;\n> >  \t}\n> >  \n> > +\tstd::vector<std::unique_ptr<Request>> requests_;\n> > +\n> >  \tstd::unique_ptr<CameraConfiguration> config_;\n> >  \tFrameBufferAllocator *allocator_;\n> >  };\n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index 28faeb91..e63ab298 100644\n> > --- a/test/camera/statemachine.cpp\n> > +++ b/test/camera/statemachine.cpp\n> > @@ -101,13 +101,10 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \n> >  \t\t/* Test operations which should pass. */\n> > -\t\tRequest *request2 = camera_->createRequest();\n> > +\t\tstd::unique_ptr<Request> request2 = camera_->createRequest();\n> >  \t\tif (!request2)\n> >  \t\t\treturn TestFail;\n> >  \n> > -\t\t/* Never handed to hardware so need to manually delete it. */\n> > -\t\tdelete request2;\n> > -\n> >  \t\t/* Test valid state transitions, end in Running state. */\n> >  \t\tif (camera_->release())\n> >  \t\t\treturn TestFail;\n> > @@ -146,7 +143,7 @@ protected:\n> >  \t\t\treturn TestFail;\n> >  \n> >  \t\t/* Test operations which should pass. */\n> > -\t\tRequest *request = camera_->createRequest();\n> > +\t\tstd::unique_ptr<Request> request = camera_->createRequest();\n> >  \t\tif (!request)\n> >  \t\t\treturn TestFail;\n> >  \n> > @@ -154,7 +151,7 @@ protected:\n> >  \t\tif (request->addBuffer(stream, allocator_->buffers(stream)[0].get()))\n> >  \t\t\treturn TestFail;\n> >  \n> > -\t\tif (camera_->queueRequest(request))\n> > +\t\tif (camera_->queueRequest(request.get()))\n> >  \t\t\treturn TestFail;\n> >  \n> >  \t\t/* Test valid state transitions, end in Available state. */\n\n\nThanks,\n\nPaul","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E6929C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Oct 2020 13:12:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CDCA63B10;\n\tFri,  2 Oct 2020 15:12:36 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B5D8060361\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Oct 2020 15:12:34 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB48C2A2;\n\tFri,  2 Oct 2020 15:12:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"D1nkGzeC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601644354;\n\tbh=5B96+HCBREPJ8HrTwXG/b+OOTfa4GL1Oqy8Fsi1FWCU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=D1nkGzeC1Ug87exkdQCiS3OyHqAHVOjfHrjUV091POVn/xKL2I8UcmK0coCQYLu6W\n\tfWDd6916shDu+5Jf6S7JVZg70OzMNFsUtE9+Z/62g9e1o22cbSCVv+CcgraobttMi2\n\tLTTQqdCXIrP7oi8CQ7xQ0dYsLZOncZWkoLZHoc1g=","Date":"Fri, 2 Oct 2020 22:12:26 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20201002131226.GF45948@pyrite.rasen.tech>","References":"<20201002122043.454058-1-paul.elder@ideasonboard.com>\n\t<20201002130345.GB3933@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002130345.GB3933@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4] libcamera, android, cam, gstreamer,\n\tqcam, v4l2: Reuse Request","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]