Patch Detail
Show a patch.
GET /api/1.1/patches/1688/?format=api
{ "id": 1688, "url": "https://patchwork.libcamera.org/api/1.1/patches/1688/?format=api", "web_url": "https://patchwork.libcamera.org/patch/1688/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20190713172351.25452-8-laurent.pinchart@ideasonboard.com>", "date": "2019-07-13T17:23:42", "name": "[libcamera-devel,v2,07/16] libcamera: v4l2_videodevice: Signal buffer completion at streamoff time", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "f9f0b2ff94a137066f9fd2a0ff2b6834994735c1", "submitter": { "id": 2, "url": "https://patchwork.libcamera.org/api/1.1/people/2/?format=api", "name": "Laurent Pinchart", "email": "laurent.pinchart@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/1688/mbox/", "series": [ { "id": 430, "url": "https://patchwork.libcamera.org/api/1.1/series/430/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=430", "date": "2019-07-13T17:23:35", "name": "Add support for external buffers", "version": 2, "mbox": "https://patchwork.libcamera.org/series/430/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/1688/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/1688/checks/", "tags": {}, "headers": { "Return-Path": "<laurent.pinchart@ideasonboard.com>", "Received": [ "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 9D6D06175E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Jul 2019 19:24:36 +0200 (CEST)", "from pendragon.ideasonboard.com (softbank126209254147.bbtec.net\n\t[126.209.254.147])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A8A02B2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSat, 13 Jul 2019 19:24:35 +0200 (CEST)" ], "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1563038676;\n\tbh=ZGaT60/q1Y42I4mA9Yzu7B/9ClD1S7qlOSh5pU++Zfk=;\n\th=From:To:Subject:Date:In-Reply-To:References:From;\n\tb=QD5KdXHiUfqkQVr2ULxB7oXsIv0p9NnCgKqkiZbJAaGzLRHrR4XySzsMKUkR9D/DZ\n\tywOQC6pMMdCWWuzzYgSw9D1UUV4eU6bYGvDaKnIPbgjD26WG6XgL/CnwjnfSga1ZwG\n\tOfrp4tNfN5c5OgH12rEkrAtg9jWfaUZDz4t5HOU8=", "From": "Laurent Pinchart <laurent.pinchart@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Date": "Sat, 13 Jul 2019 20:23:42 +0300", "Message-Id": "<20190713172351.25452-8-laurent.pinchart@ideasonboard.com>", "X-Mailer": "git-send-email 2.21.0", "In-Reply-To": "<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>", "References": "<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "Subject": "[libcamera-devel] [PATCH v2 07/16] libcamera: v4l2_videodevice:\n\tSignal buffer completion at streamoff time", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.23", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "X-List-Received-Date": "Sat, 13 Jul 2019 17:24:36 -0000" }, "content": "When stopping the stream buffers have been queued, in which case their\ncompletion is never be notified to the user. This can lead to memory\nleaks. Fix it by notifying completion of all queued buffers with the\nstatus set to error.\n\nAs a result the base PipelineHandler implementation can be simplified,\nas all requests complete as the result of stopping the stream. The\nstop() method that manually completes all queued requests isn't needed\nanymore.\n\nSigned-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n---\n include/libcamera/request.h | 3 ++-\n src/libcamera/include/pipeline_handler.h | 2 +-\n src/libcamera/pipeline/ipu3/ipu3.cpp | 2 --\n src/libcamera/pipeline/rkisp1/rkisp1.cpp | 2 --\n src/libcamera/pipeline/uvcvideo.cpp | 1 -\n src/libcamera/pipeline/vimc.cpp | 1 -\n src/libcamera/pipeline_handler.cpp | 29 +++---------------------\n src/libcamera/request.cpp | 14 ++++++++----\n src/libcamera/v4l2_videodevice.cpp | 14 +++++++++++-\n test/v4l2_videodevice/buffer_sharing.cpp | 6 +++++\n 10 files changed, 34 insertions(+), 40 deletions(-)", "diff": "diff --git a/include/libcamera/request.h b/include/libcamera/request.h\nindex 7a60be617645..00c68345b5b4 100644\n--- a/include/libcamera/request.h\n+++ b/include/libcamera/request.h\n@@ -51,7 +51,7 @@ private:\n \tfriend class PipelineHandler;\n \n \tint prepare();\n-\tvoid complete(Status status);\n+\tvoid complete();\n \n \tbool completeBuffer(Buffer *buffer);\n \n@@ -62,6 +62,7 @@ private:\n \n \tuint64_t cookie_;\n \tStatus status_;\n+\tbool cancelled_;\n };\n \n } /* namespace libcamera */\ndiff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\nindex f836d5d1a600..1fdef9cea40f 100644\n--- a/src/libcamera/include/pipeline_handler.h\n+++ b/src/libcamera/include/pipeline_handler.h\n@@ -74,7 +74,7 @@ public:\n \t\t\t\tconst std::set<Stream *> &streams) = 0;\n \n \tvirtual int start(Camera *camera) = 0;\n-\tvirtual void stop(Camera *camera);\n+\tvirtual void stop(Camera *camera) = 0;\n \n \tvirtual int queueRequest(Camera *camera, Request *request);\n \ndiff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\nindex 50457891e288..ffc066a15ae9 100644\n--- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n@@ -711,8 +711,6 @@ void PipelineHandlerIPU3::stop(Camera *camera)\n \tif (ret)\n \t\tLOG(IPU3, Warning) << \"Failed to stop camera \"\n \t\t\t\t << camera->name();\n-\n-\tPipelineHandler::stop(camera);\n }\n \n int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)\ndiff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\nindex 383236435a7f..cc33a2cb2572 100644\n--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n@@ -354,8 +354,6 @@ void PipelineHandlerRkISP1::stop(Camera *camera)\n \t\tLOG(RkISP1, Warning)\n \t\t\t<< \"Failed to stop camera \" << camera->name();\n \n-\tPipelineHandler::stop(camera);\n-\n \tactiveCamera_ = nullptr;\n }\n \ndiff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\nindex 387d71ef567c..b299c5da8471 100644\n--- a/src/libcamera/pipeline/uvcvideo.cpp\n+++ b/src/libcamera/pipeline/uvcvideo.cpp\n@@ -220,7 +220,6 @@ void PipelineHandlerUVC::stop(Camera *camera)\n {\n \tUVCCameraData *data = cameraData(camera);\n \tdata->video_->streamOff();\n-\tPipelineHandler::stop(camera);\n }\n \n int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\ndiff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\nindex ff2529c974fd..7d96c3645c06 100644\n--- a/src/libcamera/pipeline/vimc.cpp\n+++ b/src/libcamera/pipeline/vimc.cpp\n@@ -222,7 +222,6 @@ void PipelineHandlerVimc::stop(Camera *camera)\n {\n \tVimcCameraData *data = cameraData(camera);\n \tdata->video_->streamOff();\n-\tPipelineHandler::stop(camera);\n }\n \n int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request)\ndiff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\nindex c609200252f1..83938a71f615 100644\n--- a/src/libcamera/pipeline_handler.cpp\n+++ b/src/libcamera/pipeline_handler.cpp\n@@ -328,31 +328,7 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)\n *\n * This method stops capturing and processing requests immediately. All pending\n * requests are cancelled and complete immediately in an error state.\n- *\n- * Pipeline handlers shall override this method to stop the pipeline, ensure\n- * that all pending request completion signaled through completeRequest() have\n- * returned, and call the base implementation of the stop() method as the last\n- * step of their implementation. The base implementation cancels all requests\n- * queued but not yet complete.\n */\n-void PipelineHandler::stop(Camera *camera)\n-{\n-\tCameraData *data = cameraData(camera);\n-\n-\twhile (!data->queuedRequests_.empty()) {\n-\t\tRequest *request = data->queuedRequests_.front();\n-\t\tdata->queuedRequests_.pop_front();\n-\n-\t\twhile (!request->pending_.empty()) {\n-\t\t\tBuffer *buffer = *request->pending_.begin();\n-\t\t\tbuffer->cancel();\n-\t\t\tcompleteBuffer(camera, request, buffer);\n-\t\t}\n-\n-\t\trequest->complete(Request::RequestCancelled);\n-\t\tcamera->requestComplete(request);\n-\t}\n-}\n \n /**\n * \\fn PipelineHandler::queueRequest()\n@@ -420,15 +396,16 @@ bool PipelineHandler::completeBuffer(Camera *camera, Request *request,\n */\n void PipelineHandler::completeRequest(Camera *camera, Request *request)\n {\n-\trequest->complete(Request::RequestComplete);\n+\trequest->complete();\n \n \tCameraData *data = cameraData(camera);\n \n \twhile (!data->queuedRequests_.empty()) {\n \t\trequest = data->queuedRequests_.front();\n-\t\tif (request->hasPendingBuffers())\n+\t\tif (request->status() == Request::RequestPending)\n \t\t\tbreak;\n \n+\t\tASSERT(!request->hasPendingBuffers());\n \t\tdata->queuedRequests_.pop_front();\n \t\tcamera->requestComplete(request);\n \t}\ndiff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\nindex 45e7133febb0..19131472710b 100644\n--- a/src/libcamera/request.cpp\n+++ b/src/libcamera/request.cpp\n@@ -56,7 +56,7 @@ LOG_DEFINE_CATEGORY(Request)\n */\n Request::Request(Camera *camera, uint64_t cookie)\n \t: camera_(camera), controls_(camera), cookie_(cookie),\n-\t status_(RequestPending)\n+\t status_(RequestPending), cancelled_(false)\n {\n }\n \n@@ -199,14 +199,15 @@ int Request::prepare()\n \n /**\n * \\brief Complete a queued request\n- * \\param[in] status The request completion status\n *\n- * Mark the request as complete by updating its status to \\a status.\n+ * Mark the request as complete by updating its status to RequestComplete,\n+ * unless buffers have been cancelled in which case the status is set to\n+ * RequestCancelled.\n */\n-void Request::complete(Status status)\n+void Request::complete()\n {\n \tASSERT(!hasPendingBuffers());\n-\tstatus_ = status;\n+\tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n }\n \n /**\n@@ -229,6 +230,9 @@ bool Request::completeBuffer(Buffer *buffer)\n \n \tbuffer->setRequest(nullptr);\n \n+\tif (buffer->status() == Buffer::BufferCancelled)\n+\t\tcancelled_ = true;\n+\n \treturn !hasPendingBuffers();\n }\n \ndiff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\nindex 65b4098abc05..aa3136378997 100644\n--- a/src/libcamera/v4l2_videodevice.cpp\n+++ b/src/libcamera/v4l2_videodevice.cpp\n@@ -1090,7 +1090,9 @@ int V4L2VideoDevice::streamOn()\n * \\brief Stop the video stream\n *\n * Buffers that are still queued when the video stream is stopped are\n- * implicitly dequeued, but no bufferReady signal is emitted for them.\n+ * immediately dequeued with their status set to Buffer::BufferError,\n+ * and the bufferReady signal is emitted for them. The order in which those\n+ * buffers are dequeued is not specified.\n *\n * \\return 0 on success or a negative error code otherwise\n */\n@@ -1105,6 +1107,16 @@ int V4L2VideoDevice::streamOff()\n \t\treturn ret;\n \t}\n \n+\t/* Send back all queued buffers. */\n+\tfor (auto it : queuedBuffers_) {\n+\t\tunsigned int index = it.first;\n+\t\tBuffer *buffer = it.second;\n+\n+\t\tbuffer->index_ = index;\n+\t\tbuffer->cancel();\n+\t\tbufferReady.emit(buffer);\n+\t}\n+\n \tqueuedBuffers_.clear();\n \tfdEvent_->setEnabled(false);\n \ndiff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp\nindex 459bd238fe97..4da6ba466f40 100644\n--- a/test/v4l2_videodevice/buffer_sharing.cpp\n+++ b/test/v4l2_videodevice/buffer_sharing.cpp\n@@ -95,6 +95,9 @@ protected:\n \t\tstd::cout << \"Received capture buffer: \" << buffer->index()\n \t\t\t << \" sequence \" << buffer->sequence() << std::endl;\n \n+\t\tif (buffer->status() != Buffer::BufferSuccess)\n+\t\t\treturn;\n+\n \t\toutput_->queueBuffer(buffer);\n \t\tframesCaptured_++;\n \t}\n@@ -104,6 +107,9 @@ protected:\n \t\tstd::cout << \"Received output buffer: \" << buffer->index()\n \t\t\t << \" sequence \" << buffer->sequence() << std::endl;\n \n+\t\tif (buffer->status() != Buffer::BufferSuccess)\n+\t\t\treturn;\n+\n \t\tcapture_->queueBuffer(buffer);\n \t\tframesOutput_++;\n \t}\n", "prefixes": [ "libcamera-devel", "v2", "07/16" ] }