[{"id":12907,"web_url":"https://patchwork.libcamera.org/comment/12907/","msgid":"<20200930103911.GD1516931@oden.dyn.berto.se>","date":"2020-09-30T10:39:11","subject":"Re: [libcamera-devel] [PATCH v3] libcamera, android, cam, gstreamer,\n\tqcam, v4l2: Reuse Request","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Paul,\n\nThanks for your work.\n\nOn 2020-09-30 19:17:17 +0900, Paul Elder wrote:\n> Allow reuse of the Request object by implementing reset(),\n> clearBuffers(), and reAddBuffers().  This means that the applications\n> now have the responsibility of freeing the Request objects, so make all\n> libcamera users (cam, qcam, v4l2-compat, gstreamer, android) do so.\n> \n> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> \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       |  4 ++\n>  src/android/camera_device.cpp     |  8 +++-\n>  src/cam/capture.cpp               | 29 ++++----------\n>  src/cam/capture.h                 |  3 ++\n>  src/gstreamer/gstlibcamerasrc.cpp |  6 ++-\n>  src/libcamera/camera.cpp          | 19 ++++-----\n>  src/libcamera/request.cpp         | 64 +++++++++++++++++++++++++++++++\n>  src/qcam/main_window.cpp          | 48 ++++++++++++++---------\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     | 15 ++++----\n>  test/camera/capture.cpp           | 15 ++++----\n>  test/camera/statemachine.cpp      |  9 ++---\n>  15 files changed, 186 insertions(+), 104 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 272c12c3..e4036b63 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -93,7 +93,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..6a651f36 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -38,6 +38,10 @@ public:\n>  \tRequest &operator=(const Request &) = delete;\n>  \t~Request();\n>  \n> +\tvoid reset();\n> +\tvoid clearBuffers();\n> +\tvoid reAddBuffers();\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 70d77a17..7f288617 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1395,8 +1395,13 @@ 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> r =\n>  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n> +\tRequest *request = r.release();\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> @@ -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..5b223365 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->reAddBuffers();\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..ca223df6 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,7 +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> +\tstd::unique_ptr<Request> request_unique = state->cam_->createRequest();\n> +\tRequest *request = request_unique.release();\n>  \tauto wrap = std::make_unique<RequestWrap>(request);\n>  \tfor (GstPad *srcpad : state->srcpads_) {\n>  \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)\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\tbreak;\n>  \t\t}\n>  \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index ae16a64a..d4c18c72 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -833,21 +833,25 @@ 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 either queueing the request or deleting it. The caller\n> + * continues to own the request after queueing it, so the application is\n> + * responsible for deleting the request after the request completes.\n> + * Alternatively, the request may be reused after clearing its buffers via\n> + * Request::clearBuffers().\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> @@ -863,9 +867,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> @@ -877,6 +878,8 @@ Request *Camera::createRequest(uint64_t cookie)\n>   */\n>  int Camera::queueRequest(Request *request)\n>  {\n> +\trequest->reset();\n> +\n>  \tint ret = p_->isAccessAllowed(Private::CameraRunning);\n>  \tif (ret < 0)\n>  \t\treturn ret;\n> @@ -974,13 +977,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..7d51e504 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -85,6 +85,70 @@ Request::~Request()\n>  \tdelete validator_;\n>  }\n>  \n> +/*\n> + * \\brief Reset the request\n> + *\n> + * Reset the status and controls associated with the request, to allow it to\n> + * be reused and requeued without destruction. This function is meant to be\n> + * called by Camera::queueRequest().\n> + *\n> + * \\todo Make this private and make Camera a friend class?\n> + */\n> +void Request::reset()\n> +{\n> +\tstatus_ = RequestPending;\n> +\tcancelled_ = false;\n> +\n> +\tdelete metadata_;\n> +\tdelete controls_;\n> +\tdelete validator_;\n> +\n> +\tvalidator_ = new CameraControlValidator(camera_);\n> +\tcontrols_ = new ControlList(controls::controls, validator_);\n> +\tmetadata_ = new ControlList(controls::controls);\n> +}\n> +\n> +/*\n> + * \\brief Clear buffers associated with the request\n> + *\n> + * Delete the pointers to the FrameBuffers that were added to this request via\n> + * addBuffers().\n> + *\n> + * This function should be called in (or after) the request completion handler\n> + * in applications prior to modifying the buffers and requeueing the request to\n> + * the camera. If the buffers do not need to be modified, and are to be reused,\n> + * use reAddBuffers(). If the request is meant to be transient, then it\n> + * should be deleted at the end of the completion handler.\n> + */\n> +void Request::clearBuffers()\n> +{\n> +\tpending_.clear();\n> +\n> +\tbufferMap_.clear();\n> +}\n> +\n> +/*\n> + * \\brief Re-add the buffers that were added to the request\n> + *\n> + * Re-add the FrameBuffers that were added to this request via addBuffers().\n> + *\n> + * This function should be called in (or after) the request completion handler\n> + * in applications that wish to reuse the buffers that were added to the request\n> + * when it was queued to the camera. If the buffers need to be modified, they\n> + * should be cleared with clearBuffers() first instead. If the request is\n> + * meant to be transient, then it should be deleted at the end of the\n> + * completion handler.\n> + */\n> +void Request::reAddBuffers()\n> +{\n> +\tpending_.clear();\n> +\n> +\tfor (auto pair : bufferMap_) {\n> +\t\tpair.second->request_ = this;\n> +\t\tpending_.insert(pair.second);\n> +\t}\n> +}\n\nI wonder would it not be neat to call this unconditionally in \nPipelineHandler::completeRequest()?\n\nAn application wishing to reuse a request can then simply just requeue \nit without touching it. While an application wishing to modify the \nrequest before reusing it will be responsible for removeAllBuffers() or \nremoveBuffer(Stream *) the Request to fit it's new usage.\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 985743f3..466f3053 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> @@ -725,11 +724,19 @@ void MainWindow::processCapture()\n>  \t}\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\t{\n> +\t\t\tQMutexLocker locker(&mutex_);\n> +\t\t\tfreeQueue_.enqueue(request);\n> +\t\t}\n> +\t\tRequest::BufferMap buffers = request->buffers();\n> +\t\tprocessViewfinder(buffers[vfStream_]);\n> +\t}\n>  \n> -\tif (request.buffers_.count(rawStream_))\n> -\t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> +\tif (request->buffers().count(rawStream_)) {\n> +\t\tRequest::BufferMap buffers = request->buffers();\n> +\t\tprocessRaw(buffers[rawStream_], request->metadata());\n> +\t}\n>  }\n>  \n>  void MainWindow::processViewfinder(FrameBuffer *buffer)\n> @@ -754,12 +761,17 @@ 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\treturn;\n> +\n> +\t\trequest = freeQueue_.dequeue();\n>  \t}\n>  \n> +\trequest->clearBuffers();\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..5a9701ff 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->clearBuffers();\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..b4e8a6d8 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -50,7 +50,7 @@ protected:\n>  \t\tif (request->status() != Request::RequestComplete)\n>  \t\t\treturn;\n>  \n> -\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\t\tconst Request::BufferMap buffers = request->buffers();\n>  \n>  \t\tcompleteRequestsCount_++;\n>  \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->clearBuffers();\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..0116481b 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -44,7 +44,7 @@ protected:\n>  \t\tif (request->status() != Request::RequestComplete)\n>  \t\t\treturn;\n>  \n> -\t\tconst Request::BufferMap &buffers = request->buffers();\n> +\t\tconst Request::BufferMap buffers = request->buffers();\n>  \n>  \t\tcompleteRequestsCount_++;\n>  \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->clearBuffers();\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> 2.27.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 27604C3B5B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 30 Sep 2020 10:39:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AA8B962211;\n\tWed, 30 Sep 2020 12:39:15 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5580660BD4\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 30 Sep 2020 12:39:13 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id u4so1126693ljd.10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 30 Sep 2020 03:39:13 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tb27sm146786lfq.133.2020.09.30.03.39.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 30 Sep 2020 03:39:11 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"zKY2eScz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=YJ72lU4W+VUWwbpJjy+1InxS3ws0YPX8eYqKLSnZhZs=;\n\tb=zKY2eSczpicuYr5Mqiq3DZoWDNBGV8ATMu7PnC252+65FKld4Gz69TO0BHHQMqbMM9\n\tqdlnOrN6EIu5cOPeJ/9XYCtkC+Migvv1Jw5Sj4qKNyfUgVHkZWW732CPJEppeR318rxj\n\tuih2QOpKX3+BXDjR2Cu4un9RatkWj3MjUsI+sLKMBmMihLXDAoP+6FUzCbw11tliUzfW\n\tzJWJpouUsxfgan6/8gdme2X3yIsdwkLaNNKRV3wErcniSisj1nKogtE8iXgCsFzO/TUA\n\thd2QUhk/dppSN7jsYEQfsIM+RoYE2zGywNXak1twvnnTr0u1ayG852xwNBQNJZrWxmNQ\n\twI6Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=YJ72lU4W+VUWwbpJjy+1InxS3ws0YPX8eYqKLSnZhZs=;\n\tb=egWTcKTpEUKb7Lx4cj9KoZ7X3Cxu0D8duezJOz0IM7cBEbqB8JeLcDwCZE6RoOwn7K\n\t5N9K3qS4MQG8sA0r4Orq0pxmOz4qiEdnhJQna9VAtaEa5RdSe09TYSe+47K93hsY+Pv4\n\trsDHLPVVFZ3iIyIn+XuCwoPAlnWqX+7QVadScO77XiiB3qeYMKEpfq5FKHaNAy8EcXMD\n\tf/k6q1hZor1JevVITeqEXYBBrZ+f7QSc7TNvhfwtYx0pE37eR2lUDnxrneV7svdi90ZZ\n\tRUMX1INTWdZ86N3gja93iauA4uxxLxHPe3EmQ+tcgqh3JZcNxSRGdNU5y1yGwjjOnHtp\n\tNJsg==","X-Gm-Message-State":"AOAM533EJm1J+LfUlMyGwTcUBHpKcgY+i6mPm3A+uSvutsvmrDZZcvri\n\tb7vdROSb9iyRhqLJLrYnHi9evg==","X-Google-Smtp-Source":"ABdhPJzN109o1C3/rLqHGckQaO96wGDPOTcpmW9LujAKlS6anu9lYY0tcEvMGER2e73Nch68Zffqsw==","X-Received":"by 2002:a2e:b610:: with SMTP id r16mr730076ljn.226.1601462352195;\n\tWed, 30 Sep 2020 03:39:12 -0700 (PDT)","Date":"Wed, 30 Sep 2020 12:39:11 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Paul Elder <paul.elder@ideasonboard.com>","Message-ID":"<20200930103911.GD1516931@oden.dyn.berto.se>","References":"<20200930101717.422619-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930101717.422619-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] 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=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":12930,"web_url":"https://patchwork.libcamera.org/comment/12930/","msgid":"<20201002031858.GX3722@pendragon.ideasonboard.com>","date":"2020-10-02T03:18:58","subject":"Re: [libcamera-devel] [PATCH v3] 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 and Niklas,\n\nGlad to see this nearing completion :-)\n\nOn Wed, Sep 30, 2020 at 12:39:11PM +0200, Niklas Söderlund wrote:\n> On 2020-09-30 19:17:17 +0900, Paul Elder wrote:\n> > Allow reuse of the Request object by implementing reset(),\n> > clearBuffers(), and reAddBuffers().  This means that the applications\n> > now have the responsibility of freeing the Request objects, so make all\n> > libcamera users (cam, qcam, v4l2-compat, gstreamer, android) do so.\n> > \n> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>\n> > \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       |  4 ++\n> >  src/android/camera_device.cpp     |  8 +++-\n> >  src/cam/capture.cpp               | 29 ++++----------\n> >  src/cam/capture.h                 |  3 ++\n> >  src/gstreamer/gstlibcamerasrc.cpp |  6 ++-\n> >  src/libcamera/camera.cpp          | 19 ++++-----\n> >  src/libcamera/request.cpp         | 64 +++++++++++++++++++++++++++++++\n> >  src/qcam/main_window.cpp          | 48 ++++++++++++++---------\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     | 15 ++++----\n> >  test/camera/capture.cpp           | 15 ++++----\n> >  test/camera/statemachine.cpp      |  9 ++---\n> >  15 files changed, 186 insertions(+), 104 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 272c12c3..e4036b63 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -93,7 +93,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..6a651f36 100644\n> > --- a/include/libcamera/request.h\n> > +++ b/include/libcamera/request.h\n> > @@ -38,6 +38,10 @@ public:\n> >  \tRequest &operator=(const Request &) = delete;\n> >  \t~Request();\n> >  \n> > +\tvoid reset();\n> > +\tvoid clearBuffers();\n> > +\tvoid reAddBuffers();\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 70d77a17..7f288617 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1395,8 +1395,13 @@ 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> r =\n> >  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n> > +\tRequest *request = r.release();\n\nA comment would be good here:\n\n\t/* The request will be deleted in the completion handler. */\n\tRequest *request = r.release();\n\nI wonder if we should try to allocate a pool of requests and reuse them.\nProbably not worth it, and not a candidate for this patch anyway.\n\nI would actually move the r.release() just before calling\n->queueRequest(). Something along the lines of\n\n\tstd::unique_ptr<Request> request =\n  \t\tcamera_->createRequest(reinterpret_cast<uint64_t>(descriptor));\n\n> > +\tif (!request) {\n> > +\t\tLOG(HAL, Error) << \"Failed to create request\";\n> > +\t\treturn -ENOMEM;\n> > +\t}\n\nThe code here and below stays the same, but with\n\n-\t\tdelete request;\n\nin the error path. Then,\n\n\tint ret = camera_->queueRequest(request.get());\n\tif (ret) {\n\t\tLOG(HAL, Error) << \"Failed to queue request\";\n\t\tdelete descriptor;\n\t\treturn ret;\n \t}\n\n\t/* The request will be freed by the completion handler. */\n\trequest.release();\n\n> >  \n> >  \tfor (unsigned int i = 0; i < descriptor->numBuffers; ++i) {\n> >  \t\tCameraStream *cameraStream =\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..5b223365 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->reAddBuffers();\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..ca223df6 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,7 +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> > +\tstd::unique_ptr<Request> request_unique = state->cam_->createRequest();\n> > +\tRequest *request = request_unique.release();\n\nCould you take into account the above comments for the HAL and apply\nthem here ?\n\n> >  \tauto wrap = std::make_unique<RequestWrap>(request);\n> >  \tfor (GstPad *srcpad : state->srcpads_) {\n> >  \t\tGstLibcameraPool *pool = gst_libcamera_pad_get_pool(srcpad);\n> > @@ -281,7 +284,6 @@ gst_libcamera_src_task_run(gpointer user_data)\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\tbreak;\n> >  \t\t}\n> >  \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index ae16a64a..d4c18c72 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -833,21 +833,25 @@ 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 either queueing the request or deleting it. The caller\n\nThe previous sentence isn't correct anymore, as queing the request won't\ncause it to be deleted.\n\n> > + * continues to own the request after queueing it, so the application is\n> > + * responsible for deleting the request after the request completes.\n> > + * Alternatively, the request may be reused after clearing its buffers via\n> > + * Request::clearBuffers().\n\nYou could write\n\n * The ownership of the returned request is passed to the caller, which is\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> >   *\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> > @@ -863,9 +867,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> > @@ -877,6 +878,8 @@ Request *Camera::createRequest(uint64_t cookie)\n> >   */\n> >  int Camera::queueRequest(Request *request)\n> >  {\n> > +\trequest->reset();\n> > +\n> >  \tint ret = p_->isAccessAllowed(Private::CameraRunning);\n> >  \tif (ret < 0)\n> >  \t\treturn ret;\n> > @@ -974,13 +977,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..7d51e504 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -85,6 +85,70 @@ Request::~Request()\n> >  \tdelete validator_;\n> >  }\n> >  \n> > +/*\n> > + * \\brief Reset the request\n> > + *\n> > + * Reset the status and controls associated with the request, to allow it to\n> > + * be reused and requeued without destruction. This function is meant to be\n> > + * called by Camera::queueRequest().\n> > + *\n> > + * \\todo Make this private and make Camera a friend class?\n> > + */\n> > +void Request::reset()\n> > +{\n> > +\tstatus_ = RequestPending;\n> > +\tcancelled_ = false;\n> > +\n> > +\tdelete metadata_;\n> > +\tdelete controls_;\n> > +\tdelete validator_;\n> > +\n> > +\tvalidator_ = new CameraControlValidator(camera_);\n> > +\tcontrols_ = new ControlList(controls::controls, validator_);\n> > +\tmetadata_ = new ControlList(controls::controls);\n> > +}\n> > +\n> > +/*\n> > + * \\brief Clear buffers associated with the request\n> > + *\n> > + * Delete the pointers to the FrameBuffers that were added to this request via\n> > + * addBuffers().\n> > + *\n> > + * This function should be called in (or after) the request completion handler\n> > + * in applications prior to modifying the buffers and requeueing the request to\n> > + * the camera. If the buffers do not need to be modified, and are to be reused,\n> > + * use reAddBuffers(). If the request is meant to be transient, then it\n> > + * should be deleted at the end of the completion handler.\n> > + */\n> > +void Request::clearBuffers()\n> > +{\n> > +\tpending_.clear();\n> > +\n> > +\tbufferMap_.clear();\n> > +}\n> > +\n> > +/*\n> > + * \\brief Re-add the buffers that were added to the request\n> > + *\n> > + * Re-add the FrameBuffers that were added to this request via addBuffers().\n> > + *\n> > + * This function should be called in (or after) the request completion handler\n> > + * in applications that wish to reuse the buffers that were added to the request\n> > + * when it was queued to the camera. If the buffers need to be modified, they\n> > + * should be cleared with clearBuffers() first instead. If the request is\n> > + * meant to be transient, then it should be deleted at the end of the\n> > + * completion handler.\n> > + */\n> > +void Request::reAddBuffers()\n> > +{\n> > +\tpending_.clear();\n> > +\n> > +\tfor (auto pair : bufferMap_) {\n> > +\t\tpair.second->request_ = this;\n> > +\t\tpending_.insert(pair.second);\n> > +\t}\n> > +}\n> \n> I wonder would it not be neat to call this unconditionally in \n> PipelineHandler::completeRequest()?\n> \n> An application wishing to reuse a request can then simply just requeue \n> it without touching it. While an application wishing to modify the \n> request before reusing it will be responsible for removeAllBuffers() or \n> removeBuffer(Stream *) the Request to fit it's new usage.\n\nI think the API is indeed a bit convoluted, but I don't think we should\ncall Request::reAddBuffers() implicitly. It would open the door to bugs\nif an application forgets to call clearBuffers().\n\nI would actually merge those three functions together, and add an\nargument to tell whether to reuse buffers or not. The argument should be\nan enum value from the Request class, and have a default value of not\nreusing buffers. Applications would then always have to call\nRequest::reset() or Request::reset(Request::ReuseBuffers) explicitly,\nand if they don't, we can easily catch it in Camera::queueRequest() by\nchecking the request status.\n\nFeel free to propose alternatives or additional improvements. The goal\nis to have an API that is optimized for the common case (not reusing\nbuffers), intuitive, simple to use correctly, and difficult to use\nincorrectly (catching the error in Camera::queueRequest() in this case,\nalthough if you can find a better way I'm all ears, but I doubt we can\ncatch the issue at compile time).\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 985743f3..466f3053 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> > @@ -725,11 +724,19 @@ void MainWindow::processCapture()\n> >  \t}\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\t{\n> > +\t\t\tQMutexLocker locker(&mutex_);\n> > +\t\t\tfreeQueue_.enqueue(request);\n> > +\t\t}\n\nThis seems weird. Shouldn't the request always be added back to the\nfreeQueue_, not only if it contains a buffer for the viewfinder stream ?\nShouldn't you move this block to the end of the function ?\n\n> > +\t\tRequest::BufferMap buffers = request->buffers();\n\nI don't think you need to copy the buffer map, it's only needed if you\nwant to use it after calling request->clearBuffers().\n\n> > +\t\tprocessViewfinder(buffers[vfStream_]);\n> > +\t}\n> >  \n> > -\tif (request.buffers_.count(rawStream_))\n> > -\t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> > +\tif (request->buffers().count(rawStream_)) {\n> > +\t\tRequest::BufferMap buffers = request->buffers();\n\nSame here.\n\n> > +\t\tprocessRaw(buffers[rawStream_], request->metadata());\n> > +\t}\n> >  }\n> >  \n> >  void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > @@ -754,12 +761,17 @@ 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\treturn;\n\nThis should never happen, right ? I'd keep the qWarning().\n\n> > +\n> > +\t\trequest = freeQueue_.dequeue();\n> >  \t}\n> >  \n> > +\trequest->clearBuffers();\n\nI think I'd move clearBuffers() to the completion handler (or to be\nprecise when you're done with the request, before adding it to the\nfreeQueue_) if possible, to keep the request in a clean state after it\ncompletes.\n\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..5a9701ff 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->clearBuffers();\n\nSame here, clearBuffer() in the completion handler if possible would be\nbest.\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..b4e8a6d8 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -50,7 +50,7 @@ protected:\n> >  \t\tif (request->status() != Request::RequestComplete)\n> >  \t\t\treturn;\n> >  \n> > -\t\tconst Request::BufferMap &buffers = request->buffers();\n> > +\t\tconst Request::BufferMap buffers = request->buffers();\n\nI don't think you need this, as you extract the FrameBuffer pointer\nbefore calling clearBuffers(), and you don't use buffers after that.\n\n> >  \n> >  \t\tcompleteRequestsCount_++;\n> >  \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->clearBuffers();\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..0116481b 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -44,7 +44,7 @@ protected:\n> >  \t\tif (request->status() != Request::RequestComplete)\n> >  \t\t\treturn;\n> >  \n> > -\t\tconst Request::BufferMap &buffers = request->buffers();\n> > +\t\tconst Request::BufferMap buffers = request->buffers();\n\nDitto.\n\n> >  \n> >  \t\tcompleteRequestsCount_++;\n> >  \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->clearBuffers();\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\nThe tests look good, but could you run them under valgrind to make sure\nwe don't leak anything ?","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 CE1EEC3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Oct 2020 03:19:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BD226300B;\n\tFri,  2 Oct 2020 05:19:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7750D6229F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Oct 2020 05:19:36 +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 C287B2A2;\n\tFri,  2 Oct 2020 05:19:35 +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=\"lSy00Mrz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601608776;\n\tbh=GaTO6zv7djudjfl4nnwB6D5s040wJgzG+CItulrJLVM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lSy00MrzoM9ik+PeGO9nZ3n8S6RNcFjl7jDaOGTr+3JWXdyzmcbyVPLW+xSSNCaB7\n\tGymgYtGggtcWm/dLxBr2Mw+n1vHxh0VJdvV+Sk28NfLd6uPZqOyl9jZKRlx1DHue+n\n\tfvxCIda4/b52rKopqvJE1/24yj4Kd+txGcreq2rM=","Date":"Fri, 2 Oct 2020 06:18:58 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Message-ID":"<20201002031858.GX3722@pendragon.ideasonboard.com>","References":"<20200930101717.422619-1-paul.elder@ideasonboard.com>\n\t<20200930103911.GD1516931@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930103911.GD1516931@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3] 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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]