[{"id":12883,"web_url":"https://patchwork.libcamera.org/comment/12883/","msgid":"<20200929224651.GA1516931@oden.dyn.berto.se>","date":"2020-09-29T22:46:51","subject":"Re: [libcamera-devel] [PATCH v2] 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-29 18:52:00 +0900, Paul Elder wrote:\n> Allow reuse of the Request object by implementing a reset() function.\n> This means that the applications now have the responsibility of freeing\n> the Request objects, so make all libcamera users (cam, qcam,\n> v4l2-compat, gstreamer, android) do so.\n> \n> Signed-off-by: Paul Elder \n> \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> - I haven't yet tested android\n> ---\n> include/libcamera/camera.h | 2 +-\n> include/libcamera/request.h | 2 ++\n> src/android/camera_device.cpp | 8 +++++-\n> src/cam/capture.cpp | 19 ++++++------\n> src/cam/capture.h | 3 ++\n> src/gstreamer/gstlibcamerasrc.cpp | 6 ++--\n> src/libcamera/camera.cpp | 15 ++++------\n> src/libcamera/request.cpp | 24 ++++++++++++++++\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, 141 insertions(+), 93 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 generateConfiguration(const StreamRoles &roles = {});\n> \tint configure(CameraConfiguration *config);\n> \n> -\tRequest *createRequest(uint64_t cookie = 0);\n> +\tstd::unique_ptr createRequest(uint64_t cookie = 0);\n\nI like this.\n\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..9f615a24 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();\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 r =\n> \t\tcamera_->createRequest(reinterpret_cast(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\nI don't like this ;-)\n\nWould it not make sens to leverage the unique_ptr?\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..5b9238e2 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 requests;\n> \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> -\t\tRequest *request = camera_->createRequest();\n> +\t\tstd::unique_ptr 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 : requests_) {\n> +\t\tret = camera_->queueRequest(request.get());\n\nWould it not make sens to take this all the way and have \nCamera::queueRequest() take a unique pointer that is returned in \nrequestComplete(). Then the application could store the unique ptr and \nreuse it or let it expire and have the request deleted?\n\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> \t\t\tcamera_->stop();\n> @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)\n> \tif (request->status() == Request::RequestCancelled)\n> \t\treturn;\n> \n> -\tconst Request::BufferMap &buffers = request->buffers();\n> +\tconst Request::BufferMap buffers = request->buffers();\n> \n> \t/*\n> \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)\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> +\trequest->reset();\n> \n> \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> \t\tconst Stream *stream = it->first;\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 \n> #include \n> +#include \n> \n> #include \n> #include \n> @@ -43,6 +44,8 @@ private:\n> \tEventLoop *loop_;\n> \tunsigned int captureCount_;\n> \tunsigned int captureLimit_;\n> +\n> +\tstd::vector> 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_unique = state->cam_->createRequest();\n> +\tRequest *request = request_unique.release();\n> \tauto wrap = std::make_unique(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..8c2bbc76 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -833,21 +833,23 @@ 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 deleting the request\n> + * is the responsibility of the application after the request completes.\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 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(this, cookie);\n> }\n> \n> /**\n> @@ -863,9 +865,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> @@ -974,13 +973,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..dd3040fe 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -85,6 +85,30 @@ Request::~Request()\n> \tdelete validator_;\n> }\n> \n> +/*\n> + * \\brief Reset the request\n> + *\n> + * Reset the request, to allow it to be reused and requeued without\n> + * destruction. All contents of the request will be reset, so any references\n> + * previously returned by the request will become invalid.\n> + */\n\nWould it make sens to split this function in two? One that resets the \nstatus_, cancelled_, metadata_ and any output controls and one that \nclears the buffers.\n\nThe reset of output parameters could be called by Camera::queueRequest() \nand the clearBuffers() would be called optionally by applications.\n\nThat way applications would be able to both reuse a request by either by \nkeeping all buffer associations or clearing them and repopulating them \nwith new buffers. Also by clearing output parameters in the framework \napplications will be unable to get it wrong.\n\n> +void Request::reset()\n> +{\n> +\tbufferMap_.clear();\n> +\tpending_.clear();\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> * \\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..c2f8b175 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 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 = 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 : 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->reset();\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 \n> +#include \n> \n> #include \n> #include \n> @@ -22,6 +23,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \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> freeBuffers_;\n> -\tQQueue doneQueue_;\n> -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> +\tQQueue doneQueue_;\n> +\tQQueue 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> 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 = 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 &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 =\n> -\t\tstd::unique_ptr(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> \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> pendingRequests_;\n> +\tstd::vector> requestPool_;\n> +\n> +\tstd::deque pendingRequests_;\n> \tstd::deque> completedBuffers_;\n> \n> \tint efd_;\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 64e96264..9cfdb388 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->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 requests;\n> \t\tfor (const std::unique_ptr &buffer : source.buffers()) {\n> -\t\t\tRequest *request = camera_->createRequest();\n> +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> +\n> \tunsigned int completeBuffersCount_;\n> \tunsigned int completeRequestsCount_;\n> \tstd::unique_ptr config_;\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index 51bbd258..5c86c490 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->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 requests;\n> \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> -\t\t\tRequest *request = camera_->createRequest();\n> +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> +\n> \tstd::unique_ptr 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 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 = 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":"","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 3B006C3B5C\n\tfor ;\n\tTue, 29 Sep 2020 22:46:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AFFC6621C9;\n\tWed, 30 Sep 2020 00:46:55 +0200 (CEST)","from mail-lf1-x134.google.com (mail-lf1-x134.google.com\n\t[IPv6:2a00:1450:4864:20::134])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 299EE60366\n\tfor ;\n\tWed, 30 Sep 2020 00:46:54 +0200 (CEST)","by mail-lf1-x134.google.com with SMTP id y11so7494015lfl.5\n\tfor ;\n\tTue, 29 Sep 2020 15:46:54 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t78sm3426318lfn.20.2020.09.29.15.46.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 29 Sep 2020 15:46:52 -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=\"UEu1IjNM\"; 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=nFgz8wVGcQu67gpK0ZM2p+ml0lMWTFai9qgg7I1uNkI=;\n\tb=UEu1IjNMdIR8Q0932YpBgS91fPCwE+bTF7R4D4TTe6McPGf5SfrWG+EiOES13eTw0A\n\tVXDzuIKQXqNnOY2na0k8NZ5Z/oplykVh7nBPV1OfXAOKa7noy7wxuMyy/CKzIboqGHSk\n\txspEEZFtGIoBb5tCwLjaMq7YQuqDA9SbSiIdp6gYWtCz+e8tdkSYNzHi7rewfV1mIurc\n\tZbytmFD+IMWEMUSqc6KBKw1O/Nso2bxJjkENGu1SL+A7cengCT4D8OK0Th4HhYe3lyjn\n\tLy5b1EJM+wTIw/3wUtNTq9qaIHAbXepn5O7cCXUE7YRS9q8ATp97mld99BkZeKYsnl70\n\tcOsQ==","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=nFgz8wVGcQu67gpK0ZM2p+ml0lMWTFai9qgg7I1uNkI=;\n\tb=gbP2hRb+HL5wl+wfsNwbOIreOhf5vePs4rPVD5O3Clh/ijyns7eQ0xTZXXZXtCYUAK\n\tYv4NI5rc6g2mj4bY6RLu4xiHC/tg7VIh1/OkBeOWXoItgAyvVVo6dLjaPjVfsq7tlQwG\n\toKNkkUAgB9s6aOCVVk+6NExb0ybmkk8F5eDS608weNy2pTMUof6j2kSQcLp+cGuW0x79\n\tSq74L/YGpaUp3fvqimzYBhkhpofO8J6VPhgMSN3QzTuBgGLU8+f6c+I8NPOPN0eEpgYW\n\tlXOYKOBRo+i3+lnPqky5zednXxVliw70bsk17zlUV8clKOx6EEHquS5VN4o6/tR2oavv\n\tsbGg==","X-Gm-Message-State":"AOAM5328h5bU+nkMVnfgC16hbfEBw5JjNpI8lNqmYlxk3Lh6wkREBVSW\n\tSn0PCCEIdvljZ3Es8Qan8zQ4KPEctiPeEA==","X-Google-Smtp-Source":"ABdhPJys4JO8QMBiG7tcIVnMarzsf/xhBXy0fSC6PW40veXKsCTRtmQjAy5SO6w3hLr/9kl1Q9bl9w==","X-Received":"by 2002:ac2:5298:: with SMTP id\n\tq24mr1872372lfm.164.1601419613039; \n\tTue, 29 Sep 2020 15:46:53 -0700 (PDT)","Date":"Wed, 30 Sep 2020 00:46:51 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"<20200929224651.GA1516931@oden.dyn.berto.se>","References":"<20200929095200.396199-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929095200.396199-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":12903,"web_url":"https://patchwork.libcamera.org/comment/12903/","msgid":"<20200930095507.GW45948@pyrite.rasen.tech>","date":"2020-09-30T09:55:07","subject":"Re: [libcamera-devel] [PATCH v2] 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 Niklas,\n\nThank you for the review.\n\nOn Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:\n> Hi Paul,\n> \n> Thanks for your work.\n> \n> On 2020-09-29 18:52:00 +0900, Paul Elder wrote:\n> > Allow reuse of the Request object by implementing a reset() function.\n> > This means that the applications now have the responsibility of freeing\n> > the Request objects, so make all libcamera users (cam, qcam,\n> > v4l2-compat, gstreamer, android) do so.\n> > \n> > Signed-off-by: Paul Elder \n> > \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> > - I haven't yet tested android\n> > ---\n> > include/libcamera/camera.h | 2 +-\n> > include/libcamera/request.h | 2 ++\n> > src/android/camera_device.cpp | 8 +++++-\n> > src/cam/capture.cpp | 19 ++++++------\n> > src/cam/capture.h | 3 ++\n> > src/gstreamer/gstlibcamerasrc.cpp | 6 ++--\n> > src/libcamera/camera.cpp | 15 ++++------\n> > src/libcamera/request.cpp | 24 ++++++++++++++++\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, 141 insertions(+), 93 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 generateConfiguration(const StreamRoles &roles = {});\n> > \tint configure(CameraConfiguration *config);\n> > \n> > -\tRequest *createRequest(uint64_t cookie = 0);\n> > +\tstd::unique_ptr createRequest(uint64_t cookie = 0);\n> \n> I like this.\n\n:)\n\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..9f615a24 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();\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 r =\n> > \t\tcamera_->createRequest(reinterpret_cast(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> I don't like this ;-)\n\n:(\n\n> Would it not make sens to leverage the unique_ptr?\n\nFrom what I could tell, android doesn't requeue the request in the\nrequest completion handler (CameraDevice::requestComplete()), which\nmeans that android uses the request transiently, like how request was\nmeant to be used before (no reuse). So I kept it that way.\n\nOr do you mean that I should let the unique pointer die on its own by\nletting it go out of scope? Wouldn't that free the request its pointing\nto, and segfault somewhere between this and the completion handler?\n\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..5b9238e2 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 requests;\n> > \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > -\t\tRequest *request = camera_->createRequest();\n> > +\t\tstd::unique_ptr 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 : requests_) {\n> > +\t\tret = camera_->queueRequest(request.get());\n> \n> Would it not make sens to take this all the way and have \n> Camera::queueRequest() take a unique pointer that is returned in \n> requestComplete(). Then the application could store the unique ptr and \n> reuse it or let it expire and have the request deleted?\n\nThe problem with queueRequest() taking a unique pointer is that the\ncamera then owns the request... but it can't give ownership back to the\napplication since the completion handler signal can have multiple slots.\nWe can't give all of them a unique pointer, but if we give them a\nregular pointer, then whose responsibility is it for freeing the\nrequest? So we make the application keep ownership of the request. If it\nrelinquishes ownership then it can free it at the end of the completion\nhandler (like android and gstreamer does now).\n\n> > \t\tif (ret < 0) {\n> > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > \t\t\tcamera_->stop();\n> > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)\n> > \tif (request->status() == Request::RequestCancelled)\n> > \t\treturn;\n> > \n> > -\tconst Request::BufferMap &buffers = request->buffers();\n> > +\tconst Request::BufferMap buffers = request->buffers();\n> > \n> > \t/*\n> > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)\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> > +\trequest->reset();\n> > \n> > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > \t\tconst Stream *stream = it->first;\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 \n> > #include \n> > +#include \n> > \n> > #include \n> > #include \n> > @@ -43,6 +44,8 @@ private:\n> > \tEventLoop *loop_;\n> > \tunsigned int captureCount_;\n> > \tunsigned int captureLimit_;\n> > +\n> > +\tstd::vector> 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_unique = state->cam_->createRequest();\n> > +\tRequest *request = request_unique.release();\n> > \tauto wrap = std::make_unique(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..8c2bbc76 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -833,21 +833,23 @@ 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 deleting the request\n> > + * is the responsibility of the application after the request completes.\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 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(this, cookie);\n> > }\n> > \n> > /**\n> > @@ -863,9 +865,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> > @@ -974,13 +973,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..dd3040fe 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -85,6 +85,30 @@ Request::~Request()\n> > \tdelete validator_;\n> > }\n> > \n> > +/*\n> > + * \\brief Reset the request\n> > + *\n> > + * Reset the request, to allow it to be reused and requeued without\n> > + * destruction. All contents of the request will be reset, so any references\n> > + * previously returned by the request will become invalid.\n> > + */\n> \n> Would it make sens to split this function in two? One that resets the \n> status_, cancelled_, metadata_ and any output controls and one that \n> clears the buffers.\n> \n> The reset of output parameters could be called by Camera::queueRequest() \n> and the clearBuffers() would be called optionally by applications.\n> \n> That way applications would be able to both reuse a request by either by \n> keeping all buffer associations or clearing them and repopulating them \n> with new buffers. Also by clearing output parameters in the framework \n> applications will be unable to get it wrong.\n\nOh interesting idea.\n\nOkay, I did a reset() that's called in Camera::queueRequest(), that\nresets status_, cancelled_, metadata_, controls_, and validator_. Then a\nclearBuffers() that nukes the buffer map. And a reAddBuffers() that\ntakes the current buffer map, and reattaches the request to the buffers\n(otherwise we get a segfault on buffer->request_ since completeBuffer()\nnulls buffer->request_ and there's no addBuffer() to reattach it).\n\nAnd so the apps that:\n- still use transient request (android, gstreamer)\n - can just delete it at the end of the completion handler\n- the apps the want to reuse the buffers (cam)\n - can just call request->reAddBuffers() at the end of the completion\n handler\n- and apps that need to change up their buffers (qcam, v4l2)\n - can do so at the end of the completion handler, with no special\n calls\n\nSee v3 for details :)\n\n\nThanks,\n\nPaul\n\n> > +void Request::reset()\n> > +{\n> > +\tbufferMap_.clear();\n> > +\tpending_.clear();\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> > * \\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..c2f8b175 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 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 = 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 : 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->reset();\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 \n> > +#include \n> > \n> > #include \n> > #include \n> > @@ -22,6 +23,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \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> freeBuffers_;\n> > -\tQQueue doneQueue_;\n> > -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> > +\tQQueue doneQueue_;\n> > +\tQQueue 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> 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 = 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 &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 =\n> > -\t\tstd::unique_ptr(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> > \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> pendingRequests_;\n> > +\tstd::vector> requestPool_;\n> > +\n> > +\tstd::deque pendingRequests_;\n> > \tstd::deque> completedBuffers_;\n> > \n> > \tint efd_;\n> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > index 64e96264..9cfdb388 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->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 requests;\n> > \t\tfor (const std::unique_ptr &buffer : source.buffers()) {\n> > -\t\t\tRequest *request = camera_->createRequest();\n> > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > +\n> > \tunsigned int completeBuffersCount_;\n> > \tunsigned int completeRequestsCount_;\n> > \tstd::unique_ptr config_;\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index 51bbd258..5c86c490 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->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 requests;\n> > \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> > -\t\t\tRequest *request = camera_->createRequest();\n> > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > +\n> > \tstd::unique_ptr 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 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 = 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":"","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 5524BC3B5B\n\tfor ;\n\tWed, 30 Sep 2020 09:55:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C57D66220F;\n\tWed, 30 Sep 2020 11:55:17 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4AF106035F\n\tfor ;\n\tWed, 30 Sep 2020 11:55:16 +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 791BC329;\n\tWed, 30 Sep 2020 11:55:14 +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=\"mZtrOHzo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601459716;\n\tbh=QfYfEaxuQqzcSAmYoTDRxV9XTUrYMcLSWipQf8TeExU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mZtrOHzoIOGtNBxlQtCEsLMujlbSZSa6Ocpxmx/XbQwe+U4CzeA3imHLfKRyyOSvN\n\toot44S7Z+60Ue9FESGGUvAGWxKP+wFT6NfGGATQykR3c+W72qvDX5AKCTWGbzDkTiV\n\tXwniDUqp8RPHWxL046bwvyTsWqOYlUFubS6qP+OQ=","Date":"Wed, 30 Sep 2020 18:55:07 +0900","From":"paul.elder@ideasonboard.com","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Message-ID":"<20200930095507.GW45948@pyrite.rasen.tech>","References":"<20200929095200.396199-1-paul.elder@ideasonboard.com>\n\t<20200929224651.GA1516931@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929224651.GA1516931@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":12904,"web_url":"https://patchwork.libcamera.org/comment/12904/","msgid":"<20200930095908.GX45948@pyrite.rasen.tech>","date":"2020-09-30T09:59:08","subject":"Re: [libcamera-devel] [PATCH v2] 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":"On Wed, Sep 30, 2020 at 06:55:07PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Niklas,\n> \n> Thank you for the review.\n> \n> On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:\n> > Hi Paul,\n> > \n> > Thanks for your work.\n> > \n> > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:\n> > > Allow reuse of the Request object by implementing a reset() function.\n> > > This means that the applications now have the responsibility of freeing\n> > > the Request objects, so make all libcamera users (cam, qcam,\n> > > v4l2-compat, gstreamer, android) do so.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \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> > > - I haven't yet tested android\n> > > ---\n> > > include/libcamera/camera.h | 2 +-\n> > > include/libcamera/request.h | 2 ++\n> > > src/android/camera_device.cpp | 8 +++++-\n> > > src/cam/capture.cpp | 19 ++++++------\n> > > src/cam/capture.h | 3 ++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 6 ++--\n> > > src/libcamera/camera.cpp | 15 ++++------\n> > > src/libcamera/request.cpp | 24 ++++++++++++++++\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, 141 insertions(+), 93 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 generateConfiguration(const StreamRoles &roles = {});\n> > > \tint configure(CameraConfiguration *config);\n> > > \n> > > -\tRequest *createRequest(uint64_t cookie = 0);\n> > > +\tstd::unique_ptr createRequest(uint64_t cookie = 0);\n> > \n> > I like this.\n> \n> :)\n> \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..9f615a24 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();\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 r =\n> > > \t\tcamera_->createRequest(reinterpret_cast(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> > I don't like this ;-)\n> \n> :(\n> \n> > Would it not make sens to leverage the unique_ptr?\n> \n> From what I could tell, android doesn't requeue the request in the\n> request completion handler (CameraDevice::requestComplete()), which\n> means that android uses the request transiently, like how request was\n> meant to be used before (no reuse). So I kept it that way.\n> \n> Or do you mean that I should let the unique pointer die on its own by\n> letting it go out of scope? Wouldn't that free the request its pointing\n> to, and segfault somewhere between this and the completion handler?\n> \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..5b9238e2 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 requests;\n> > > \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > > -\t\tRequest *request = camera_->createRequest();\n> > > +\t\tstd::unique_ptr 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 : requests_) {\n> > > +\t\tret = camera_->queueRequest(request.get());\n> > \n> > Would it not make sens to take this all the way and have \n> > Camera::queueRequest() take a unique pointer that is returned in \n> > requestComplete(). Then the application could store the unique ptr and \n> > reuse it or let it expire and have the request deleted?\n> \n> The problem with queueRequest() taking a unique pointer is that the\n> camera then owns the request... but it can't give ownership back to the\n> application since the completion handler signal can have multiple slots.\n> We can't give all of them a unique pointer, but if we give them a\n> regular pointer, then whose responsibility is it for freeing the\n> request? So we make the application keep ownership of the request. If it\n> relinquishes ownership then it can free it at the end of the completion\n> handler (like android and gstreamer does now).\n> \n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > \t\t\tcamera_->stop();\n> > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)\n> > > \tif (request->status() == Request::RequestCancelled)\n> > > \t\treturn;\n> > > \n> > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > +\tconst Request::BufferMap buffers = request->buffers();\n> > > \n> > > \t/*\n> > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)\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> > > +\trequest->reset();\n> > > \n> > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > \t\tconst Stream *stream = it->first;\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 \n> > > #include \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -43,6 +44,8 @@ private:\n> > > \tEventLoop *loop_;\n> > > \tunsigned int captureCount_;\n> > > \tunsigned int captureLimit_;\n> > > +\n> > > +\tstd::vector> 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_unique = state->cam_->createRequest();\n> > > +\tRequest *request = request_unique.release();\n> > > \tauto wrap = std::make_unique(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..8c2bbc76 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -833,21 +833,23 @@ 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 deleting the request\n> > > + * is the responsibility of the application after the request completes.\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 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(this, cookie);\n> > > }\n> > > \n> > > /**\n> > > @@ -863,9 +865,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> > > @@ -974,13 +973,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..dd3040fe 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -85,6 +85,30 @@ Request::~Request()\n> > > \tdelete validator_;\n> > > }\n> > > \n> > > +/*\n> > > + * \\brief Reset the request\n> > > + *\n> > > + * Reset the request, to allow it to be reused and requeued without\n> > > + * destruction. All contents of the request will be reset, so any references\n> > > + * previously returned by the request will become invalid.\n> > > + */\n> > \n> > Would it make sens to split this function in two? One that resets the \n> > status_, cancelled_, metadata_ and any output controls and one that \n> > clears the buffers.\n> > \n> > The reset of output parameters could be called by Camera::queueRequest() \n> > and the clearBuffers() would be called optionally by applications.\n> > \n> > That way applications would be able to both reuse a request by either by \n> > keeping all buffer associations or clearing them and repopulating them \n> > with new buffers. Also by clearing output parameters in the framework \n> > applications will be unable to get it wrong.\n> \n> Oh interesting idea.\n> \n> Okay, I did a reset() that's called in Camera::queueRequest(), that\n> resets status_, cancelled_, metadata_, controls_, and validator_. Then a\n> clearBuffers() that nukes the buffer map. And a reAddBuffers() that\n> takes the current buffer map, and reattaches the request to the buffers\n> (otherwise we get a segfault on buffer->request_ since completeBuffer()\n> nulls buffer->request_ and there's no addBuffer() to reattach it).\n> \n> And so the apps that:\n> - still use transient request (android, gstreamer)\n> - can just delete it at the end of the completion handler\n> - the apps the want to reuse the buffers (cam)\n> - can just call request->reAddBuffers() at the end of the completion\n> handler\n> - and apps that need to change up their buffers (qcam, v4l2)\n> - can do so at the end of the completion handler, with no special\n> calls\n\nFor this last category, clearBuffers() is required before changing the\nbuffers.\n\n\nPaul\n\n> \n> See v3 for details :)\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > > +void Request::reset()\n> > > +{\n> > > +\tbufferMap_.clear();\n> > > +\tpending_.clear();\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> > > * \\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..c2f8b175 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 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 = 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 : 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->reset();\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 \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -22,6 +23,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \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> freeBuffers_;\n> > > -\tQQueue doneQueue_;\n> > > -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> > > +\tQQueue doneQueue_;\n> > > +\tQQueue 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> 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 = 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 &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 =\n> > > -\t\tstd::unique_ptr(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> > > \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> pendingRequests_;\n> > > +\tstd::vector> requestPool_;\n> > > +\n> > > +\tstd::deque pendingRequests_;\n> > > \tstd::deque> completedBuffers_;\n> > > \n> > > \tint efd_;\n> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > > index 64e96264..9cfdb388 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->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 requests;\n> > > \t\tfor (const std::unique_ptr &buffer : source.buffers()) {\n> > > -\t\t\tRequest *request = camera_->createRequest();\n> > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > +\n> > > \tunsigned int completeBuffersCount_;\n> > > \tunsigned int completeRequestsCount_;\n> > > \tstd::unique_ptr config_;\n> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > > index 51bbd258..5c86c490 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->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 requests;\n> > > \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> > > -\t\t\tRequest *request = camera_->createRequest();\n> > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > +\n> > > \tstd::unique_ptr 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 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 = 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\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","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 40231C3B5B\n\tfor ;\n\tWed, 30 Sep 2020 09:59:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C21E16220F;\n\tWed, 30 Sep 2020 11:59: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 8A6326035F\n\tfor ;\n\tWed, 30 Sep 2020 11:59:23 +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 5C09D329;\n\tWed, 30 Sep 2020 11:59:13 +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=\"DLA5Via8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601459957;\n\tbh=fwAuABXV7hGKD2ZSTa4XTxm2aIuQCKiaTmW57p7lNkQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DLA5Via8/l2+WSYVGoKLRaRfp19UrNFuTChFZY2djqn+EmvsPZqu9p3j17nKTUPSo\n\tZNS93blr/rQKjERTJEDE7oGfaIAMFntSOrdGGhw+aQc9+EiUYZcgH2R3HV66oesQDf\n\tH/ofdx9/wdEX7pElJ656HJzDjXQQy3DdG2ViYUao=","Date":"Wed, 30 Sep 2020 18:59:08 +0900","From":"paul.elder@ideasonboard.com","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Message-ID":"<20200930095908.GX45948@pyrite.rasen.tech>","References":"<20200929095200.396199-1-paul.elder@ideasonboard.com>\n\t<20200929224651.GA1516931@oden.dyn.berto.se>\n\t<20200930095507.GW45948@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930095507.GW45948@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":12906,"web_url":"https://patchwork.libcamera.org/comment/12906/","msgid":"<20200930103318.GC1516931@oden.dyn.berto.se>","date":"2020-09-30T10:33:18","subject":"Re: [libcamera-devel] [PATCH v2] 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\nI know you posted a v3 but I wanted to continue one discussion point and \nto keep the context I reply here. Everything applies to v3 non the less.\n\nOn 2020-09-30 18:55:07 +0900, paul.elder@ideasonboard.com wrote:\n> Hi Niklas,\n> \n> Thank you for the review.\n> \n> On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:\n> > Hi Paul,\n> > \n> > Thanks for your work.\n> > \n> > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:\n> > > Allow reuse of the Request object by implementing a reset() function.\n> > > This means that the applications now have the responsibility of freeing\n> > > the Request objects, so make all libcamera users (cam, qcam,\n> > > v4l2-compat, gstreamer, android) do so.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \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> > > - I haven't yet tested android\n> > > ---\n> > > include/libcamera/camera.h | 2 +-\n> > > include/libcamera/request.h | 2 ++\n> > > src/android/camera_device.cpp | 8 +++++-\n> > > src/cam/capture.cpp | 19 ++++++------\n> > > src/cam/capture.h | 3 ++\n> > > src/gstreamer/gstlibcamerasrc.cpp | 6 ++--\n> > > src/libcamera/camera.cpp | 15 ++++------\n> > > src/libcamera/request.cpp | 24 ++++++++++++++++\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, 141 insertions(+), 93 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 generateConfiguration(const StreamRoles &roles = {});\n> > > \tint configure(CameraConfiguration *config);\n> > > \n> > > -\tRequest *createRequest(uint64_t cookie = 0);\n> > > +\tstd::unique_ptr createRequest(uint64_t cookie = 0);\n> > \n> > I like this.\n> \n> :)\n> \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..9f615a24 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();\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 r =\n> > > \t\tcamera_->createRequest(reinterpret_cast(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> > I don't like this ;-)\n> \n> :(\n> \n> > Would it not make sens to leverage the unique_ptr?\n> \n> From what I could tell, android doesn't requeue the request in the\n> request completion handler (CameraDevice::requestComplete()), which\n> means that android uses the request transiently, like how request was\n> meant to be used before (no reuse). So I kept it that way.\n> \n> Or do you mean that I should let the unique pointer die on its own by\n> letting it go out of scope? Wouldn't that free the request its pointing\n> to, and segfault somewhere between this and the completion handler?\n\nI meant letting the unique pointer go out of scope after the \nrequestComplete handler have completed. Reading your reply bellow I \nunderstand this connects into that discussion so I will leave this one \nfor now.\n\n> \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..5b9238e2 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 requests;\n> > > \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > > -\t\tRequest *request = camera_->createRequest();\n> > > +\t\tstd::unique_ptr 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 : requests_) {\n> > > +\t\tret = camera_->queueRequest(request.get());\n> > \n> > Would it not make sens to take this all the way and have \n> > Camera::queueRequest() take a unique pointer that is returned in \n> > requestComplete(). Then the application could store the unique ptr and \n> > reuse it or let it expire and have the request deleted?\n> \n> The problem with queueRequest() taking a unique pointer is that the\n> camera then owns the request... but it can't give ownership back to the\n> application since the completion handler signal can have multiple slots.\n> We can't give all of them a unique pointer, but if we give them a\n> regular pointer, then whose responsibility is it for freeing the\n> request? So we make the application keep ownership of the request. If it\n> relinquishes ownership then it can free it at the end of the completion\n> handler (like android and gstreamer does now).\n\nWe have touched upon this limitation before, maybe we can solve the \nissue by finding a way to limit the requestComplete() callback to a \nsingle slot? I think it's such a neat idea to have the ownership of a \nRequest be handed between application and libcamera in this fashion.\n\nWould it be possible to extend our singal/slot to express a special type \nof single subscriber signal?\n\n> \n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > \t\t\tcamera_->stop();\n> > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)\n> > > \tif (request->status() == Request::RequestCancelled)\n> > > \t\treturn;\n> > > \n> > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > +\tconst Request::BufferMap buffers = request->buffers();\n> > > \n> > > \t/*\n> > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)\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> > > +\trequest->reset();\n> > > \n> > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > \t\tconst Stream *stream = it->first;\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 \n> > > #include \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -43,6 +44,8 @@ private:\n> > > \tEventLoop *loop_;\n> > > \tunsigned int captureCount_;\n> > > \tunsigned int captureLimit_;\n> > > +\n> > > +\tstd::vector> 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_unique = state->cam_->createRequest();\n> > > +\tRequest *request = request_unique.release();\n> > > \tauto wrap = std::make_unique(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..8c2bbc76 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -833,21 +833,23 @@ 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 deleting the request\n> > > + * is the responsibility of the application after the request completes.\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 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(this, cookie);\n> > > }\n> > > \n> > > /**\n> > > @@ -863,9 +865,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> > > @@ -974,13 +973,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..dd3040fe 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -85,6 +85,30 @@ Request::~Request()\n> > > \tdelete validator_;\n> > > }\n> > > \n> > > +/*\n> > > + * \\brief Reset the request\n> > > + *\n> > > + * Reset the request, to allow it to be reused and requeued without\n> > > + * destruction. All contents of the request will be reset, so any references\n> > > + * previously returned by the request will become invalid.\n> > > + */\n> > \n> > Would it make sens to split this function in two? One that resets the \n> > status_, cancelled_, metadata_ and any output controls and one that \n> > clears the buffers.\n> > \n> > The reset of output parameters could be called by Camera::queueRequest() \n> > and the clearBuffers() would be called optionally by applications.\n> > \n> > That way applications would be able to both reuse a request by either by \n> > keeping all buffer associations or clearing them and repopulating them \n> > with new buffers. Also by clearing output parameters in the framework \n> > applications will be unable to get it wrong.\n> \n> Oh interesting idea.\n> \n> Okay, I did a reset() that's called in Camera::queueRequest(), that\n> resets status_, cancelled_, metadata_, controls_, and validator_. Then a\n> clearBuffers() that nukes the buffer map. And a reAddBuffers() that\n> takes the current buffer map, and reattaches the request to the buffers\n> (otherwise we get a segfault on buffer->request_ since completeBuffer()\n> nulls buffer->request_ and there's no addBuffer() to reattach it).\n> \n> And so the apps that:\n> - still use transient request (android, gstreamer)\n> - can just delete it at the end of the completion handler\n> - the apps the want to reuse the buffers (cam)\n> - can just call request->reAddBuffers() at the end of the completion\n> handler\n> - and apps that need to change up their buffers (qcam, v4l2)\n> - can do so at the end of the completion handler, with no special\n> calls\n> \n> See v3 for details :)\n\nNice!\n\n> \n> \n> Thanks,\n> \n> Paul\n> \n> > > +void Request::reset()\n> > > +{\n> > > +\tbufferMap_.clear();\n> > > +\tpending_.clear();\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> > > * \\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..c2f8b175 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 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 = 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 : 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->reset();\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 \n> > > +#include \n> > > \n> > > #include \n> > > #include \n> > > @@ -22,6 +23,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \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> freeBuffers_;\n> > > -\tQQueue doneQueue_;\n> > > -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> > > +\tQQueue doneQueue_;\n> > > +\tQQueue 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> 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 = 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 &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 =\n> > > -\t\tstd::unique_ptr(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> > > \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> pendingRequests_;\n> > > +\tstd::vector> requestPool_;\n> > > +\n> > > +\tstd::deque pendingRequests_;\n> > > \tstd::deque> completedBuffers_;\n> > > \n> > > \tint efd_;\n> > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > > index 64e96264..9cfdb388 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->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 requests;\n> > > \t\tfor (const std::unique_ptr &buffer : source.buffers()) {\n> > > -\t\t\tRequest *request = camera_->createRequest();\n> > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > +\n> > > \tunsigned int completeBuffersCount_;\n> > > \tunsigned int completeRequestsCount_;\n> > > \tstd::unique_ptr config_;\n> > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > > index 51bbd258..5c86c490 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->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 requests;\n> > > \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> > > -\t\t\tRequest *request = camera_->createRequest();\n> > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > +\n> > > \tstd::unique_ptr 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 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 = 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":"","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 02ED6C3B5C\n\tfor ;\n\tWed, 30 Sep 2020 10:33:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62E2962215;\n\tWed, 30 Sep 2020 12:33:22 +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 0CBA760BD4\n\tfor ;\n\tWed, 30 Sep 2020 12:33:21 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id k25so1116935ljg.9\n\tfor ;\n\tWed, 30 Sep 2020 03:33:20 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tv11sm146773lfg.39.2020.09.30.03.33.18\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 30 Sep 2020 03:33:18 -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=\"SNIbJC+c\"; 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=nyEmLsP2kd0812YULIsVQUXRfw1NT9ChfsEtPlH07dg=;\n\tb=SNIbJC+cqeNggBbwGzx+k5bVGJpM2t0EudFX2738RTYldETE8EmJ+sryuU/gDZt4hu\n\tG8Z9aGQkm2Dnid7gt/aBJ5zGPk5CLp0gbvdrgqz7GH4j1FcKamIaYclOwT3k0Gew+V1d\n\tlSFjpLVevyND+hU0/CUQQm8bD2BTlIjDhDFfuHVD8cBv6It9mGVl/n8PrHxAvu8NmNIn\n\twvsajEpab9gaxuuy8W1TOoVCgf7EMuaWRhVknv/Z6TaVOrBm8a8ETzVm3g5vNT8WTWgA\n\tfJy0NaAFC4DAQnMJwic9NwjtEAlOEEOX6IDJZFMd9Esdvzod59gllG8E5aVy4/g3esQK\n\tiDdA==","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=nyEmLsP2kd0812YULIsVQUXRfw1NT9ChfsEtPlH07dg=;\n\tb=AP9C9NUtJozqe0fhe9U2MVvbv6+z8htwgPN6+50EyB86swcW+JR8xbwv3iguz5byYl\n\t2U2vjLDepYu461c6oTDMQ07kjdJKCgWudUFQV3rWD5srTYAWGE/TYU2D0XuJBEeXmLqQ\n\tYnlOGJybszgLTXPdkrGBROnzYPxPzAuczYnIgbdP0Gz29mE23LDbFBFno6+vCIOkTdrV\n\tYR6grpmOb9z5Tbt5aJj1ttLKJuUY8PW6lmyjesSzXSfveBVq64UcTWYi296iiOQ7oRo4\n\tKi0yGKnZOGsDkVj3zeXufcqCE4N3ZyJJ1s8MkgSj3hlr7DbQ0bWTXO4rrUXnFhbJy6uX\n\t6E+g==","X-Gm-Message-State":"AOAM532upERzHSX6jdYQz01Cy7svMaUq6PTnYM9JMQ8n0Ot9FrZ4Fdf3\n\txl3kecQpWAwB+vyMoAFHjJFnzpCVbhblMw==","X-Google-Smtp-Source":"ABdhPJx+i37Nunj79uKBgfv3nuk11zTHoLTwLS16Hgl8JrEHxhzAAqPDbojO+a4Wey027T+sY2luWg==","X-Received":"by 2002:a2e:5357:: with SMTP id t23mr616962ljd.31.1601461999048; \n\tWed, 30 Sep 2020 03:33:19 -0700 (PDT)","Date":"Wed, 30 Sep 2020 12:33:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"paul.elder@ideasonboard.com","Message-ID":"<20200930103318.GC1516931@oden.dyn.berto.se>","References":"<20200929095200.396199-1-paul.elder@ideasonboard.com>\n\t<20200929224651.GA1516931@oden.dyn.berto.se>\n\t<20200930095507.GW45948@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930095507.GW45948@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}},{"id":12929,"web_url":"https://patchwork.libcamera.org/comment/12929/","msgid":"<20201002025557.GE45948@pyrite.rasen.tech>","date":"2020-10-02T02:55:57","subject":"Re: [libcamera-devel] [PATCH v2] 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 Niklas,\n\nOn Wed, Sep 30, 2020 at 12:33:18PM +0200, Niklas Söderlund wrote:\n> Hi Paul,\n> \n> I know you posted a v3 but I wanted to continue one discussion point and \n> to keep the context I reply here. Everything applies to v3 non the less.\n> \n> On 2020-09-30 18:55:07 +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Niklas,\n> > \n> > Thank you for the review.\n> > \n> > On Wed, Sep 30, 2020 at 12:46:51AM +0200, Niklas Söderlund wrote:\n> > > Hi Paul,\n> > > \n> > > Thanks for your work.\n> > > \n> > > On 2020-09-29 18:52:00 +0900, Paul Elder wrote:\n> > > > Allow reuse of the Request object by implementing a reset() function.\n> > > > This means that the applications now have the responsibility of freeing\n> > > > the Request objects, so make all libcamera users (cam, qcam,\n> > > > v4l2-compat, gstreamer, android) do so.\n> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > \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> > > > - I haven't yet tested android\n> > > > ---\n> > > > include/libcamera/camera.h | 2 +-\n> > > > include/libcamera/request.h | 2 ++\n> > > > src/android/camera_device.cpp | 8 +++++-\n> > > > src/cam/capture.cpp | 19 ++++++------\n> > > > src/cam/capture.h | 3 ++\n> > > > src/gstreamer/gstlibcamerasrc.cpp | 6 ++--\n> > > > src/libcamera/camera.cpp | 15 ++++------\n> > > > src/libcamera/request.cpp | 24 ++++++++++++++++\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, 141 insertions(+), 93 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 generateConfiguration(const StreamRoles &roles = {});\n> > > > \tint configure(CameraConfiguration *config);\n> > > > \n> > > > -\tRequest *createRequest(uint64_t cookie = 0);\n> > > > +\tstd::unique_ptr createRequest(uint64_t cookie = 0);\n> > > \n> > > I like this.\n> > \n> > :)\n> > \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..9f615a24 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();\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 r =\n> > > > \t\tcamera_->createRequest(reinterpret_cast(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> > > I don't like this ;-)\n> > \n> > :(\n> > \n> > > Would it not make sens to leverage the unique_ptr?\n> > \n> > From what I could tell, android doesn't requeue the request in the\n> > request completion handler (CameraDevice::requestComplete()), which\n> > means that android uses the request transiently, like how request was\n> > meant to be used before (no reuse). So I kept it that way.\n> > \n> > Or do you mean that I should let the unique pointer die on its own by\n> > letting it go out of scope? Wouldn't that free the request its pointing\n> > to, and segfault somewhere between this and the completion handler?\n> \n> I meant letting the unique pointer go out of scope after the \n> requestComplete handler have completed. Reading your reply bellow I \n> understand this connects into that discussion so I will leave this one \n> for now.\n> \n> > \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..5b9238e2 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 requests;\n> > > > \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > > > -\t\tRequest *request = camera_->createRequest();\n> > > > +\t\tstd::unique_ptr 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 : requests_) {\n> > > > +\t\tret = camera_->queueRequest(request.get());\n> > > \n> > > Would it not make sens to take this all the way and have \n> > > Camera::queueRequest() take a unique pointer that is returned in \n> > > requestComplete(). Then the application could store the unique ptr and \n> > > reuse it or let it expire and have the request deleted?\n> > \n> > The problem with queueRequest() taking a unique pointer is that the\n> > camera then owns the request... but it can't give ownership back to the\n> > application since the completion handler signal can have multiple slots.\n> > We can't give all of them a unique pointer, but if we give them a\n> > regular pointer, then whose responsibility is it for freeing the\n> > request? So we make the application keep ownership of the request. If it\n> > relinquishes ownership then it can free it at the end of the completion\n> > handler (like android and gstreamer does now).\n> \n> We have touched upon this limitation before, maybe we can solve the \n> issue by finding a way to limit the requestComplete() callback to a \n> single slot? I think it's such a neat idea to have the ownership of a \n> Request be handed between application and libcamera in this fashion.\n\nIf I remember correctly, the limitation is because BoundMethodMember\nneeds to make a copy of the parameters to the slot, and that doesn't\nwork well with unique_ptr.\n\n> Would it be possible to extend our singal/slot to express a special type \n> of single subscriber signal?\n\n...maybe? Like class SignalSingle : public SignalBase which keeps a\ncounter of how many slots are connected, and limit it to 1? We'd still\nhave to extend BoundMethodMember though, to use std::move instead of\nstd::copy...\n\nConclusion: not now :)\n\n\nPaul\n\n> > \n> > > > \t\tif (ret < 0) {\n> > > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > > \t\t\tcamera_->stop();\n> > > > @@ -156,7 +157,7 @@ void Capture::requestComplete(Request *request)\n> > > > \tif (request->status() == Request::RequestCancelled)\n> > > > \t\treturn;\n> > > > \n> > > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > > +\tconst Request::BufferMap buffers = request->buffers();\n> > > > \n> > > > \t/*\n> > > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > > @@ -206,11 +207,7 @@ void Capture::requestComplete(Request *request)\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> > > > +\trequest->reset();\n> > > > \n> > > > \tfor (auto it = buffers.begin(); it != buffers.end(); ++it) {\n> > > > \t\tconst Stream *stream = it->first;\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 \n> > > > #include \n> > > > +#include \n> > > > \n> > > > #include \n> > > > #include \n> > > > @@ -43,6 +44,8 @@ private:\n> > > > \tEventLoop *loop_;\n> > > > \tunsigned int captureCount_;\n> > > > \tunsigned int captureLimit_;\n> > > > +\n> > > > +\tstd::vector> 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_unique = state->cam_->createRequest();\n> > > > +\tRequest *request = request_unique.release();\n> > > > \tauto wrap = std::make_unique(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..8c2bbc76 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -833,21 +833,23 @@ 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 deleting the request\n> > > > + * is the responsibility of the application after the request completes.\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 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(this, cookie);\n> > > > }\n> > > > \n> > > > /**\n> > > > @@ -863,9 +865,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> > > > @@ -974,13 +973,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..dd3040fe 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -85,6 +85,30 @@ Request::~Request()\n> > > > \tdelete validator_;\n> > > > }\n> > > > \n> > > > +/*\n> > > > + * \\brief Reset the request\n> > > > + *\n> > > > + * Reset the request, to allow it to be reused and requeued without\n> > > > + * destruction. All contents of the request will be reset, so any references\n> > > > + * previously returned by the request will become invalid.\n> > > > + */\n> > > \n> > > Would it make sens to split this function in two? One that resets the \n> > > status_, cancelled_, metadata_ and any output controls and one that \n> > > clears the buffers.\n> > > \n> > > The reset of output parameters could be called by Camera::queueRequest() \n> > > and the clearBuffers() would be called optionally by applications.\n> > > \n> > > That way applications would be able to both reuse a request by either by \n> > > keeping all buffer associations or clearing them and repopulating them \n> > > with new buffers. Also by clearing output parameters in the framework \n> > > applications will be unable to get it wrong.\n> > \n> > Oh interesting idea.\n> > \n> > Okay, I did a reset() that's called in Camera::queueRequest(), that\n> > resets status_, cancelled_, metadata_, controls_, and validator_. Then a\n> > clearBuffers() that nukes the buffer map. And a reAddBuffers() that\n> > takes the current buffer map, and reattaches the request to the buffers\n> > (otherwise we get a segfault on buffer->request_ since completeBuffer()\n> > nulls buffer->request_ and there's no addBuffer() to reattach it).\n> > \n> > And so the apps that:\n> > - still use transient request (android, gstreamer)\n> > - can just delete it at the end of the completion handler\n> > - the apps the want to reuse the buffers (cam)\n> > - can just call request->reAddBuffers() at the end of the completion\n> > handler\n> > - and apps that need to change up their buffers (qcam, v4l2)\n> > - can do so at the end of the completion handler, with no special\n> > calls\n> > \n> > See v3 for details :)\n> \n> Nice!\n> \n> > \n> > \n> > Thanks,\n> > \n> > Paul\n> > \n> > > > +void Request::reset()\n> > > > +{\n> > > > +\tbufferMap_.clear();\n> > > > +\tpending_.clear();\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> > > > * \\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..c2f8b175 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 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 = 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 : 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->reset();\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 \n> > > > +#include \n> > > > \n> > > > #include \n> > > > #include \n> > > > @@ -22,6 +23,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \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> freeBuffers_;\n> > > > -\tQQueue doneQueue_;\n> > > > -\tQMutex mutex_; /* Protects freeBuffers_ and doneQueue_ */\n> > > > +\tQQueue doneQueue_;\n> > > > +\tQQueue 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> 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 = 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 &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 =\n> > > > -\t\tstd::unique_ptr(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> > > > \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> pendingRequests_;\n> > > > +\tstd::vector> requestPool_;\n> > > > +\n> > > > +\tstd::deque pendingRequests_;\n> > > > \tstd::deque> completedBuffers_;\n> > > > \n> > > > \tint efd_;\n> > > > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > > > index 64e96264..9cfdb388 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->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 requests;\n> > > > \t\tfor (const std::unique_ptr &buffer : source.buffers()) {\n> > > > -\t\t\tRequest *request = camera_->createRequest();\n> > > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > > +\n> > > > \tunsigned int completeBuffersCount_;\n> > > > \tunsigned int completeRequestsCount_;\n> > > > \tstd::unique_ptr config_;\n> > > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > > > index 51bbd258..5c86c490 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->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 requests;\n> > > > \t\tfor (const std::unique_ptr &buffer : allocator_->buffers(stream)) {\n> > > > -\t\t\tRequest *request = camera_->createRequest();\n> > > > +\t\t\tstd::unique_ptr 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 : 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> requests_;\n> > > > +\n> > > > \tstd::unique_ptr 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 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 = 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":"","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 7B24DC3B5B\n\tfor ;\n\tFri, 2 Oct 2020 02:56:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 050C6625AF;\n\tFri, 2 Oct 2020 04:56:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 351FF6229F\n\tfor ;\n\tFri, 2 Oct 2020 04:56:06 +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 39C9B9E7;\n\tFri, 2 Oct 2020 04:56:03 +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=\"Yu6kMTkV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601607365;\n\tbh=GAy9wU294jUCYSRHLl/BuZN0+4e+1ou9KzdwDD5kcpk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yu6kMTkVxSUTBxjEcRWJN7pSNeuf5vaz4iuJ3KrmW5Yx6ialCvzoVuoWmo6Kyqf1N\n\tVn+mIG3VWH8YyPw8sZ2zXAZ4xbWtLavLeJh/4FDOwBezkVZ+pMs+MYeN3FpSqrYn68\n\tmJ1cbbObkRNHkz1XipC5iv3ffMzSkEimi1XnFmUU=","Date":"Fri, 2 Oct 2020 11:55:57 +0900","From":"paul.elder@ideasonboard.com","To":"Niklas =?iso-8859-1?q?S=F6derlund?= ","Message-ID":"<20201002025557.GE45948@pyrite.rasen.tech>","References":"<20200929095200.396199-1-paul.elder@ideasonboard.com>\n\t<20200929224651.GA1516931@oden.dyn.berto.se>\n\t<20200930095507.GW45948@pyrite.rasen.tech>\n\t<20200930103318.GC1516931@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930103318.GC1516931@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v2] 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":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","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\" "}}]