[{"id":2260,"web_url":"https://patchwork.libcamera.org/comment/2260/","msgid":"<20190714104457.GE31102@wyvern>","date":"2019-07-14T10:44:57","subject":"Re: [libcamera-devel] [PATCH v2 07/16] libcamera: v4l2_videodevice:\n\tSignal buffer completion at streamoff time","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your patch.\n\nOn 2019-07-13 20:23:42 +0300, Laurent Pinchart wrote:\n> When stopping the stream buffers have been queued, in which case their\n> completion is never be notified to the user. This can lead to memory\n> leaks. Fix it by notifying completion of all queued buffers with the\n> status set to error.\n> \n> As a result the base PipelineHandler implementation can be simplified,\n> as all requests complete as the result of stopping the stream. The\n> stop() method that manually completes all queued requests isn't needed\n> anymore.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\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(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 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 */\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 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>  \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 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)\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 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>  \n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 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)\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 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)\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 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}\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 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>  \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 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>  \n> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp\n> index 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> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<niklas.soderlund@ragnatech.se>","Received":["from mail-pf1-x443.google.com (mail-pf1-x443.google.com\n\t[IPv6:2607:f8b0:4864:20::443])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AEDA961572\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Jul 2019 12:45:03 +0200 (CEST)","by mail-pf1-x443.google.com with SMTP id p184so6144109pfp.7\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 14 Jul 2019 03:45:03 -0700 (PDT)","from localhost (softbank126159224182.bbtec.net. [126.159.224.182])\n\tby smtp.gmail.com with ESMTPSA id\n\tbo20sm10926298pjb.23.2019.07.14.03.45.00\n\t(version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256);\n\tSun, 14 Jul 2019 03:45:01 -0700 (PDT)"],"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\t:user-agent; bh=Eg6foNAEj3/D1uiuXx4xeYxiVcCdrfp5fii2Fy1I/MM=;\n\tb=kGifFOByZhGANzhVzJqQjsrD2+WxFzMyY9EUM21rwCMghOClKi8LCOit2SqqOk5dgj\n\t/baIF81i9uQ3yFGdbjjifRzmak2y1kgyaW2iGZp4kOHtEUMmIc8OjKbVMvnusrkmsPTb\n\tYAzMYA/g9QNuGji6vwz3G8VJG5huHjpLmb9TQQ3eFIipZP3rGwkxqRsi8qE1bREK3UI2\n\tYYQdS1os9C4F46E7rA1oUxj+y+qEox128TgblrUidIrN6G7+xlVMWyR7gSIVZqlQE8JE\n\tT+YwXITLKC2Gt6bOnvv1NfHsLqHK2ipPkZgUliEk5JIS06RJFJ8Gp1vFvMmW8+QhhN//\n\tOt3A==","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:user-agent;\n\tbh=Eg6foNAEj3/D1uiuXx4xeYxiVcCdrfp5fii2Fy1I/MM=;\n\tb=qI/6w322AINkusRntivUZKWaibkgcrwa7M8p0CuPrIMTjA2POGMCO2W7PPUYQQlfVT\n\tqkJo+Lv15XM22xIVjroOt7yK3JPkZ/4iCbPtGFa79zWFi8kVGexCp3aql2C2DAIN2lFp\n\t/RpevWUMZ60g9UkQu7bibZJXWcAOSacnnERKLAmCW+Xqeur1j7/ob0K7bD5/RWiQpTAl\n\tvR7PFItGuvLOrx6Wq+FkjtTr6EIqVb2BrCom7K2FKRAEf5D0Zi4dAbozMmhQYCukc/XS\n\t0yFyoFdpYYj8Nju8thGiyVaaKka+dOx0qlYYIQCWJa1vciHEaGzIgvZfn82GPElkxgOj\n\tGUVw==","X-Gm-Message-State":"APjAAAUAOHqw+DWxRNzt3zWMDvXctph2p58yVlEZseTfPsvumNs09bvr\n\tHaV13FqDXlGeBGYFjHPQOC4=","X-Google-Smtp-Source":"APXvYqz/izbNMQsx8Ua8CplUTY/ERqHqz/6CssQCtvTdOZJLpFeX8ZyvBcaVOXvVprVha17+ERvI0A==","X-Received":"by 2002:a17:90a:3270:: with SMTP id\n\tk103mr22118702pjb.54.1563101102290; \n\tSun, 14 Jul 2019 03:45:02 -0700 (PDT)","Date":"Sun, 14 Jul 2019 19:44:57 +0900","From":"Niklas =?iso-8859-1?q?S=F6derlund?= <niklas.soderlund@ragnatech.se>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190714104457.GE31102@wyvern>","References":"<20190713172351.25452-1-laurent.pinchart@ideasonboard.com>\n\t<20190713172351.25452-8-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190713172351.25452-8-laurent.pinchart@ideasonboard.com>","User-Agent":"Mutt/1.12.1 (2019-06-15)","Subject":"Re: [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":"Sun, 14 Jul 2019 10:45:04 -0000"}}]