[{"id":12725,"web_url":"https://patchwork.libcamera.org/comment/12725/","msgid":"<20200924120951.GD502216@oden.dyn.berto.se>","date":"2020-09-24T12:09:51","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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 patch.\n\nOn 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> In this RFC, I've only made qcam and cam reuse the Request object. Just\n> gauging the direction, and then I'll put it in for andoird and gstreamer\n> and v4l2-compat as well.\n> ---\n> include/libcamera/request.h | 2 ++\n> src/cam/capture.cpp | 20 ++++++++++----------\n> src/cam/capture.h | 3 +++\n> src/libcamera/camera.cpp | 4 +---\n> src/libcamera/request.cpp | 9 +++++++++\n> src/qcam/main_window.cpp | 26 ++++++++++++--------------\n> src/qcam/main_window.h | 14 ++++++++++----\n> src/qcam/viewfinder.h | 4 +++-\n> src/qcam/viewfinder_gl.cpp | 9 +++++----\n> src/qcam/viewfinder_gl.h | 5 +++--\n> src/qcam/viewfinder_qt.cpp | 10 ++++++----\n> src/qcam/viewfinder_qt.h | 8 ++++++--\n> 12 files changed, 70 insertions(+), 44 deletions(-)\n> \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/cam/capture.cpp b/src/cam/capture.cpp\n> index 5510c009..fe9d591a 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n> \t\twriter_ = nullptr;\n> \t}\n> \n> +\tfor (Request *req : requests_)\n> +\t\tdelete req;\n> +\n> \tdelete allocator;\n> \n> \treturn ret;\n> @@ -92,7 +95,6 @@ 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\tif (!request) {\n> @@ -117,7 +119,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(request);\n> \t}\n> \n> \tret = camera_->start();\n> @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> \t\treturn ret;\n> \t}\n> \n> -\tfor (Request *request : requests) {\n> +\tfor (Request *request : requests_) {\n> \t\tret = camera_->queueRequest(request);\n> \t\tif (ret < 0) {\n> \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> \n> void Capture::requestComplete(Request *request)\n> {\n> -\tif (request->status() == Request::RequestCancelled)\n> +\tif (request->status() == Request::RequestCancelled) {\n> +\t\trequest->reset();\n> \t\treturn;\n> +\t}\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 +210,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\nHum, is this really \"reusing\" the request object? I imagined a reused \nrequest would retain its buffers while gaining the ability to be be \nrequeued to the Camera after it have completed. With this patch we still \nneed to keep the logic to fill the request with buffers each iteration, \nis there some advantage to this I'm missing?\n\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..62cf9bf1 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/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index ae16a64a..4cc68bce 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -974,13 +974,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..606866a6 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -85,6 +85,15 @@ Request::~Request()\n> \tdelete validator_;\n> }\n> \n> +void Request::reset()\n> +{\n> +\tbufferMap_.clear();\n> +\tpending_.clear();\n> +\n> +\tstatus_ = RequestPending;\n> +\tcancelled_ = false;\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..1531b44f 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> @@ -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(request);\n> \t}\n> \n> \t/* Start the title timer and the camera. */\n> @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> \n> \t/* Queue all requests. */\n> -\tfor (Request *request : requests) {\n> +\tfor (Request *request : requests_) {\n> \t\tret = camera_->queueRequest(request);\n> \t\tif (ret < 0) {\n> \t\t\tqWarning() << \"Can't queue request\";\n> @@ -535,7 +534,7 @@ error_disconnect:\n> \tcamera_->stop();\n> \n> error:\n> -\tfor (Request *request : requests)\n> +\tfor (Request *request : requests_)\n> \t\tdelete request;\n> \n> \tfor (auto &iter : mappedBuffers_) {\n> @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n> \t}\n> \tmappedBuffers_.clear();\n> \n> +\tfor (Request *req : requests_)\n> +\t\tdelete req;\n> +\n> \tdelete allocator_;\n> \n> \tisCapturing_ = false;\n> @@ -701,7 +703,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->buffers(), request->metadata(), request });\n> \t}\n> \n> \tQCoreApplication::postEvent(this, new CaptureEvent);\n> @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n> \n> \t/* Process buffers. */\n> \tif (request.buffers_.count(vfStream_))\n> -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n> \n> \tif (request.buffers_.count(rawStream_))\n> \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> }\n> \n> -void MainWindow::processViewfinder(FrameBuffer *buffer)\n> +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n> {\n> \tframesCaptured_++;\n> \n> @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> \n> \t/* Render the frame on the viewfinder. */\n> -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n> +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n> }\n> \n> -void MainWindow::queueRequest(FrameBuffer *buffer)\n> +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n> {\n> -\tRequest *request = camera_->createRequest();\n> -\tif (!request) {\n> -\t\tqWarning() << \"Can't create request\";\n> -\t\treturn;\n> -\t}\n> +\trequest->reset();\n> \n> \trequest->addBuffer(vfStream_, buffer);\n> \n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 5c61a4df..83535373 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> @@ -49,13 +51,15 @@ public:\n> \t}\n> \n> \tCaptureRequest(const Request::BufferMap &buffers,\n> -\t\t const ControlList &metadata)\n> -\t\t: buffers_(buffers), metadata_(metadata)\n> +\t\t const ControlList &metadata,\n> +\t\t Request * const request)\n> +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n> \t{\n> \t}\n> \n> \tRequest::BufferMap buffers_;\n> \tControlList metadata_;\n> +\tRequest *request_;\n> };\n> \n> class MainWindow : public QMainWindow\n> @@ -79,7 +83,7 @@ private Q_SLOTS:\n> \tvoid captureRaw();\n> \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n> \n> -\tvoid queueRequest(FrameBuffer *buffer);\n> +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n> \n> private:\n> \tint createToolbars();\n> @@ -96,7 +100,7 @@ private:\n> \tvoid requestComplete(Request *request);\n> \tvoid processCapture();\n> \tvoid processHotplug(HotplugEvent *e);\n> -\tvoid processViewfinder(FrameBuffer *buffer);\n> +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n> \n> \t/* UI elements */\n> \tQToolBar *toolbar_;\n> @@ -135,6 +139,8 @@ private:\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/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index 67da1df2..26c20f1b 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -27,7 +27,9 @@ public:\n> \tvirtual const QList &nativeFormats() const = 0;\n> \n> \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> +\tvirtual void render(libcamera::Request *request,\n> +\t\t\t libcamera::FrameBuffer *buffer,\n> +\t\t\t MappedBuffer *map) = 0;\n> \tvirtual void stop() = 0;\n> \n> \tvirtual QImage getCurrentImage() = 0;\n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index fbe21dcf..c65b41f6 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -23,7 +23,7 @@ static const QList supportedFormats{\n> };\n> \n> ViewFinderGL::ViewFinderGL(QWidget *parent)\n> -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n> +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n> \t fragmentShader_(nullptr), vertexShader_(nullptr),\n> \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n> \t textureU_(QOpenGLTexture::Target2D),\n> @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> void ViewFinderGL::stop()\n> {\n> \tif (buffer_) {\n> -\t\trenderComplete(buffer_);\n> +\t\trenderComplete(request_, buffer_);\n> \t\tbuffer_ = nullptr;\n> \t}\n> }\n> @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n> \treturn grabFramebuffer();\n> }\n> \n> -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> {\n> \tif (buffer->planes().size() != 1) {\n> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \t}\n> \n> \tif (buffer_)\n> -\t\trenderComplete(buffer_);\n> +\t\trenderComplete(request_, buffer_);\n> \n> \tyuvData_ = static_cast(map->memory);\n> \tupdate();\n> \tbuffer_ = buffer;\n> +\trequest_ = request;\n> }\n> \n> bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> index 69502b7a..7db41841 100644\n> --- a/src/qcam/viewfinder_gl.h\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -36,13 +36,13 @@ public:\n> \tconst QList &nativeFormats() const override;\n> \n> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> \tvoid stop() override;\n> \n> \tQImage getCurrentImage() override;\n> \n> Q_SIGNALS:\n> -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> \n> protected:\n> \tvoid initializeGL() override;\n> @@ -60,6 +60,7 @@ private:\n> \tvoid doRender();\n> \n> \t/* Captured image size, format and buffer */\n> +\tlibcamera::Request *request_;\n> \tlibcamera::FrameBuffer *buffer_;\n> \tlibcamera::PixelFormat format_;\n> \tQSize size_;\n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index e436714c..7f9f913b 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -34,7 +34,7 @@ static const QMap nativeFormats\n> };\n> \n> ViewFinderQt::ViewFinderQt(QWidget *parent)\n> -\t: QWidget(parent), buffer_(nullptr)\n> +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n> {\n> \ticon_ = QIcon(\":camera-off.svg\");\n> }\n> @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> \treturn 0;\n> }\n> \n> -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> +void ViewFinderQt::render(libcamera::Request *request,\n> +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> {\n> \tif (buffer->planes().size() != 1) {\n> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \t\t\t\t\tsize / size_.height(),\n> \t\t\t\t\t::nativeFormats[format_]);\n> \t\t\tstd::swap(buffer, buffer_);\n> +\t\t\tstd::swap(request, request_);\n> \t\t} else {\n> \t\t\t/*\n> \t\t\t * Otherwise, convert the format and release the frame\n> @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \tupdate();\n> \n> \tif (buffer)\n> -\t\trenderComplete(buffer);\n> +\t\trenderComplete(request, buffer);\n> }\n> \n> void ViewFinderQt::stop()\n> @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n> \timage_ = QImage();\n> \n> \tif (buffer_) {\n> -\t\trenderComplete(buffer_);\n> +\t\trenderComplete(request_, buffer_);\n> \t\tbuffer_ = nullptr;\n> \t}\n> \n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> index d7554288..5f62d7f6 100644\n> --- a/src/qcam/viewfinder_qt.h\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -17,6 +17,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \"format_converter.h\"\n> #include \"viewfinder.h\"\n> @@ -32,13 +33,15 @@ public:\n> \tconst QList &nativeFormats() const override;\n> \n> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> +\tvoid render(libcamera::Request *request,\n> +\t\t libcamera::FrameBuffer *buffer,\n> +\t\t MappedBuffer *map) override;\n> \tvoid stop() override;\n> \n> \tQImage getCurrentImage() override;\n> \n> Q_SIGNALS:\n> -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> \n> protected:\n> \tvoid paintEvent(QPaintEvent *) override;\n> @@ -56,6 +59,7 @@ private:\n> \tQPixmap pixmap_;\n> \n> \t/* Buffer and render image */\n> +\tlibcamera::Request *request_;\n> \tlibcamera::FrameBuffer *buffer_;\n> \tQImage image_;\n> \tQMutex mutex_; /* Prevent concurrent access to image_ */\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 DE7CFC3B5B\n\tfor ;\n\tThu, 24 Sep 2020 12:09:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B28262FE1;\n\tThu, 24 Sep 2020 14:09:55 +0200 (CEST)","from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 43B7F60362\n\tfor ;\n\tThu, 24 Sep 2020 14:09:54 +0200 (CEST)","by mail-lf1-x141.google.com with SMTP id y2so3592391lfy.10\n\tfor ;\n\tThu, 24 Sep 2020 05:09: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\tm10sm1904614lfo.184.2020.09.24.05.09.52\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 24 Sep 2020 05:09: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=\"uad/JwSR\"; 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=7u9vAd81Kkw5aBOSI+U8neWqOgcR0CI8PvyK4adeV3A=;\n\tb=uad/JwSR94R3lBf3ImU/IkRIQClMUNIM1DAxgvrhDIjLu03RTiiZ2zxTK9c88iVNKx\n\to0D/wGzS/BYQjyja69Pp+zg1cKpZD9VBXth73yEZaQa4x5/E9u5yIC7bizZ5lILTR2pC\n\tMB2f6ID3nTJZC6SiYPMHD+33b6MvQSx5X93zOJO9kOdJI3yzD96Fl2EOHwtRREbkmZgV\n\tpQ0u5O39WyNMnvPiO2Uni9UijicA0EtrrBFrAUsEq/VmtKtTtBhsxaAsM5XKxfXayS/A\n\t652qETFWe/M/CfAW0FHxkdzs4SHd7cQDCUF/k9SE31lqHF1dufI6LmKv/5zYmu4TdUV+\n\tBpqw==","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=7u9vAd81Kkw5aBOSI+U8neWqOgcR0CI8PvyK4adeV3A=;\n\tb=HBcK5+4fUtKuvrtvjd3zRGsggo6aet5sRqjD4IuWTl6skd8g2GS08yTCMRELN686c4\n\ty19QKVmwTTmgQOW3BymME3YuZ82JHX/RjdcRtxW15v8bvdEBkITaplDNimfwcnctm0EU\n\t+y71oKEt4JAbHaLRGbolRZ2ON4plbdWOr3ZAwT1fVIFzN7VvBZnUZLUpHTkTX7J+alGY\n\t5r46LewBrXcdBa+ym26+mw2z5R79hFET8ntFINyKIZctiRZ4jyBr1p0AR91a1BXHuBtR\n\tuMD1wdcxgyWDxAnA+QR6khazCyERBM8DDkURb9skceJIXqvuhxvVNbjFXmeLh7Y+FJjw\n\t049g==","X-Gm-Message-State":"AOAM531Fw3jk9l4mJLvDU4T9hHdNasnvQeEPnosFZtuvJ8nMPUZo0hqe\n\tne92YcvGcsi3+DZIi+wKG5NHSx+OPUu3Sg==","X-Google-Smtp-Source":"ABdhPJzSJLAE+Q+hNhUsm1+iaIDGBhOxBEqARouRMKQojc/GoG5DdSiaV+r7ykCQrkIdieKi3ED7aQ==","X-Received":"by 2002:a05:6512:3191:: with SMTP id\n\ti17mr197959lfe.460.1600949393209; \n\tThu, 24 Sep 2020 05:09:53 -0700 (PDT)","Date":"Thu, 24 Sep 2020 14:09:51 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Paul Elder ","Message-ID":"<20200924120951.GD502216@oden.dyn.berto.se>","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200923121448.281346-1-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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":12727,"web_url":"https://patchwork.libcamera.org/comment/12727/","msgid":"<20200924124056.GD3968@pendragon.ideasonboard.com>","date":"2020-09-24T12:40:56","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: Reuse Request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:\n> On 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > In this RFC, I've only made qcam and cam reuse the Request object. Just\n> > gauging the direction, and then I'll put it in for andoird and gstreamer\n> > and v4l2-compat as well.\n> > ---\n> > include/libcamera/request.h | 2 ++\n> > src/cam/capture.cpp | 20 ++++++++++----------\n> > src/cam/capture.h | 3 +++\n> > src/libcamera/camera.cpp | 4 +---\n> > src/libcamera/request.cpp | 9 +++++++++\n> > src/qcam/main_window.cpp | 26 ++++++++++++--------------\n> > src/qcam/main_window.h | 14 ++++++++++----\n> > src/qcam/viewfinder.h | 4 +++-\n> > src/qcam/viewfinder_gl.cpp | 9 +++++----\n> > src/qcam/viewfinder_gl.h | 5 +++--\n> > src/qcam/viewfinder_qt.cpp | 10 ++++++----\n> > src/qcam/viewfinder_qt.h | 8 ++++++--\n> > 12 files changed, 70 insertions(+), 44 deletions(-)\n> > \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/cam/capture.cpp b/src/cam/capture.cpp\n> > index 5510c009..fe9d591a 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n> > \t\twriter_ = nullptr;\n> > \t}\n> > \n> > +\tfor (Request *req : requests_)\n> > +\t\tdelete req;\n> > +\n> > \tdelete allocator;\n> > \n> > \treturn ret;\n> > @@ -92,7 +95,6 @@ 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\tif (!request) {\n> > @@ -117,7 +119,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(request);\n> > \t}\n> > \n> > \tret = camera_->start();\n> > @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > \t\treturn ret;\n> > \t}\n> > \n> > -\tfor (Request *request : requests) {\n> > +\tfor (Request *request : requests_) {\n> > \t\tret = camera_->queueRequest(request);\n> > \t\tif (ret < 0) {\n> > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > \n> > void Capture::requestComplete(Request *request)\n> > {\n> > -\tif (request->status() == Request::RequestCancelled)\n> > +\tif (request->status() == Request::RequestCancelled) {\n> > +\t\trequest->reset();\n> > \t\treturn;\n> > +\t}\n> > \n> > -\tconst Request::BufferMap &buffers = request->buffers();\n> > +\tconst Request::BufferMap buffers = request->buffers();\n\nA copy isn't very nice, but we can live with it for now.\n\n> > \n> > \t/*\n> > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > @@ -206,11 +210,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> Hum, is this really \"reusing\" the request object? I imagined a reused \n> request would retain its buffers while gaining the ability to be be \n> requeued to the Camera after it have completed. With this patch we still \n> need to keep the logic to fill the request with buffers each iteration, \n> is there some advantage to this I'm missing?\n\nTwo advantages, we don't free and reallocate the request needlessly, and\nit can be stored in the application and used after the request handler\ncompletes.\n\nI don't think retaining buffers by default is a good idea, in many cases\nwe'll have a different number of buffers that can be cycled through than\nthe number of allocated requests.\n\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..62cf9bf1 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/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index ae16a64a..4cc68bce 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -974,13 +974,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..606866a6 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -85,6 +85,15 @@ Request::~Request()\n> > \tdelete validator_;\n> > }\n> > \n\nNo documentation ? :-) You should also update the documentation of\nCamera::queueRequest(). Thinking about it, I think we should enforce\nownership of requests by the application by returning a\nstd::unique_ptr from Camera::createRequest(). We may even want\nto drop Camera::createRequest() completely, but that can be done in a\nseparate patch.\n\n> > +void Request::reset()\n> > +{\n> > +\tbufferMap_.clear();\n> > +\tpending_.clear();\n\nYou need to clear controls_ and metadata_ too.\n\n> > +\n> > +\tstatus_ = RequestPending;\n> > +\tcancelled_ = false;\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..1531b44f 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> > @@ -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(request);\n> > \t}\n> > \n> > \t/* Start the title timer and the camera. */\n> > @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n> > \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > \n> > \t/* Queue all requests. */\n> > -\tfor (Request *request : requests) {\n> > +\tfor (Request *request : requests_) {\n> > \t\tret = camera_->queueRequest(request);\n> > \t\tif (ret < 0) {\n> > \t\t\tqWarning() << \"Can't queue request\";\n> > @@ -535,7 +534,7 @@ error_disconnect:\n> > \tcamera_->stop();\n> > \n> > error:\n> > -\tfor (Request *request : requests)\n> > +\tfor (Request *request : requests_)\n> > \t\tdelete request;\n> > \n> > \tfor (auto &iter : mappedBuffers_) {\n> > @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n> > \t}\n> > \tmappedBuffers_.clear();\n> > \n> > +\tfor (Request *req : requests_)\n> > +\t\tdelete req;\n\nHave you tried stopping and restarting capture ? You delete all the\nrequests here, but the vector still contains the pointers. When capture\nis restarted I expect issue. A\n\n\trequests_.clear();\n\nis needed, and I would also turn it into a\nstd::vector> to avoid the manual delete loops.\nSame thing for the cam application (even if in that case capture can't\nbe stopped and restarted at the moment, but let's still do it right).\n\n> > +\n> > \tdelete allocator_;\n> > \n> > \tisCapturing_ = false;\n> > @@ -701,7 +703,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->buffers(), request->metadata(), request });\n\nYou aobut enqueuing the request only ? The reason to enqueue a copy of\nbuffers() and metadata() was because the request was deleted right after\nthis function returns, now that it's not the case anymore, we can just\npass the request around. The MainWindow::CaptureRequest class can be\nremoved.\n\n> > \t}\n> > \n> > \tQCoreApplication::postEvent(this, new CaptureEvent);\n> > @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n> > \n> > \t/* Process buffers. */\n> > \tif (request.buffers_.count(vfStream_))\n> > -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> > +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n> > \n> > \tif (request.buffers_.count(rawStream_))\n> > \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n\nInstead of passing the request to processViewfinder(), how about adding\nit to a free list here, and taking a request from the free list in\nMainWindow::queueRequest() ? That would avoid the changes to the\nviewfinder.\n\n> > }\n> > \n> > -void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n> > {\n> > \tframesCaptured_++;\n> > \n> > @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > \n> > \t/* Render the frame on the viewfinder. */\n> > -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n> > +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n> > }\n> > \n> > -void MainWindow::queueRequest(FrameBuffer *buffer)\n> > +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n> > {\n> > -\tRequest *request = camera_->createRequest();\n> > -\tif (!request) {\n> > -\t\tqWarning() << \"Can't create request\";\n> > -\t\treturn;\n> > -\t}\n> > +\trequest->reset();\n> > \n> > \trequest->addBuffer(vfStream_, buffer);\n> > \n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 5c61a4df..83535373 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> > @@ -49,13 +51,15 @@ public:\n> > \t}\n> > \n> > \tCaptureRequest(const Request::BufferMap &buffers,\n> > -\t\t const ControlList &metadata)\n> > -\t\t: buffers_(buffers), metadata_(metadata)\n> > +\t\t const ControlList &metadata,\n> > +\t\t Request * const request)\n> > +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n> > \t{\n> > \t}\n> > \n> > \tRequest::BufferMap buffers_;\n> > \tControlList metadata_;\n> > +\tRequest *request_;\n> > };\n> > \n> > class MainWindow : public QMainWindow\n> > @@ -79,7 +83,7 @@ private Q_SLOTS:\n> > \tvoid captureRaw();\n> > \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n> > \n> > -\tvoid queueRequest(FrameBuffer *buffer);\n> > +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n> > \n> > private:\n> > \tint createToolbars();\n> > @@ -96,7 +100,7 @@ private:\n> > \tvoid requestComplete(Request *request);\n> > \tvoid processCapture();\n> > \tvoid processHotplug(HotplugEvent *e);\n> > -\tvoid processViewfinder(FrameBuffer *buffer);\n> > +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n> > \n> > \t/* UI elements */\n> > \tQToolBar *toolbar_;\n> > @@ -135,6 +139,8 @@ private:\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/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > index 67da1df2..26c20f1b 100644\n> > --- a/src/qcam/viewfinder.h\n> > +++ b/src/qcam/viewfinder.h\n> > @@ -27,7 +27,9 @@ public:\n> > \tvirtual const QList &nativeFormats() const = 0;\n> > \n> > \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> > +\tvirtual void render(libcamera::Request *request,\n> > +\t\t\t libcamera::FrameBuffer *buffer,\n> > +\t\t\t MappedBuffer *map) = 0;\n> > \tvirtual void stop() = 0;\n> > \n> > \tvirtual QImage getCurrentImage() = 0;\n> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > index fbe21dcf..c65b41f6 100644\n> > --- a/src/qcam/viewfinder_gl.cpp\n> > +++ b/src/qcam/viewfinder_gl.cpp\n> > @@ -23,7 +23,7 @@ static const QList supportedFormats{\n> > };\n> > \n> > ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n> > +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n> > \t fragmentShader_(nullptr), vertexShader_(nullptr),\n> > \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n> > \t textureU_(QOpenGLTexture::Target2D),\n> > @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > void ViewFinderGL::stop()\n> > {\n> > \tif (buffer_) {\n> > -\t\trenderComplete(buffer_);\n> > +\t\trenderComplete(request_, buffer_);\n> > \t\tbuffer_ = nullptr;\n> > \t}\n> > }\n> > @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n> > \treturn grabFramebuffer();\n> > }\n> > \n> > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > {\n> > \tif (buffer->planes().size() != 1) {\n> > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \t}\n> > \n> > \tif (buffer_)\n> > -\t\trenderComplete(buffer_);\n> > +\t\trenderComplete(request_, buffer_);\n> > \n> > \tyuvData_ = static_cast(map->memory);\n> > \tupdate();\n> > \tbuffer_ = buffer;\n> > +\trequest_ = request;\n> > }\n> > \n> > bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > index 69502b7a..7db41841 100644\n> > --- a/src/qcam/viewfinder_gl.h\n> > +++ b/src/qcam/viewfinder_gl.h\n> > @@ -36,13 +36,13 @@ public:\n> > \tconst QList &nativeFormats() const override;\n> > \n> > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > \tvoid stop() override;\n> > \n> > \tQImage getCurrentImage() override;\n> > \n> > Q_SIGNALS:\n> > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > \n> > protected:\n> > \tvoid initializeGL() override;\n> > @@ -60,6 +60,7 @@ private:\n> > \tvoid doRender();\n> > \n> > \t/* Captured image size, format and buffer */\n> > +\tlibcamera::Request *request_;\n> > \tlibcamera::FrameBuffer *buffer_;\n> > \tlibcamera::PixelFormat format_;\n> > \tQSize size_;\n> > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > index e436714c..7f9f913b 100644\n> > --- a/src/qcam/viewfinder_qt.cpp\n> > +++ b/src/qcam/viewfinder_qt.cpp\n> > @@ -34,7 +34,7 @@ static const QMap nativeFormats\n> > };\n> > \n> > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > -\t: QWidget(parent), buffer_(nullptr)\n> > +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n> > {\n> > \ticon_ = QIcon(\":camera-off.svg\");\n> > }\n> > @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > \treturn 0;\n> > }\n> > \n> > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > +void ViewFinderQt::render(libcamera::Request *request,\n> > +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > {\n> > \tif (buffer->planes().size() != 1) {\n> > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \t\t\t\t\tsize / size_.height(),\n> > \t\t\t\t\t::nativeFormats[format_]);\n> > \t\t\tstd::swap(buffer, buffer_);\n> > +\t\t\tstd::swap(request, request_);\n> > \t\t} else {\n> > \t\t\t/*\n> > \t\t\t * Otherwise, convert the format and release the frame\n> > @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \tupdate();\n> > \n> > \tif (buffer)\n> > -\t\trenderComplete(buffer);\n> > +\t\trenderComplete(request, buffer);\n> > }\n> > \n> > void ViewFinderQt::stop()\n> > @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n> > \timage_ = QImage();\n> > \n> > \tif (buffer_) {\n> > -\t\trenderComplete(buffer_);\n> > +\t\trenderComplete(request_, buffer_);\n> > \t\tbuffer_ = nullptr;\n> > \t}\n> > \n> > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > index d7554288..5f62d7f6 100644\n> > --- a/src/qcam/viewfinder_qt.h\n> > +++ b/src/qcam/viewfinder_qt.h\n> > @@ -17,6 +17,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > \n> > #include \"format_converter.h\"\n> > #include \"viewfinder.h\"\n> > @@ -32,13 +33,15 @@ public:\n> > \tconst QList &nativeFormats() const override;\n> > \n> > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > +\tvoid render(libcamera::Request *request,\n> > +\t\t libcamera::FrameBuffer *buffer,\n> > +\t\t MappedBuffer *map) override;\n> > \tvoid stop() override;\n> > \n> > \tQImage getCurrentImage() override;\n> > \n> > Q_SIGNALS:\n> > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > \n> > protected:\n> > \tvoid paintEvent(QPaintEvent *) override;\n> > @@ -56,6 +59,7 @@ private:\n> > \tQPixmap pixmap_;\n> > \n> > \t/* Buffer and render image */\n> > +\tlibcamera::Request *request_;\n> > \tlibcamera::FrameBuffer *buffer_;\n> > \tQImage image_;\n> > \tQMutex mutex_; /* Prevent concurrent access to image_ */","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 7E552C3B5B\n\tfor ;\n\tThu, 24 Sep 2020 12:41:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0D4862FDA;\n\tThu, 24 Sep 2020 14:41:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5AD960362\n\tfor ;\n\tThu, 24 Sep 2020 14:41:33 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA4E62FD;\n\tThu, 24 Sep 2020 14:41:29 +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=\"NL0njjXg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600951290;\n\tbh=rG5+SIIURgkicC6DtrmV2dBsrJRMuD+v4C3VmKlhKlU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NL0njjXgjL143CYaZ6qb2NOPy0FjY2bsXSi038KwjAg+Dxp4nfp05PI4L9AG/Oqcc\n\ty+2tpy6T5YYT9zxlmum1f0j0xSobSY8OyUpolRye+iuycYCCxMow3OypukDR+sgcqU\n\tFM+Qpelkr9zpQd3pqILCPWwA99tL4PBi5M9dhM6Q=","Date":"Thu, 24 Sep 2020 15:40:56 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200924124056.GD3968@pendragon.ideasonboard.com>","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>\n\t<20200924120951.GD502216@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200924120951.GD502216@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12755,"web_url":"https://patchwork.libcamera.org/comment/12755/","msgid":"<20200925022038.GO45948@pyrite.rasen.tech>","date":"2020-09-25T02:20:38","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: Reuse Request","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hello,\n\nOn Thu, Sep 24, 2020 at 03:40:56PM +0300, Laurent Pinchart wrote:\n> Hello,\n> \n> On Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:\n> > On 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > \n> > > ---\n> > > In this RFC, I've only made qcam and cam reuse the Request object. Just\n> > > gauging the direction, and then I'll put it in for andoird and gstreamer\n> > > and v4l2-compat as well.\n> > > ---\n> > > include/libcamera/request.h | 2 ++\n> > > src/cam/capture.cpp | 20 ++++++++++----------\n> > > src/cam/capture.h | 3 +++\n> > > src/libcamera/camera.cpp | 4 +---\n> > > src/libcamera/request.cpp | 9 +++++++++\n> > > src/qcam/main_window.cpp | 26 ++++++++++++--------------\n> > > src/qcam/main_window.h | 14 ++++++++++----\n> > > src/qcam/viewfinder.h | 4 +++-\n> > > src/qcam/viewfinder_gl.cpp | 9 +++++----\n> > > src/qcam/viewfinder_gl.h | 5 +++--\n> > > src/qcam/viewfinder_qt.cpp | 10 ++++++----\n> > > src/qcam/viewfinder_qt.h | 8 ++++++--\n> > > 12 files changed, 70 insertions(+), 44 deletions(-)\n> > > \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/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 5510c009..fe9d591a 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n> > > \t\twriter_ = nullptr;\n> > > \t}\n> > > \n> > > +\tfor (Request *req : requests_)\n> > > +\t\tdelete req;\n> > > +\n> > > \tdelete allocator;\n> > > \n> > > \treturn ret;\n> > > @@ -92,7 +95,6 @@ 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\tif (!request) {\n> > > @@ -117,7 +119,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(request);\n> > > \t}\n> > > \n> > > \tret = camera_->start();\n> > > @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > \t\treturn ret;\n> > > \t}\n> > > \n> > > -\tfor (Request *request : requests) {\n> > > +\tfor (Request *request : requests_) {\n> > > \t\tret = camera_->queueRequest(request);\n> > > \t\tif (ret < 0) {\n> > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > \n> > > void Capture::requestComplete(Request *request)\n> > > {\n> > > -\tif (request->status() == Request::RequestCancelled)\n> > > +\tif (request->status() == Request::RequestCancelled) {\n> > > +\t\trequest->reset();\n> > > \t\treturn;\n> > > +\t}\n> > > \n> > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > +\tconst Request::BufferMap buffers = request->buffers();\n> \n> A copy isn't very nice, but we can live with it for now.\n\nWithout the copy, all the buffers become invalid upon reset :/\n\n> > > \n> > > \t/*\n> > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > @@ -206,11 +210,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> > Hum, is this really \"reusing\" the request object? I imagined a reused \n> > request would retain its buffers while gaining the ability to be be \n> > requeued to the Camera after it have completed. With this patch we still \n> > need to keep the logic to fill the request with buffers each iteration, \n> > is there some advantage to this I'm missing?\n> \n> Two advantages, we don't free and reallocate the request needlessly, and\n> it can be stored in the application and used after the request handler\n> completes.\n> \n> I don't think retaining buffers by default is a good idea, in many cases\n> we'll have a different number of buffers that can be cycled through than\n> the number of allocated requests.\n> \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..62cf9bf1 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/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index ae16a64a..4cc68bce 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -974,13 +974,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..606866a6 100644\n> > > --- a/src/libcamera/request.cpp\n> > > +++ b/src/libcamera/request.cpp\n> > > @@ -85,6 +85,15 @@ Request::~Request()\n> > > \tdelete validator_;\n> > > }\n> > > \n> \n> No documentation ? :-) You should also update the documentation of\n\nNot yet :p\n\n> Camera::queueRequest(). Thinking about it, I think we should enforce\n> ownership of requests by the application by returning a\n> std::unique_ptr from Camera::createRequest(). We may even want\n> to drop Camera::createRequest() completely, but that can be done in a\n> separate patch.\n\nYeah... but I thought there was a problem when the Request completes,\nsince there could be multiple slots, and they all can't receive unique\npointers.\n\nOn the other hand... it's only one application... could we require that\nthe application only connect one slot to the Request completion handler,\nand then the application can split it among multiple sub-handlers if it\nwants to? Then we can keep unique_ptr for the Request, and the ownership\ncan be clear.\n\n> > > +void Request::reset()\n> > > +{\n> > > +\tbufferMap_.clear();\n> > > +\tpending_.clear();\n> \n> You need to clear controls_ and metadata_ too.\n\nOh okay, those can't be reused?\n\n> > > +\n> > > +\tstatus_ = RequestPending;\n> > > +\tcancelled_ = false;\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..1531b44f 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> > > @@ -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(request);\n> > > \t}\n> > > \n> > > \t/* Start the title timer and the camera. */\n> > > @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n> > > \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > > \n> > > \t/* Queue all requests. */\n> > > -\tfor (Request *request : requests) {\n> > > +\tfor (Request *request : requests_) {\n> > > \t\tret = camera_->queueRequest(request);\n> > > \t\tif (ret < 0) {\n> > > \t\t\tqWarning() << \"Can't queue request\";\n> > > @@ -535,7 +534,7 @@ error_disconnect:\n> > > \tcamera_->stop();\n> > > \n> > > error:\n> > > -\tfor (Request *request : requests)\n> > > +\tfor (Request *request : requests_)\n> > > \t\tdelete request;\n> > > \n> > > \tfor (auto &iter : mappedBuffers_) {\n> > > @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n> > > \t}\n> > > \tmappedBuffers_.clear();\n> > > \n> > > +\tfor (Request *req : requests_)\n> > > +\t\tdelete req;\n> \n> Have you tried stopping and restarting capture ? You delete all the\n> requests here, but the vector still contains the pointers. When capture\n> is restarted I expect issue. A\n> \n> \trequests_.clear();\n\nAh, right.\n\n> is needed, and I would also turn it into a\n> std::vector> to avoid the manual delete loops.\n> Same thing for the cam application (even if in that case capture can't\n> be stopped and restarted at the moment, but let's still do it right).\n> \n> > > +\n> > > \tdelete allocator_;\n> > > \n> > > \tisCapturing_ = false;\n> > > @@ -701,7 +703,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->buffers(), request->metadata(), request });\n> \n> You aobut enqueuing the request only ? The reason to enqueue a copy of\n> buffers() and metadata() was because the request was deleted right after\n> this function returns, now that it's not the case anymore, we can just\n> pass the request around. The MainWindow::CaptureRequest class can be\n> removed.\n\nack\n\n> > > \t}\n> > > \n> > > \tQCoreApplication::postEvent(this, new CaptureEvent);\n> > > @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n> > > \n> > > \t/* Process buffers. */\n> > > \tif (request.buffers_.count(vfStream_))\n> > > -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> > > +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n> > > \n> > > \tif (request.buffers_.count(rawStream_))\n> > > \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> \n> Instead of passing the request to processViewfinder(), how about adding\n> it to a free list here, and taking a request from the free list in\n> MainWindow::queueRequest() ? That would avoid the changes to the\n> viewfinder.\n\nack\n\n> > > }\n> > > \n> > > -void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n> > > {\n> > > \tframesCaptured_++;\n> > > \n> > > @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > > \n> > > \t/* Render the frame on the viewfinder. */\n> > > -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n> > > +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n> > > }\n> > > \n> > > -void MainWindow::queueRequest(FrameBuffer *buffer)\n> > > +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n> > > {\n> > > -\tRequest *request = camera_->createRequest();\n> > > -\tif (!request) {\n> > > -\t\tqWarning() << \"Can't create request\";\n> > > -\t\treturn;\n> > > -\t}\n> > > +\trequest->reset();\n> > > \n> > > \trequest->addBuffer(vfStream_, buffer);\n> > > \n> > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > index 5c61a4df..83535373 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> > > @@ -49,13 +51,15 @@ public:\n> > > \t}\n> > > \n> > > \tCaptureRequest(const Request::BufferMap &buffers,\n> > > -\t\t const ControlList &metadata)\n> > > -\t\t: buffers_(buffers), metadata_(metadata)\n> > > +\t\t const ControlList &metadata,\n> > > +\t\t Request * const request)\n> > > +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n> > > \t{\n> > > \t}\n> > > \n> > > \tRequest::BufferMap buffers_;\n> > > \tControlList metadata_;\n> > > +\tRequest *request_;\n> > > };\n> > > \n> > > class MainWindow : public QMainWindow\n> > > @@ -79,7 +83,7 @@ private Q_SLOTS:\n> > > \tvoid captureRaw();\n> > > \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n> > > \n> > > -\tvoid queueRequest(FrameBuffer *buffer);\n> > > +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n> > > \n> > > private:\n> > > \tint createToolbars();\n> > > @@ -96,7 +100,7 @@ private:\n> > > \tvoid requestComplete(Request *request);\n> > > \tvoid processCapture();\n> > > \tvoid processHotplug(HotplugEvent *e);\n> > > -\tvoid processViewfinder(FrameBuffer *buffer);\n> > > +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n> > > \n> > > \t/* UI elements */\n> > > \tQToolBar *toolbar_;\n> > > @@ -135,6 +139,8 @@ private:\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/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > > index 67da1df2..26c20f1b 100644\n> > > --- a/src/qcam/viewfinder.h\n> > > +++ b/src/qcam/viewfinder.h\n> > > @@ -27,7 +27,9 @@ public:\n> > > \tvirtual const QList &nativeFormats() const = 0;\n> > > \n> > > \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > > -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> > > +\tvirtual void render(libcamera::Request *request,\n> > > +\t\t\t libcamera::FrameBuffer *buffer,\n> > > +\t\t\t MappedBuffer *map) = 0;\n> > > \tvirtual void stop() = 0;\n> > > \n> > > \tvirtual QImage getCurrentImage() = 0;\n> > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > index fbe21dcf..c65b41f6 100644\n> > > --- a/src/qcam/viewfinder_gl.cpp\n> > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > @@ -23,7 +23,7 @@ static const QList supportedFormats{\n> > > };\n> > > \n> > > ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n> > > +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n> > > \t fragmentShader_(nullptr), vertexShader_(nullptr),\n> > > \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n> > > \t textureU_(QOpenGLTexture::Target2D),\n> > > @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > > void ViewFinderGL::stop()\n> > > {\n> > > \tif (buffer_) {\n> > > -\t\trenderComplete(buffer_);\n> > > +\t\trenderComplete(request_, buffer_);\n> > > \t\tbuffer_ = nullptr;\n> > > \t}\n> > > }\n> > > @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n> > > \treturn grabFramebuffer();\n> > > }\n> > > \n> > > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > {\n> > > \tif (buffer->planes().size() != 1) {\n> > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > \t}\n> > > \n> > > \tif (buffer_)\n> > > -\t\trenderComplete(buffer_);\n> > > +\t\trenderComplete(request_, buffer_);\n> > > \n> > > \tyuvData_ = static_cast(map->memory);\n> > > \tupdate();\n> > > \tbuffer_ = buffer;\n> > > +\trequest_ = request;\n> > > }\n> > > \n> > > bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > > index 69502b7a..7db41841 100644\n> > > --- a/src/qcam/viewfinder_gl.h\n> > > +++ b/src/qcam/viewfinder_gl.h\n> > > @@ -36,13 +36,13 @@ public:\n> > > \tconst QList &nativeFormats() const override;\n> > > \n> > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > \tvoid stop() override;\n> > > \n> > > \tQImage getCurrentImage() override;\n> > > \n> > > Q_SIGNALS:\n> > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > \n> > > protected:\n> > > \tvoid initializeGL() override;\n> > > @@ -60,6 +60,7 @@ private:\n> > > \tvoid doRender();\n> > > \n> > > \t/* Captured image size, format and buffer */\n> > > +\tlibcamera::Request *request_;\n> > > \tlibcamera::FrameBuffer *buffer_;\n> > > \tlibcamera::PixelFormat format_;\n> > > \tQSize size_;\n> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > index e436714c..7f9f913b 100644\n> > > --- a/src/qcam/viewfinder_qt.cpp\n> > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > @@ -34,7 +34,7 @@ static const QMap nativeFormats\n> > > };\n> > > \n> > > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > > -\t: QWidget(parent), buffer_(nullptr)\n> > > +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n> > > {\n> > > \ticon_ = QIcon(\":camera-off.svg\");\n> > > }\n> > > @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > > \treturn 0;\n> > > }\n> > > \n> > > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > +void ViewFinderQt::render(libcamera::Request *request,\n> > > +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > {\n> > > \tif (buffer->planes().size() != 1) {\n> > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > \t\t\t\t\tsize / size_.height(),\n> > > \t\t\t\t\t::nativeFormats[format_]);\n> > > \t\t\tstd::swap(buffer, buffer_);\n> > > +\t\t\tstd::swap(request, request_);\n> > > \t\t} else {\n> > > \t\t\t/*\n> > > \t\t\t * Otherwise, convert the format and release the frame\n> > > @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > \tupdate();\n> > > \n> > > \tif (buffer)\n> > > -\t\trenderComplete(buffer);\n> > > +\t\trenderComplete(request, buffer);\n> > > }\n> > > \n> > > void ViewFinderQt::stop()\n> > > @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n> > > \timage_ = QImage();\n> > > \n> > > \tif (buffer_) {\n> > > -\t\trenderComplete(buffer_);\n> > > +\t\trenderComplete(request_, buffer_);\n> > > \t\tbuffer_ = nullptr;\n> > > \t}\n> > > \n> > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > > index d7554288..5f62d7f6 100644\n> > > --- a/src/qcam/viewfinder_qt.h\n> > > +++ b/src/qcam/viewfinder_qt.h\n> > > @@ -17,6 +17,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > \n> > > #include \"format_converter.h\"\n> > > #include \"viewfinder.h\"\n> > > @@ -32,13 +33,15 @@ public:\n> > > \tconst QList &nativeFormats() const override;\n> > > \n> > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > +\tvoid render(libcamera::Request *request,\n> > > +\t\t libcamera::FrameBuffer *buffer,\n> > > +\t\t MappedBuffer *map) override;\n> > > \tvoid stop() override;\n> > > \n> > > \tQImage getCurrentImage() override;\n> > > \n> > > Q_SIGNALS:\n> > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > \n> > > protected:\n> > > \tvoid paintEvent(QPaintEvent *) override;\n> > > @@ -56,6 +59,7 @@ private:\n> > > \tQPixmap pixmap_;\n> > > \n> > > \t/* Buffer and render image */\n> > > +\tlibcamera::Request *request_;\n> > > \tlibcamera::FrameBuffer *buffer_;\n> > > \tQImage image_;\n> > > \tQMutex mutex_; /* Prevent concurrent access to image_ */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 21F11C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 02:20:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A30963005;\n\tFri, 25 Sep 2020 04:20:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39F0160576\n\tfor ;\n\tFri, 25 Sep 2020 04:20:50 +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 0EE3E2D7;\n\tFri, 25 Sep 2020 04:20:45 +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=\"d3pZZgPO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601000447;\n\tbh=FO7iMpjA5ax0XTqmplOfUigPrL9hWGdZt1RARpqBFkc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d3pZZgPOUJ+Sf/A/UdgTs2r08nXHuRsOGZ/Fvm5UACdQHdtZb9cWfID3aDp5LWqFw\n\tMKuvXTeKeVUhtyB8QA4C077CSaWX5Ul6MbtJmwxbmq8mQcDYr9pwH2yiz8PRqFYG8f\n\tDf1J2E8iPdTifxzOq2D+e1L1wokuu47sIJcAoA5g=","Date":"Fri, 25 Sep 2020 11:20:38 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20200925022038.GO45948@pyrite.rasen.tech>","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>\n\t<20200924120951.GD502216@oden.dyn.berto.se>\n\t<20200924124056.GD3968@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200924124056.GD3968@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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":12756,"web_url":"https://patchwork.libcamera.org/comment/12756/","msgid":"<20200925023952.GA8144@pendragon.ideasonboard.com>","date":"2020-09-25T02:39:52","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: Reuse Request","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Fri, Sep 25, 2020 at 11:20:38AM +0900, paul.elder@ideasonboard.com wrote:\n> On Thu, Sep 24, 2020 at 03:40:56PM +0300, Laurent Pinchart wrote:\n> > On Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:\n> > > On 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n> > > > \n> > > > Signed-off-by: Paul Elder \n> > > > \n> > > > ---\n> > > > In this RFC, I've only made qcam and cam reuse the Request object. Just\n> > > > gauging the direction, and then I'll put it in for andoird and gstreamer\n> > > > and v4l2-compat as well.\n> > > > ---\n> > > > include/libcamera/request.h | 2 ++\n> > > > src/cam/capture.cpp | 20 ++++++++++----------\n> > > > src/cam/capture.h | 3 +++\n> > > > src/libcamera/camera.cpp | 4 +---\n> > > > src/libcamera/request.cpp | 9 +++++++++\n> > > > src/qcam/main_window.cpp | 26 ++++++++++++--------------\n> > > > src/qcam/main_window.h | 14 ++++++++++----\n> > > > src/qcam/viewfinder.h | 4 +++-\n> > > > src/qcam/viewfinder_gl.cpp | 9 +++++----\n> > > > src/qcam/viewfinder_gl.h | 5 +++--\n> > > > src/qcam/viewfinder_qt.cpp | 10 ++++++----\n> > > > src/qcam/viewfinder_qt.h | 8 ++++++--\n> > > > 12 files changed, 70 insertions(+), 44 deletions(-)\n> > > > \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/cam/capture.cpp b/src/cam/capture.cpp\n> > > > index 5510c009..fe9d591a 100644\n> > > > --- a/src/cam/capture.cpp\n> > > > +++ b/src/cam/capture.cpp\n> > > > @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n> > > > \t\twriter_ = nullptr;\n> > > > \t}\n> > > > \n> > > > +\tfor (Request *req : requests_)\n> > > > +\t\tdelete req;\n> > > > +\n> > > > \tdelete allocator;\n> > > > \n> > > > \treturn ret;\n> > > > @@ -92,7 +95,6 @@ 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\tif (!request) {\n> > > > @@ -117,7 +119,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(request);\n> > > > \t}\n> > > > \n> > > > \tret = camera_->start();\n> > > > @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > \t\treturn ret;\n> > > > \t}\n> > > > \n> > > > -\tfor (Request *request : requests) {\n> > > > +\tfor (Request *request : requests_) {\n> > > > \t\tret = camera_->queueRequest(request);\n> > > > \t\tif (ret < 0) {\n> > > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > > @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > \n> > > > void Capture::requestComplete(Request *request)\n> > > > {\n> > > > -\tif (request->status() == Request::RequestCancelled)\n> > > > +\tif (request->status() == Request::RequestCancelled) {\n> > > > +\t\trequest->reset();\n> > > > \t\treturn;\n> > > > +\t}\n> > > > \n> > > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > > +\tconst Request::BufferMap buffers = request->buffers();\n> > \n> > A copy isn't very nice, but we can live with it for now.\n> \n> Without the copy, all the buffers become invalid upon reset :/\n\nI know, that's why I said we can live with it for now :-)\n\n> > > > \n> > > > \t/*\n> > > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > > @@ -206,11 +210,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> > > Hum, is this really \"reusing\" the request object? I imagined a reused \n> > > request would retain its buffers while gaining the ability to be be \n> > > requeued to the Camera after it have completed. With this patch we still \n> > > need to keep the logic to fill the request with buffers each iteration, \n> > > is there some advantage to this I'm missing?\n> > \n> > Two advantages, we don't free and reallocate the request needlessly, and\n> > it can be stored in the application and used after the request handler\n> > completes.\n> > \n> > I don't think retaining buffers by default is a good idea, in many cases\n> > we'll have a different number of buffers that can be cycled through than\n> > the number of allocated requests.\n> > \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..62cf9bf1 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/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > index ae16a64a..4cc68bce 100644\n> > > > --- a/src/libcamera/camera.cpp\n> > > > +++ b/src/libcamera/camera.cpp\n> > > > @@ -974,13 +974,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..606866a6 100644\n> > > > --- a/src/libcamera/request.cpp\n> > > > +++ b/src/libcamera/request.cpp\n> > > > @@ -85,6 +85,15 @@ Request::~Request()\n> > > > \tdelete validator_;\n> > > > }\n> > > > \n> > \n> > No documentation ? :-) You should also update the documentation of\n> \n> Not yet :p\n> \n> > Camera::queueRequest(). Thinking about it, I think we should enforce\n> > ownership of requests by the application by returning a\n> > std::unique_ptr from Camera::createRequest(). We may even want\n> > to drop Camera::createRequest() completely, but that can be done in a\n> > separate patch.\n> \n> Yeah... but I thought there was a problem when the Request completes,\n> since there could be multiple slots, and they all can't receive unique\n> pointers.\n> \n> On the other hand... it's only one application... could we require that\n> the application only connect one slot to the Request completion handler,\n> and then the application can split it among multiple sub-handlers if it\n> wants to? Then we can keep unique_ptr for the Request, and the ownership\n> can be clear.\n\nI don't think there's a way to implement this, the number of connected\nslots isn't known at compile time, the compiler won't like a\nstd::unique_ptr<> in a signal in any case.\n\nBut that's not what I was proposing. With request reuse, ownership of\nthe request stays in the application. Passing the request pointer to\nCamera::queueRequest() borrows a reference, the function shouldn't take\na unique pointer. The completion handler will thus keep using a normal\npointer too. It's only Camera::createRequest() that would return a\nunique pointer, to enforce ownership of the request by the application.\n\n> > > > +void Request::reset()\n> > > > +{\n> > > > +\tbufferMap_.clear();\n> > > > +\tpending_.clear();\n> > \n> > You need to clear controls_ and metadata_ too.\n> \n> Oh okay, those can't be reused?\n\nWhy should they ?\n\n> > > > +\n> > > > +\tstatus_ = RequestPending;\n> > > > +\tcancelled_ = false;\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..1531b44f 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> > > > @@ -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(request);\n> > > > \t}\n> > > > \n> > > > \t/* Start the title timer and the camera. */\n> > > > @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n> > > > \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > > > \n> > > > \t/* Queue all requests. */\n> > > > -\tfor (Request *request : requests) {\n> > > > +\tfor (Request *request : requests_) {\n> > > > \t\tret = camera_->queueRequest(request);\n> > > > \t\tif (ret < 0) {\n> > > > \t\t\tqWarning() << \"Can't queue request\";\n> > > > @@ -535,7 +534,7 @@ error_disconnect:\n> > > > \tcamera_->stop();\n> > > > \n> > > > error:\n> > > > -\tfor (Request *request : requests)\n> > > > +\tfor (Request *request : requests_)\n> > > > \t\tdelete request;\n> > > > \n> > > > \tfor (auto &iter : mappedBuffers_) {\n> > > > @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n> > > > \t}\n> > > > \tmappedBuffers_.clear();\n> > > > \n> > > > +\tfor (Request *req : requests_)\n> > > > +\t\tdelete req;\n> > \n> > Have you tried stopping and restarting capture ? You delete all the\n> > requests here, but the vector still contains the pointers. When capture\n> > is restarted I expect issue. A\n> > \n> > \trequests_.clear();\n> \n> Ah, right.\n> \n> > is needed, and I would also turn it into a\n> > std::vector> to avoid the manual delete loops.\n> > Same thing for the cam application (even if in that case capture can't\n> > be stopped and restarted at the moment, but let's still do it right).\n> > \n> > > > +\n> > > > \tdelete allocator_;\n> > > > \n> > > > \tisCapturing_ = false;\n> > > > @@ -701,7 +703,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->buffers(), request->metadata(), request });\n> > \n> > You aobut enqueuing the request only ? The reason to enqueue a copy of\n> > buffers() and metadata() was because the request was deleted right after\n> > this function returns, now that it's not the case anymore, we can just\n> > pass the request around. The MainWindow::CaptureRequest class can be\n> > removed.\n> \n> ack\n> \n> > > > \t}\n> > > > \n> > > > \tQCoreApplication::postEvent(this, new CaptureEvent);\n> > > > @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n> > > > \n> > > > \t/* Process buffers. */\n> > > > \tif (request.buffers_.count(vfStream_))\n> > > > -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> > > > +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n> > > > \n> > > > \tif (request.buffers_.count(rawStream_))\n> > > > \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> > \n> > Instead of passing the request to processViewfinder(), how about adding\n> > it to a free list here, and taking a request from the free list in\n> > MainWindow::queueRequest() ? That would avoid the changes to the\n> > viewfinder.\n> \n> ack\n> \n> > > > }\n> > > > \n> > > > -void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > > +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n> > > > {\n> > > > \tframesCaptured_++;\n> > > > \n> > > > @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > > \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > > > \n> > > > \t/* Render the frame on the viewfinder. */\n> > > > -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n> > > > +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n> > > > }\n> > > > \n> > > > -void MainWindow::queueRequest(FrameBuffer *buffer)\n> > > > +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n> > > > {\n> > > > -\tRequest *request = camera_->createRequest();\n> > > > -\tif (!request) {\n> > > > -\t\tqWarning() << \"Can't create request\";\n> > > > -\t\treturn;\n> > > > -\t}\n> > > > +\trequest->reset();\n> > > > \n> > > > \trequest->addBuffer(vfStream_, buffer);\n> > > > \n> > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > > index 5c61a4df..83535373 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> > > > @@ -49,13 +51,15 @@ public:\n> > > > \t}\n> > > > \n> > > > \tCaptureRequest(const Request::BufferMap &buffers,\n> > > > -\t\t const ControlList &metadata)\n> > > > -\t\t: buffers_(buffers), metadata_(metadata)\n> > > > +\t\t const ControlList &metadata,\n> > > > +\t\t Request * const request)\n> > > > +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n> > > > \t{\n> > > > \t}\n> > > > \n> > > > \tRequest::BufferMap buffers_;\n> > > > \tControlList metadata_;\n> > > > +\tRequest *request_;\n> > > > };\n> > > > \n> > > > class MainWindow : public QMainWindow\n> > > > @@ -79,7 +83,7 @@ private Q_SLOTS:\n> > > > \tvoid captureRaw();\n> > > > \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n> > > > \n> > > > -\tvoid queueRequest(FrameBuffer *buffer);\n> > > > +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n> > > > \n> > > > private:\n> > > > \tint createToolbars();\n> > > > @@ -96,7 +100,7 @@ private:\n> > > > \tvoid requestComplete(Request *request);\n> > > > \tvoid processCapture();\n> > > > \tvoid processHotplug(HotplugEvent *e);\n> > > > -\tvoid processViewfinder(FrameBuffer *buffer);\n> > > > +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n> > > > \n> > > > \t/* UI elements */\n> > > > \tQToolBar *toolbar_;\n> > > > @@ -135,6 +139,8 @@ private:\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/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > > > index 67da1df2..26c20f1b 100644\n> > > > --- a/src/qcam/viewfinder.h\n> > > > +++ b/src/qcam/viewfinder.h\n> > > > @@ -27,7 +27,9 @@ public:\n> > > > \tvirtual const QList &nativeFormats() const = 0;\n> > > > \n> > > > \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > > > -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> > > > +\tvirtual void render(libcamera::Request *request,\n> > > > +\t\t\t libcamera::FrameBuffer *buffer,\n> > > > +\t\t\t MappedBuffer *map) = 0;\n> > > > \tvirtual void stop() = 0;\n> > > > \n> > > > \tvirtual QImage getCurrentImage() = 0;\n> > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > > index fbe21dcf..c65b41f6 100644\n> > > > --- a/src/qcam/viewfinder_gl.cpp\n> > > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > > @@ -23,7 +23,7 @@ static const QList supportedFormats{\n> > > > };\n> > > > \n> > > > ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > > -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n> > > > +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n> > > > \t fragmentShader_(nullptr), vertexShader_(nullptr),\n> > > > \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n> > > > \t textureU_(QOpenGLTexture::Target2D),\n> > > > @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > > > void ViewFinderGL::stop()\n> > > > {\n> > > > \tif (buffer_) {\n> > > > -\t\trenderComplete(buffer_);\n> > > > +\t\trenderComplete(request_, buffer_);\n> > > > \t\tbuffer_ = nullptr;\n> > > > \t}\n> > > > }\n> > > > @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n> > > > \treturn grabFramebuffer();\n> > > > }\n> > > > \n> > > > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > {\n> > > > \tif (buffer->planes().size() != 1) {\n> > > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > > @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > \t}\n> > > > \n> > > > \tif (buffer_)\n> > > > -\t\trenderComplete(buffer_);\n> > > > +\t\trenderComplete(request_, buffer_);\n> > > > \n> > > > \tyuvData_ = static_cast(map->memory);\n> > > > \tupdate();\n> > > > \tbuffer_ = buffer;\n> > > > +\trequest_ = request;\n> > > > }\n> > > > \n> > > > bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> > > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > > > index 69502b7a..7db41841 100644\n> > > > --- a/src/qcam/viewfinder_gl.h\n> > > > +++ b/src/qcam/viewfinder_gl.h\n> > > > @@ -36,13 +36,13 @@ public:\n> > > > \tconst QList &nativeFormats() const override;\n> > > > \n> > > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > \tvoid stop() override;\n> > > > \n> > > > \tQImage getCurrentImage() override;\n> > > > \n> > > > Q_SIGNALS:\n> > > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > > \n> > > > protected:\n> > > > \tvoid initializeGL() override;\n> > > > @@ -60,6 +60,7 @@ private:\n> > > > \tvoid doRender();\n> > > > \n> > > > \t/* Captured image size, format and buffer */\n> > > > +\tlibcamera::Request *request_;\n> > > > \tlibcamera::FrameBuffer *buffer_;\n> > > > \tlibcamera::PixelFormat format_;\n> > > > \tQSize size_;\n> > > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > > index e436714c..7f9f913b 100644\n> > > > --- a/src/qcam/viewfinder_qt.cpp\n> > > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > > @@ -34,7 +34,7 @@ static const QMap nativeFormats\n> > > > };\n> > > > \n> > > > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > > > -\t: QWidget(parent), buffer_(nullptr)\n> > > > +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n> > > > {\n> > > > \ticon_ = QIcon(\":camera-off.svg\");\n> > > > }\n> > > > @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > > > \treturn 0;\n> > > > }\n> > > > \n> > > > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > +void ViewFinderQt::render(libcamera::Request *request,\n> > > > +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > {\n> > > > \tif (buffer->planes().size() != 1) {\n> > > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > > @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > \t\t\t\t\tsize / size_.height(),\n> > > > \t\t\t\t\t::nativeFormats[format_]);\n> > > > \t\t\tstd::swap(buffer, buffer_);\n> > > > +\t\t\tstd::swap(request, request_);\n> > > > \t\t} else {\n> > > > \t\t\t/*\n> > > > \t\t\t * Otherwise, convert the format and release the frame\n> > > > @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > \tupdate();\n> > > > \n> > > > \tif (buffer)\n> > > > -\t\trenderComplete(buffer);\n> > > > +\t\trenderComplete(request, buffer);\n> > > > }\n> > > > \n> > > > void ViewFinderQt::stop()\n> > > > @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n> > > > \timage_ = QImage();\n> > > > \n> > > > \tif (buffer_) {\n> > > > -\t\trenderComplete(buffer_);\n> > > > +\t\trenderComplete(request_, buffer_);\n> > > > \t\tbuffer_ = nullptr;\n> > > > \t}\n> > > > \n> > > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > > > index d7554288..5f62d7f6 100644\n> > > > --- a/src/qcam/viewfinder_qt.h\n> > > > +++ b/src/qcam/viewfinder_qt.h\n> > > > @@ -17,6 +17,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > \n> > > > #include \"format_converter.h\"\n> > > > #include \"viewfinder.h\"\n> > > > @@ -32,13 +33,15 @@ public:\n> > > > \tconst QList &nativeFormats() const override;\n> > > > \n> > > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > +\tvoid render(libcamera::Request *request,\n> > > > +\t\t libcamera::FrameBuffer *buffer,\n> > > > +\t\t MappedBuffer *map) override;\n> > > > \tvoid stop() override;\n> > > > \n> > > > \tQImage getCurrentImage() override;\n> > > > \n> > > > Q_SIGNALS:\n> > > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > > \n> > > > protected:\n> > > > \tvoid paintEvent(QPaintEvent *) override;\n> > > > @@ -56,6 +59,7 @@ private:\n> > > > \tQPixmap pixmap_;\n> > > > \n> > > > \t/* Buffer and render image */\n> > > > +\tlibcamera::Request *request_;\n> > > > \tlibcamera::FrameBuffer *buffer_;\n> > > > \tQImage image_;\n> > > > \tQMutex mutex_; /* Prevent concurrent access to image_ */","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 19F27C3B5C\n\tfor ;\n\tFri, 25 Sep 2020 02:40:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86F9B62FF8;\n\tFri, 25 Sep 2020 04:40:29 +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 5525960576\n\tfor ;\n\tFri, 25 Sep 2020 04:40:27 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42AF72D7;\n\tFri, 25 Sep 2020 04:40:26 +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=\"n7Sdx/wG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601001626;\n\tbh=4CXvSFJYbU/54eA/1o61f181QUNfQBvbj8McMLUhfcc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=n7Sdx/wGQJjbMDNepj5xThu2lSZjZlVFPdFHnozfMgLAFIKN3dVN7MkJODjPal+9W\n\tWQqb1y9Tm6TUpYSSWvKUGSF8kV826s1c8WAp3Bpz6D3xZbFJoql1cWh/Slye0j9gf7\n\tuFj1Kb815sz0r3FlpefUFteGQfatxSHks3/vadBU=","Date":"Fri, 25 Sep 2020 05:39:52 +0300","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"<20200925023952.GA8144@pendragon.ideasonboard.com>","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>\n\t<20200924120951.GD502216@oden.dyn.berto.se>\n\t<20200924124056.GD3968@pendragon.ideasonboard.com>\n\t<20200925022038.GO45948@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200925022038.GO45948@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12759,"web_url":"https://patchwork.libcamera.org/comment/12759/","msgid":"<11cfdb8a-1eeb-fd57-b42f-92d2f99f6e68@uajain.com>","date":"2020-09-25T07:04:34","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: Reuse Request","submitter":{"id":1,"url":"https://patchwork.libcamera.org/api/people/1/","name":"Umang Jain","email":"email@uajain.com"},"content":"Hi,\n\nI have a question/confusion.\n\nOn 9/25/20 7:50 AM, paul.elder@ideasonboard.com wrote:\n> Hello,\n>\n> On Thu, Sep 24, 2020 at 03:40:56PM +0300, Laurent Pinchart wrote:\n>> Hello,\n>>\n>> On Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:\n>>> On 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n>>>>\n>>>> Signed-off-by: Paul Elder \n>>>>\n>>>> ---\n>>>> In this RFC, I've only made qcam and cam reuse the Request object. Just\n>>>> gauging the direction, and then I'll put it in for andoird and gstreamer\n>>>> and v4l2-compat as well.\n>>>> ---\n>>>> include/libcamera/request.h | 2 ++\n>>>> src/cam/capture.cpp | 20 ++++++++++----------\n>>>> src/cam/capture.h | 3 +++\n>>>> src/libcamera/camera.cpp | 4 +---\n>>>> src/libcamera/request.cpp | 9 +++++++++\n>>>> src/qcam/main_window.cpp | 26 ++++++++++++--------------\n>>>> src/qcam/main_window.h | 14 ++++++++++----\n>>>> src/qcam/viewfinder.h | 4 +++-\n>>>> src/qcam/viewfinder_gl.cpp | 9 +++++----\n>>>> src/qcam/viewfinder_gl.h | 5 +++--\n>>>> src/qcam/viewfinder_qt.cpp | 10 ++++++----\n>>>> src/qcam/viewfinder_qt.h | 8 ++++++--\n>>>> 12 files changed, 70 insertions(+), 44 deletions(-)\n>>>>\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/cam/capture.cpp b/src/cam/capture.cpp\n>>>> index 5510c009..fe9d591a 100644\n>>>> --- a/src/cam/capture.cpp\n>>>> +++ b/src/cam/capture.cpp\n>>>> @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n>>>> \t\twriter_ = nullptr;\n>>>> \t}\n>>>> \n>>>> +\tfor (Request *req : requests_)\n>>>> +\t\tdelete req;\n>>>> +\n>>>> \tdelete allocator;\n>>>> \n>>>> \treturn ret;\n>>>> @@ -92,7 +95,6 @@ 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\tif (!request) {\n>>>> @@ -117,7 +119,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(request);\n>>>> \t}\n>>>> \n>>>> \tret = camera_->start();\n>>>> @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n>>>> \t\treturn ret;\n>>>> \t}\n>>>> \n>>>> -\tfor (Request *request : requests) {\n>>>> +\tfor (Request *request : requests_) {\n>>>> \t\tret = camera_->queueRequest(request);\n>>>> \t\tif (ret < 0) {\n>>>> \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n>>>> @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n>>>> \n>>>> void Capture::requestComplete(Request *request)\n>>>> {\n>>>> -\tif (request->status() == Request::RequestCancelled)\n>>>> +\tif (request->status() == Request::RequestCancelled) {\n>>>> +\t\trequest->reset();\n>>>> \t\treturn;\n>>>> +\t}\n>>>> \n>>>> -\tconst Request::BufferMap &buffers = request->buffers();\n>>>> +\tconst Request::BufferMap buffers = request->buffers();\n>> A copy isn't very nice, but we can live with it for now.\n> Without the copy, all the buffers become invalid upon reset :/\nIf I copy the BufferMap as is it here, would be also copy all the memory \nallocated to BufferMap's FrameBuffer(s)?\nThe reason I ask, because reset() seems to bufferMap_.clear() which \nmeans, it will destroy all members it is holding. Then once \nFrameBuffer(s) are destroyed via reset(), I would expect some \nre-allocation of memory happening somewhere for those FrameBuffer(s). Is \nthere where this copy comes into picture?\n>\n>>>> \n>>>> \t/*\n>>>> \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n>>>> @@ -206,11 +210,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>>> Hum, is this really \"reusing\" the request object? I imagined a reused\n>>> request would retain its buffers while gaining the ability to be be\n>>> requeued to the Camera after it have completed. With this patch we still\n>>> need to keep the logic to fill the request with buffers each iteration,\n>>> is there some advantage to this I'm missing?\n>> Two advantages, we don't free and reallocate the request needlessly, and\n>> it can be stored in the application and used after the request handler\n>> completes.\n>>\n>> I don't think retaining buffers by default is a good idea, in many cases\n>> we'll have a different number of buffers that can be cycled through than\n>> the number of allocated requests.\n>>\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..62cf9bf1 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/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>>> index ae16a64a..4cc68bce 100644\n>>>> --- a/src/libcamera/camera.cpp\n>>>> +++ b/src/libcamera/camera.cpp\n>>>> @@ -974,13 +974,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..606866a6 100644\n>>>> --- a/src/libcamera/request.cpp\n>>>> +++ b/src/libcamera/request.cpp\n>>>> @@ -85,6 +85,15 @@ Request::~Request()\n>>>> \tdelete validator_;\n>>>> }\n>>>> \n>> No documentation ? :-) You should also update the documentation of\n> Not yet :p\n>\n>> Camera::queueRequest(). Thinking about it, I think we should enforce\n>> ownership of requests by the application by returning a\n>> std::unique_ptr from Camera::createRequest(). We may even want\n>> to drop Camera::createRequest() completely, but that can be done in a\n>> separate patch.\n> Yeah... but I thought there was a problem when the Request completes,\n> since there could be multiple slots, and they all can't receive unique\n> pointers.\n>\n> On the other hand... it's only one application... could we require that\n> the application only connect one slot to the Request completion handler,\n> and then the application can split it among multiple sub-handlers if it\n> wants to? Then we can keep unique_ptr for the Request, and the ownership\n> can be clear.\n>\n>>>> +void Request::reset()\n>>>> +{\n>>>> +\tbufferMap_.clear();\n>>>> +\tpending_.clear();\n>> You need to clear controls_ and metadata_ too.\n> Oh okay, those can't be reused?\n>\n>>>> +\n>>>> +\tstatus_ = RequestPending;\n>>>> +\tcancelled_ = false;\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..1531b44f 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>>>> @@ -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(request);\n>>>> \t}\n>>>> \n>>>> \t/* Start the title timer and the camera. */\n>>>> @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n>>>> \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n>>>> \n>>>> \t/* Queue all requests. */\n>>>> -\tfor (Request *request : requests) {\n>>>> +\tfor (Request *request : requests_) {\n>>>> \t\tret = camera_->queueRequest(request);\n>>>> \t\tif (ret < 0) {\n>>>> \t\t\tqWarning() << \"Can't queue request\";\n>>>> @@ -535,7 +534,7 @@ error_disconnect:\n>>>> \tcamera_->stop();\n>>>> \n>>>> error:\n>>>> -\tfor (Request *request : requests)\n>>>> +\tfor (Request *request : requests_)\n>>>> \t\tdelete request;\n>>>> \n>>>> \tfor (auto &iter : mappedBuffers_) {\n>>>> @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n>>>> \t}\n>>>> \tmappedBuffers_.clear();\n>>>> \n>>>> +\tfor (Request *req : requests_)\n>>>> +\t\tdelete req;\n>> Have you tried stopping and restarting capture ? You delete all the\n>> requests here, but the vector still contains the pointers. When capture\n>> is restarted I expect issue. A\n>>\n>> \trequests_.clear();\n> Ah, right.\n>\n>> is needed, and I would also turn it into a\n>> std::vector> to avoid the manual delete loops.\n>> Same thing for the cam application (even if in that case capture can't\n>> be stopped and restarted at the moment, but let's still do it right).\n>>\n>>>> +\n>>>> \tdelete allocator_;\n>>>> \n>>>> \tisCapturing_ = false;\n>>>> @@ -701,7 +703,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->buffers(), request->metadata(), request });\n>> You aobut enqueuing the request only ? The reason to enqueue a copy of\n>> buffers() and metadata() was because the request was deleted right after\n>> this function returns, now that it's not the case anymore, we can just\n>> pass the request around. The MainWindow::CaptureRequest class can be\n>> removed.\n> ack\n>\n>>>> \t}\n>>>> \n>>>> \tQCoreApplication::postEvent(this, new CaptureEvent);\n>>>> @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n>>>> \n>>>> \t/* Process buffers. */\n>>>> \tif (request.buffers_.count(vfStream_))\n>>>> -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n>>>> +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n>>>> \n>>>> \tif (request.buffers_.count(rawStream_))\n>>>> \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n>> Instead of passing the request to processViewfinder(), how about adding\n>> it to a free list here, and taking a request from the free list in\n>> MainWindow::queueRequest() ? That would avoid the changes to the\n>> viewfinder.\n> ack\n>\n>>>> }\n>>>> \n>>>> -void MainWindow::processViewfinder(FrameBuffer *buffer)\n>>>> +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n>>>> {\n>>>> \tframesCaptured_++;\n>>>> \n>>>> @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n>>>> \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n>>>> \n>>>> \t/* Render the frame on the viewfinder. */\n>>>> -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n>>>> +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n>>>> }\n>>>> \n>>>> -void MainWindow::queueRequest(FrameBuffer *buffer)\n>>>> +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n>>>> {\n>>>> -\tRequest *request = camera_->createRequest();\n>>>> -\tif (!request) {\n>>>> -\t\tqWarning() << \"Can't create request\";\n>>>> -\t\treturn;\n>>>> -\t}\n>>>> +\trequest->reset();\n>>>> \n>>>> \trequest->addBuffer(vfStream_, buffer);\n>>>> \n>>>> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n>>>> index 5c61a4df..83535373 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>>>> @@ -49,13 +51,15 @@ public:\n>>>> \t}\n>>>> \n>>>> \tCaptureRequest(const Request::BufferMap &buffers,\n>>>> -\t\t const ControlList &metadata)\n>>>> -\t\t: buffers_(buffers), metadata_(metadata)\n>>>> +\t\t const ControlList &metadata,\n>>>> +\t\t Request * const request)\n>>>> +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n>>>> \t{\n>>>> \t}\n>>>> \n>>>> \tRequest::BufferMap buffers_;\n>>>> \tControlList metadata_;\n>>>> +\tRequest *request_;\n>>>> };\n>>>> \n>>>> class MainWindow : public QMainWindow\n>>>> @@ -79,7 +83,7 @@ private Q_SLOTS:\n>>>> \tvoid captureRaw();\n>>>> \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n>>>> \n>>>> -\tvoid queueRequest(FrameBuffer *buffer);\n>>>> +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n>>>> \n>>>> private:\n>>>> \tint createToolbars();\n>>>> @@ -96,7 +100,7 @@ private:\n>>>> \tvoid requestComplete(Request *request);\n>>>> \tvoid processCapture();\n>>>> \tvoid processHotplug(HotplugEvent *e);\n>>>> -\tvoid processViewfinder(FrameBuffer *buffer);\n>>>> +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n>>>> \n>>>> \t/* UI elements */\n>>>> \tQToolBar *toolbar_;\n>>>> @@ -135,6 +139,8 @@ private:\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/qcam/viewfinder.h b/src/qcam/viewfinder.h\n>>>> index 67da1df2..26c20f1b 100644\n>>>> --- a/src/qcam/viewfinder.h\n>>>> +++ b/src/qcam/viewfinder.h\n>>>> @@ -27,7 +27,9 @@ public:\n>>>> \tvirtual const QList &nativeFormats() const = 0;\n>>>> \n>>>> \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n>>>> -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n>>>> +\tvirtual void render(libcamera::Request *request,\n>>>> +\t\t\t libcamera::FrameBuffer *buffer,\n>>>> +\t\t\t MappedBuffer *map) = 0;\n>>>> \tvirtual void stop() = 0;\n>>>> \n>>>> \tvirtual QImage getCurrentImage() = 0;\n>>>> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n>>>> index fbe21dcf..c65b41f6 100644\n>>>> --- a/src/qcam/viewfinder_gl.cpp\n>>>> +++ b/src/qcam/viewfinder_gl.cpp\n>>>> @@ -23,7 +23,7 @@ static const QList supportedFormats{\n>>>> };\n>>>> \n>>>> ViewFinderGL::ViewFinderGL(QWidget *parent)\n>>>> -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n>>>> +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n>>>> \t fragmentShader_(nullptr), vertexShader_(nullptr),\n>>>> \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n>>>> \t textureU_(QOpenGLTexture::Target2D),\n>>>> @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n>>>> void ViewFinderGL::stop()\n>>>> {\n>>>> \tif (buffer_) {\n>>>> -\t\trenderComplete(buffer_);\n>>>> +\t\trenderComplete(request_, buffer_);\n>>>> \t\tbuffer_ = nullptr;\n>>>> \t}\n>>>> }\n>>>> @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n>>>> \treturn grabFramebuffer();\n>>>> }\n>>>> \n>>>> -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> {\n>>>> \tif (buffer->planes().size() != 1) {\n>>>> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n>>>> @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> \t}\n>>>> \n>>>> \tif (buffer_)\n>>>> -\t\trenderComplete(buffer_);\n>>>> +\t\trenderComplete(request_, buffer_);\n>>>> \n>>>> \tyuvData_ = static_cast(map->memory);\n>>>> \tupdate();\n>>>> \tbuffer_ = buffer;\n>>>> +\trequest_ = request;\n>>>> }\n>>>> \n>>>> bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n>>>> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n>>>> index 69502b7a..7db41841 100644\n>>>> --- a/src/qcam/viewfinder_gl.h\n>>>> +++ b/src/qcam/viewfinder_gl.h\n>>>> @@ -36,13 +36,13 @@ public:\n>>>> \tconst QList &nativeFormats() const override;\n>>>> \n>>>> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n>>>> -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n>>>> +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n>>>> \tvoid stop() override;\n>>>> \n>>>> \tQImage getCurrentImage() override;\n>>>> \n>>>> Q_SIGNALS:\n>>>> -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n>>>> +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n>>>> \n>>>> protected:\n>>>> \tvoid initializeGL() override;\n>>>> @@ -60,6 +60,7 @@ private:\n>>>> \tvoid doRender();\n>>>> \n>>>> \t/* Captured image size, format and buffer */\n>>>> +\tlibcamera::Request *request_;\n>>>> \tlibcamera::FrameBuffer *buffer_;\n>>>> \tlibcamera::PixelFormat format_;\n>>>> \tQSize size_;\n>>>> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n>>>> index e436714c..7f9f913b 100644\n>>>> --- a/src/qcam/viewfinder_qt.cpp\n>>>> +++ b/src/qcam/viewfinder_qt.cpp\n>>>> @@ -34,7 +34,7 @@ static const QMap nativeFormats\n>>>> };\n>>>> \n>>>> ViewFinderQt::ViewFinderQt(QWidget *parent)\n>>>> -\t: QWidget(parent), buffer_(nullptr)\n>>>> +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n>>>> {\n>>>> \ticon_ = QIcon(\":camera-off.svg\");\n>>>> }\n>>>> @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> +void ViewFinderQt::render(libcamera::Request *request,\n>>>> +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> {\n>>>> \tif (buffer->planes().size() != 1) {\n>>>> \t\tqWarning() << \"Multi-planar buffers are not supported\";\n>>>> @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> \t\t\t\t\tsize / size_.height(),\n>>>> \t\t\t\t\t::nativeFormats[format_]);\n>>>> \t\t\tstd::swap(buffer, buffer_);\n>>>> +\t\t\tstd::swap(request, request_);\n>>>> \t\t} else {\n>>>> \t\t\t/*\n>>>> \t\t\t * Otherwise, convert the format and release the frame\n>>>> @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n>>>> \tupdate();\n>>>> \n>>>> \tif (buffer)\n>>>> -\t\trenderComplete(buffer);\n>>>> +\t\trenderComplete(request, buffer);\n>>>> }\n>>>> \n>>>> void ViewFinderQt::stop()\n>>>> @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n>>>> \timage_ = QImage();\n>>>> \n>>>> \tif (buffer_) {\n>>>> -\t\trenderComplete(buffer_);\n>>>> +\t\trenderComplete(request_, buffer_);\n>>>> \t\tbuffer_ = nullptr;\n>>>> \t}\n>>>> \n>>>> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n>>>> index d7554288..5f62d7f6 100644\n>>>> --- a/src/qcam/viewfinder_qt.h\n>>>> +++ b/src/qcam/viewfinder_qt.h\n>>>> @@ -17,6 +17,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> \n>>>> #include \"format_converter.h\"\n>>>> #include \"viewfinder.h\"\n>>>> @@ -32,13 +33,15 @@ public:\n>>>> \tconst QList &nativeFormats() const override;\n>>>> \n>>>> \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n>>>> -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n>>>> +\tvoid render(libcamera::Request *request,\n>>>> +\t\t libcamera::FrameBuffer *buffer,\n>>>> +\t\t MappedBuffer *map) override;\n>>>> \tvoid stop() override;\n>>>> \n>>>> \tQImage getCurrentImage() override;\n>>>> \n>>>> Q_SIGNALS:\n>>>> -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n>>>> +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n>>>> \n>>>> protected:\n>>>> \tvoid paintEvent(QPaintEvent *) override;\n>>>> @@ -56,6 +59,7 @@ private:\n>>>> \tQPixmap pixmap_;\n>>>> \n>>>> \t/* Buffer and render image */\n>>>> +\tlibcamera::Request *request_;\n>>>> \tlibcamera::FrameBuffer *buffer_;\n>>>> \tQImage image_;\n>>>> \tQMutex mutex_; /* Prevent concurrent access to image_ */\n>> -- \n>> Regards,\n>>\n>> Laurent Pinchart\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 EBB70C3B5B\n\tfor ;\n\tFri, 25 Sep 2020 07:04:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67DE063000;\n\tFri, 25 Sep 2020 09:04:40 +0200 (CEST)","from mail.uajain.com (static.126.159.217.95.clients.your-server.de\n\t[95.217.159.126])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70BBE60CE7\n\tfor ;\n\tFri, 25 Sep 2020 09:04:38 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=uajain.com header.i=@uajain.com\n\theader.b=\"flryzjmW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=uajain.com; s=mail;\n\tt=1601017477; bh=BlxpQ0j0Euj+3Mt+DF/SPjTYO6asRecFvR/v4sUNupw=;\n\th=Subject:To:References:From:In-Reply-To;\n\tb=flryzjmWO1JXRF7n+SsnkjzaCAXC4Qu2uUhUNGNHMoS0Zx8SEjEW2s5mNutPUjPLL\n\t+DpgefVMpJvqcNOnDzrA29CVTi7pEoHvDX7bV0S2cMZL1RL5yOy3D+21LdcTTLbRBt\n\t0V/R9jzRCZ5aggw3r76nUdtJUU3ozHc9K45ZF45Sh4L2Sss1IqawuoHvFe0CtTezRf\n\tfr/hjLHIvMYudZ/I8ms830Wl0ris9bbDsOySfzY+cxk6QS+PWrS3LnXb5U2DeOYEA6\n\tVTggJoelOJf8wTfvx6/lNKWrek/EupRPh9MkQ+e3WtLHJvjOU5oLurYw3q30smCjcc\n\t8urPjeXePqRug==","To":"libcamera-devel@lists.libcamera.org","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>\n\t<20200924120951.GD502216@oden.dyn.berto.se>\n\t<20200924124056.GD3968@pendragon.ideasonboard.com>\n\t<20200925022038.GO45948@pyrite.rasen.tech>","From":"Umang Jain ","Message-ID":"<11cfdb8a-1eeb-fd57-b42f-92d2f99f6e68@uajain.com>","Date":"Fri, 25 Sep 2020 12:34:34 +0530","Mime-Version":"1.0","In-Reply-To":"<20200925022038.GO45948@pyrite.rasen.tech>","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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","Content-Transfer-Encoding":"base64","Content-Type":"text/plain; charset=\"utf-8\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12762,"web_url":"https://patchwork.libcamera.org/comment/12762/","msgid":"<20200925092949.GQ45948@pyrite.rasen.tech>","date":"2020-09-25T09:29:49","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: Reuse Request","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Sep 25, 2020 at 12:34:34PM +0530, Umang Jain wrote:\n> Hi,\n> \n> I have a question/confusion.\n> \n> On 9/25/20 7:50 AM, paul.elder@ideasonboard.com wrote:\n> > Hello,\n> > \n> > On Thu, Sep 24, 2020 at 03:40:56PM +0300, Laurent Pinchart wrote:\n> > > Hello,\n> > > \n> > > On Thu, Sep 24, 2020 at 02:09:51PM +0200, Niklas Söderlund wrote:\n> > > > On 2020-09-23 21:14:48 +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 the Request objects, so make cam and qcam do so.\n> > > > > \n> > > > > Signed-off-by: Paul Elder \n> > > > > \n> > > > > ---\n> > > > > In this RFC, I've only made qcam and cam reuse the Request object. Just\n> > > > > gauging the direction, and then I'll put it in for andoird and gstreamer\n> > > > > and v4l2-compat as well.\n> > > > > ---\n> > > > > include/libcamera/request.h | 2 ++\n> > > > > src/cam/capture.cpp | 20 ++++++++++----------\n> > > > > src/cam/capture.h | 3 +++\n> > > > > src/libcamera/camera.cpp | 4 +---\n> > > > > src/libcamera/request.cpp | 9 +++++++++\n> > > > > src/qcam/main_window.cpp | 26 ++++++++++++--------------\n> > > > > src/qcam/main_window.h | 14 ++++++++++----\n> > > > > src/qcam/viewfinder.h | 4 +++-\n> > > > > src/qcam/viewfinder_gl.cpp | 9 +++++----\n> > > > > src/qcam/viewfinder_gl.h | 5 +++--\n> > > > > src/qcam/viewfinder_qt.cpp | 10 ++++++----\n> > > > > src/qcam/viewfinder_qt.h | 8 ++++++--\n> > > > > 12 files changed, 70 insertions(+), 44 deletions(-)\n> > > > > \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> > > > > +\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/cam/capture.cpp b/src/cam/capture.cpp\n> > > > > index 5510c009..fe9d591a 100644\n> > > > > --- a/src/cam/capture.cpp\n> > > > > +++ b/src/cam/capture.cpp\n> > > > > @@ -65,6 +65,9 @@ int Capture::run(const OptionsParser::Options &options)\n> > > > > \t\twriter_ = nullptr;\n> > > > > \t}\n> > > > > +\tfor (Request *req : requests_)\n> > > > > +\t\tdelete req;\n> > > > > +\n> > > > > \tdelete allocator;\n> > > > > \treturn ret;\n> > > > > @@ -92,7 +95,6 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > > \t * example pushing a button. For now run all streams all the time.\n> > > > > \t */\n> > > > > -\tstd::vector requests;\n> > > > > \tfor (unsigned int i = 0; i < nbuffers; i++) {\n> > > > > \t\tRequest *request = camera_->createRequest();\n> > > > > \t\tif (!request) {\n> > > > > @@ -117,7 +119,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > > \t\t\t\twriter_->mapBuffer(buffer.get());\n> > > > > \t\t}\n> > > > > -\t\trequests.push_back(request);\n> > > > > +\t\trequests_.push_back(request);\n> > > > > \t}\n> > > > > \tret = camera_->start();\n> > > > > @@ -126,7 +128,7 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > > \t\treturn ret;\n> > > > > \t}\n> > > > > -\tfor (Request *request : requests) {\n> > > > > +\tfor (Request *request : requests_) {\n> > > > > \t\tret = camera_->queueRequest(request);\n> > > > > \t\tif (ret < 0) {\n> > > > > \t\t\tstd::cerr << \"Can't queue request\" << std::endl;\n> > > > > @@ -153,10 +155,12 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > > void Capture::requestComplete(Request *request)\n> > > > > {\n> > > > > -\tif (request->status() == Request::RequestCancelled)\n> > > > > +\tif (request->status() == Request::RequestCancelled) {\n> > > > > +\t\trequest->reset();\n> > > > > \t\treturn;\n> > > > > +\t}\n> > > > > -\tconst Request::BufferMap &buffers = request->buffers();\n> > > > > +\tconst Request::BufferMap buffers = request->buffers();\n> > > A copy isn't very nice, but we can live with it for now.\n> > Without the copy, all the buffers become invalid upon reset :/\n> If I copy the BufferMap as is it here, would be also copy all the memory\n> allocated to BufferMap's FrameBuffer(s)?\n> The reason I ask, because reset() seems to bufferMap_.clear() which means,\n> it will destroy all members it is holding. Then once FrameBuffer(s) are\n> destroyed via reset(), I would expect some re-allocation of memory happening\n> somewhere for those FrameBuffer(s). Is there where this copy comes into\n> picture?\n\nBufferMap only contains regular pointers, so clear()ing the map will\ndelete the pointers, but it won't free the memory that those pointers\nwere pointing to. By copying the BufferMap, all the pointers get copied\ntoo, but only the pointers. The FrameBuffers continue to exist, and\ncopying this BufferMap copies their pointers so that they're not lost\nwhen the Request is reset().\n\n\nPaul\n\n> > > > > \t/*\n> > > > > \t * Compute the frame rate. The timestamp is arbitrarily retrieved from\n> > > > > @@ -206,11 +210,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> > > > Hum, is this really \"reusing\" the request object? I imagined a reused\n> > > > request would retain its buffers while gaining the ability to be be\n> > > > requeued to the Camera after it have completed. With this patch we still\n> > > > need to keep the logic to fill the request with buffers each iteration,\n> > > > is there some advantage to this I'm missing?\n> > > Two advantages, we don't free and reallocate the request needlessly, and\n> > > it can be stored in the application and used after the request handler\n> > > completes.\n> > > \n> > > I don't think retaining buffers by default is a good idea, in many cases\n> > > we'll have a different number of buffers that can be cycled through than\n> > > the number of allocated requests.\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..62cf9bf1 100644\n> > > > > --- a/src/cam/capture.h\n> > > > > +++ b/src/cam/capture.h\n> > > > > @@ -9,6 +9,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > +#include \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> > > > > #endif /* __CAM_CAPTURE_H__ */\n> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > > > index ae16a64a..4cc68bce 100644\n> > > > > --- a/src/libcamera/camera.cpp\n> > > > > +++ b/src/libcamera/camera.cpp\n> > > > > @@ -974,13 +974,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> > > > > } /* namespace libcamera */\n> > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > > > > index 60b30692..606866a6 100644\n> > > > > --- a/src/libcamera/request.cpp\n> > > > > +++ b/src/libcamera/request.cpp\n> > > > > @@ -85,6 +85,15 @@ Request::~Request()\n> > > > > \tdelete validator_;\n> > > > > }\n> > > No documentation ? :-) You should also update the documentation of\n> > Not yet :p\n> > \n> > > Camera::queueRequest(). Thinking about it, I think we should enforce\n> > > ownership of requests by the application by returning a\n> > > std::unique_ptr from Camera::createRequest(). We may even want\n> > > to drop Camera::createRequest() completely, but that can be done in a\n> > > separate patch.\n> > Yeah... but I thought there was a problem when the Request completes,\n> > since there could be multiple slots, and they all can't receive unique\n> > pointers.\n> > \n> > On the other hand... it's only one application... could we require that\n> > the application only connect one slot to the Request completion handler,\n> > and then the application can split it among multiple sub-handlers if it\n> > wants to? Then we can keep unique_ptr for the Request, and the ownership\n> > can be clear.\n> > \n> > > > > +void Request::reset()\n> > > > > +{\n> > > > > +\tbufferMap_.clear();\n> > > > > +\tpending_.clear();\n> > > You need to clear controls_ and metadata_ too.\n> > Oh okay, those can't be reused?\n> > \n> > > > > +\n> > > > > +\tstatus_ = RequestPending;\n> > > > > +\tcancelled_ = false;\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..1531b44f 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> > > > > \t/* Verify roles are supported. */\n> > > > > @@ -499,7 +498,7 @@ int MainWindow::startCapture()\n> > > > > \t\t\tgoto error;\n> > > > > \t\t}\n> > > > > -\t\trequests.push_back(request);\n> > > > > +\t\trequests_.push_back(request);\n> > > > > \t}\n> > > > > \t/* Start the title timer and the camera. */\n> > > > > @@ -518,7 +517,7 @@ int MainWindow::startCapture()\n> > > > > \tcamera_->requestCompleted.connect(this, &MainWindow::requestComplete);\n> > > > > \t/* Queue all requests. */\n> > > > > -\tfor (Request *request : requests) {\n> > > > > +\tfor (Request *request : requests_) {\n> > > > > \t\tret = camera_->queueRequest(request);\n> > > > > \t\tif (ret < 0) {\n> > > > > \t\t\tqWarning() << \"Can't queue request\";\n> > > > > @@ -535,7 +534,7 @@ error_disconnect:\n> > > > > \tcamera_->stop();\n> > > > > error:\n> > > > > -\tfor (Request *request : requests)\n> > > > > +\tfor (Request *request : requests_)\n> > > > > \t\tdelete request;\n> > > > > \tfor (auto &iter : mappedBuffers_) {\n> > > > > @@ -580,6 +579,9 @@ void MainWindow::stopCapture()\n> > > > > \t}\n> > > > > \tmappedBuffers_.clear();\n> > > > > +\tfor (Request *req : requests_)\n> > > > > +\t\tdelete req;\n> > > Have you tried stopping and restarting capture ? You delete all the\n> > > requests here, but the vector still contains the pointers. When capture\n> > > is restarted I expect issue. A\n> > > \n> > > \trequests_.clear();\n> > Ah, right.\n> > \n> > > is needed, and I would also turn it into a\n> > > std::vector> to avoid the manual delete loops.\n> > > Same thing for the cam application (even if in that case capture can't\n> > > be stopped and restarted at the moment, but let's still do it right).\n> > > \n> > > > > +\n> > > > > \tdelete allocator_;\n> > > > > \tisCapturing_ = false;\n> > > > > @@ -701,7 +703,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->buffers(), request->metadata(), request });\n> > > You aobut enqueuing the request only ? The reason to enqueue a copy of\n> > > buffers() and metadata() was because the request was deleted right after\n> > > this function returns, now that it's not the case anymore, we can just\n> > > pass the request around. The MainWindow::CaptureRequest class can be\n> > > removed.\n> > ack\n> > \n> > > > > \t}\n> > > > > \tQCoreApplication::postEvent(this, new CaptureEvent);\n> > > > > @@ -726,13 +728,13 @@ void MainWindow::processCapture()\n> > > > > \t/* Process buffers. */\n> > > > > \tif (request.buffers_.count(vfStream_))\n> > > > > -\t\tprocessViewfinder(request.buffers_[vfStream_]);\n> > > > > +\t\tprocessViewfinder(request.request_, request.buffers_[vfStream_]);\n> > > > > \tif (request.buffers_.count(rawStream_))\n> > > > > \t\tprocessRaw(request.buffers_[rawStream_], request.metadata_);\n> > > Instead of passing the request to processViewfinder(), how about adding\n> > > it to a free list here, and taking a request from the free list in\n> > > MainWindow::queueRequest() ? That would avoid the changes to the\n> > > viewfinder.\n> > ack\n> > \n> > > > > }\n> > > > > -void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > > > +void MainWindow::processViewfinder(Request *request, FrameBuffer *buffer)\n> > > > > {\n> > > > > \tframesCaptured_++;\n> > > > > @@ -749,16 +751,12 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > > > \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > > > > \t/* Render the frame on the viewfinder. */\n> > > > > -\tviewfinder_->render(buffer, &mappedBuffers_[buffer]);\n> > > > > +\tviewfinder_->render(request, buffer, &mappedBuffers_[buffer]);\n> > > > > }\n> > > > > -void MainWindow::queueRequest(FrameBuffer *buffer)\n> > > > > +void MainWindow::queueRequest(Request *request, FrameBuffer *buffer)\n> > > > > {\n> > > > > -\tRequest *request = camera_->createRequest();\n> > > > > -\tif (!request) {\n> > > > > -\t\tqWarning() << \"Can't create request\";\n> > > > > -\t\treturn;\n> > > > > -\t}\n> > > > > +\trequest->reset();\n> > > > > \trequest->addBuffer(vfStream_, buffer);\n> > > > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > > > > index 5c61a4df..83535373 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> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -22,6 +23,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > > #include \"../cam/stream_options.h\"\n> > > > > @@ -49,13 +51,15 @@ public:\n> > > > > \t}\n> > > > > \tCaptureRequest(const Request::BufferMap &buffers,\n> > > > > -\t\t const ControlList &metadata)\n> > > > > -\t\t: buffers_(buffers), metadata_(metadata)\n> > > > > +\t\t const ControlList &metadata,\n> > > > > +\t\t Request * const request)\n> > > > > +\t\t: buffers_(buffers), metadata_(metadata), request_(request)\n> > > > > \t{\n> > > > > \t}\n> > > > > \tRequest::BufferMap buffers_;\n> > > > > \tControlList metadata_;\n> > > > > +\tRequest *request_;\n> > > > > };\n> > > > > class MainWindow : public QMainWindow\n> > > > > @@ -79,7 +83,7 @@ private Q_SLOTS:\n> > > > > \tvoid captureRaw();\n> > > > > \tvoid processRaw(FrameBuffer *buffer, const ControlList &metadata);\n> > > > > -\tvoid queueRequest(FrameBuffer *buffer);\n> > > > > +\tvoid queueRequest(Request *request, FrameBuffer *buffer);\n> > > > > private:\n> > > > > \tint createToolbars();\n> > > > > @@ -96,7 +100,7 @@ private:\n> > > > > \tvoid requestComplete(Request *request);\n> > > > > \tvoid processCapture();\n> > > > > \tvoid processHotplug(HotplugEvent *e);\n> > > > > -\tvoid processViewfinder(FrameBuffer *buffer);\n> > > > > +\tvoid processViewfinder(Request *request, FrameBuffer *buffer);\n> > > > > \t/* UI elements */\n> > > > > \tQToolBar *toolbar_;\n> > > > > @@ -135,6 +139,8 @@ private:\n> > > > > \tQElapsedTimer frameRateInterval_;\n> > > > > \tuint32_t previousFrames_;\n> > > > > \tuint32_t framesCaptured_;\n> > > > > +\n> > > > > +\tstd::vector requests_;\n> > > > > };\n> > > > > #endif /* __QCAM_MAIN_WINDOW__ */\n> > > > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > > > > index 67da1df2..26c20f1b 100644\n> > > > > --- a/src/qcam/viewfinder.h\n> > > > > +++ b/src/qcam/viewfinder.h\n> > > > > @@ -27,7 +27,9 @@ public:\n> > > > > \tvirtual const QList &nativeFormats() const = 0;\n> > > > > \tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > > > > -\tvirtual void render(libcamera::FrameBuffer *buffer, MappedBuffer *map) = 0;\n> > > > > +\tvirtual void render(libcamera::Request *request,\n> > > > > +\t\t\t libcamera::FrameBuffer *buffer,\n> > > > > +\t\t\t MappedBuffer *map) = 0;\n> > > > > \tvirtual void stop() = 0;\n> > > > > \tvirtual QImage getCurrentImage() = 0;\n> > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > > > index fbe21dcf..c65b41f6 100644\n> > > > > --- a/src/qcam/viewfinder_gl.cpp\n> > > > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > > > @@ -23,7 +23,7 @@ static const QList supportedFormats{\n> > > > > };\n> > > > > ViewFinderGL::ViewFinderGL(QWidget *parent)\n> > > > > -\t: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),\n> > > > > +\t: QOpenGLWidget(parent), request_(nullptr), buffer_(nullptr), yuvData_(nullptr),\n> > > > > \t fragmentShader_(nullptr), vertexShader_(nullptr),\n> > > > > \t vertexBuffer_(QOpenGLBuffer::VertexBuffer),\n> > > > > \t textureU_(QOpenGLTexture::Target2D),\n> > > > > @@ -67,7 +67,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > > > > void ViewFinderGL::stop()\n> > > > > {\n> > > > > \tif (buffer_) {\n> > > > > -\t\trenderComplete(buffer_);\n> > > > > +\t\trenderComplete(request_, buffer_);\n> > > > > \t\tbuffer_ = nullptr;\n> > > > > \t}\n> > > > > }\n> > > > > @@ -79,7 +79,7 @@ QImage ViewFinderGL::getCurrentImage()\n> > > > > \treturn grabFramebuffer();\n> > > > > }\n> > > > > -void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > +void ViewFinderGL::render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > {\n> > > > > \tif (buffer->planes().size() != 1) {\n> > > > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > > > @@ -87,11 +87,12 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > \t}\n> > > > > \tif (buffer_)\n> > > > > -\t\trenderComplete(buffer_);\n> > > > > +\t\trenderComplete(request_, buffer_);\n> > > > > \tyuvData_ = static_cast(map->memory);\n> > > > > \tupdate();\n> > > > > \tbuffer_ = buffer;\n> > > > > +\trequest_ = request;\n> > > > > }\n> > > > > bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)\n> > > > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > > > > index 69502b7a..7db41841 100644\n> > > > > --- a/src/qcam/viewfinder_gl.h\n> > > > > +++ b/src/qcam/viewfinder_gl.h\n> > > > > @@ -36,13 +36,13 @@ public:\n> > > > > \tconst QList &nativeFormats() const override;\n> > > > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > > +\tvoid render(libcamera::Request *request, libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > > \tvoid stop() override;\n> > > > > \tQImage getCurrentImage() override;\n> > > > > Q_SIGNALS:\n> > > > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > > > protected:\n> > > > > \tvoid initializeGL() override;\n> > > > > @@ -60,6 +60,7 @@ private:\n> > > > > \tvoid doRender();\n> > > > > \t/* Captured image size, format and buffer */\n> > > > > +\tlibcamera::Request *request_;\n> > > > > \tlibcamera::FrameBuffer *buffer_;\n> > > > > \tlibcamera::PixelFormat format_;\n> > > > > \tQSize size_;\n> > > > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > > > index e436714c..7f9f913b 100644\n> > > > > --- a/src/qcam/viewfinder_qt.cpp\n> > > > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > > > @@ -34,7 +34,7 @@ static const QMap nativeFormats\n> > > > > };\n> > > > > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > > > > -\t: QWidget(parent), buffer_(nullptr)\n> > > > > +\t: QWidget(parent), request_(nullptr), buffer_(nullptr)\n> > > > > {\n> > > > > \ticon_ = QIcon(\":camera-off.svg\");\n> > > > > }\n> > > > > @@ -78,7 +78,8 @@ int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > > > > \treturn 0;\n> > > > > }\n> > > > > -void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > +void ViewFinderQt::render(libcamera::Request *request,\n> > > > > +\t\t\t libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > {\n> > > > > \tif (buffer->planes().size() != 1) {\n> > > > > \t\tqWarning() << \"Multi-planar buffers are not supported\";\n> > > > > @@ -106,6 +107,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > \t\t\t\t\tsize / size_.height(),\n> > > > > \t\t\t\t\t::nativeFormats[format_]);\n> > > > > \t\t\tstd::swap(buffer, buffer_);\n> > > > > +\t\t\tstd::swap(request, request_);\n> > > > > \t\t} else {\n> > > > > \t\t\t/*\n> > > > > \t\t\t * Otherwise, convert the format and release the frame\n> > > > > @@ -118,7 +120,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > > > > \tupdate();\n> > > > > \tif (buffer)\n> > > > > -\t\trenderComplete(buffer);\n> > > > > +\t\trenderComplete(request, buffer);\n> > > > > }\n> > > > > void ViewFinderQt::stop()\n> > > > > @@ -126,7 +128,7 @@ void ViewFinderQt::stop()\n> > > > > \timage_ = QImage();\n> > > > > \tif (buffer_) {\n> > > > > -\t\trenderComplete(buffer_);\n> > > > > +\t\trenderComplete(request_, buffer_);\n> > > > > \t\tbuffer_ = nullptr;\n> > > > > \t}\n> > > > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > > > > index d7554288..5f62d7f6 100644\n> > > > > --- a/src/qcam/viewfinder_qt.h\n> > > > > +++ b/src/qcam/viewfinder_qt.h\n> > > > > @@ -17,6 +17,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \"format_converter.h\"\n> > > > > #include \"viewfinder.h\"\n> > > > > @@ -32,13 +33,15 @@ public:\n> > > > > \tconst QList &nativeFormats() const override;\n> > > > > \tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > > > > -\tvoid render(libcamera::FrameBuffer *buffer, MappedBuffer *map) override;\n> > > > > +\tvoid render(libcamera::Request *request,\n> > > > > +\t\t libcamera::FrameBuffer *buffer,\n> > > > > +\t\t MappedBuffer *map) override;\n> > > > > \tvoid stop() override;\n> > > > > \tQImage getCurrentImage() override;\n> > > > > Q_SIGNALS:\n> > > > > -\tvoid renderComplete(libcamera::FrameBuffer *buffer);\n> > > > > +\tvoid renderComplete(libcamera::Request *request, libcamera::FrameBuffer *buffer);\n> > > > > protected:\n> > > > > \tvoid paintEvent(QPaintEvent *) override;\n> > > > > @@ -56,6 +59,7 @@ private:\n> > > > > \tQPixmap pixmap_;\n> > > > > \t/* Buffer and render image */\n> > > > > +\tlibcamera::Request *request_;\n> > > > > \tlibcamera::FrameBuffer *buffer_;\n> > > > > \tQImage image_;\n> > > > > \tQMutex mutex_; /* Prevent concurrent access to image_ */\n> > > -- \n> > > Regards,\n> > > \n> > > Laurent Pinchart\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\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 3B86EC3B5B\n\tfor ;\n\tFri, 25 Sep 2020 09:30:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B52AA63019;\n\tFri, 25 Sep 2020 11:29:59 +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 EB46E60576\n\tfor ;\n\tFri, 25 Sep 2020 11:29:58 +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 397AF2D7;\n\tFri, 25 Sep 2020 11:29:55 +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=\"i6Ok2nBi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601026197;\n\tbh=KULBB9vocalu0FQTSEuwmTy5oaP24a4ETwoSjtqN8/E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=i6Ok2nBiR2mbpkUX8ODBtXSyTOUHoGh2gbnPC9ds/UceofpNOvj2X/ElBWQetpUoN\n\tJCJUZ9I/mV0ejB62YjzWlhulTtljHtJ6xubDxytDD+uvh/fsa93sTQ8oYyc1OsuB8g\n\t9rjA6nzuqsd42cJZfficpnamRDrxmBSErr/tAzrs=","Date":"Fri, 25 Sep 2020 18:29:49 +0900","From":"paul.elder@ideasonboard.com","To":"Umang Jain ","Message-ID":"<20200925092949.GQ45948@pyrite.rasen.tech>","References":"<20200923121448.281346-1-paul.elder@ideasonboard.com>\n\t<20200924120951.GD502216@oden.dyn.berto.se>\n\t<20200924124056.GD3968@pendragon.ideasonboard.com>\n\t<20200925022038.GO45948@pyrite.rasen.tech>\n\t<11cfdb8a-1eeb-fd57-b42f-92d2f99f6e68@uajain.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<11cfdb8a-1eeb-fd57-b42f-92d2f99f6e68@uajain.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera, cam,\n\tqcam: 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\" "}}]