[{"id":15990,"web_url":"https://patchwork.libcamera.org/comment/15990/","msgid":"","date":"2021-03-29T05:12:15","subject":"Re: [libcamera-devel] [PATCH 3/3] Regard a request error in the\n\trequest completion","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Mon, Mar 29, 2021 at 09:27:15AM +0900, Hirokazu Honda wrote:\n> libcamera::Request contains an error value. Every\n> libcamera::Camera client should regards the error in a\n> request completion.\n> \n> Signed-off-by: Hirokazu Honda \n> ---\n> Documentation/guides/application-developer.rst | 4 ++--\n> src/android/camera_device.cpp | 6 ++++--\n> src/cam/capture.cpp | 4 +++-\n> src/gstreamer/gstlibcamerasrc.cpp | 4 ++++\n> src/qcam/main_window.cpp | 4 +++-\n> src/v4l2/v4l2_camera.cpp | 4 +++-\n> test/camera/buffer_import.cpp | 4 +++-\n> test/camera/capture.cpp | 4 +++-\n> 8 files changed, 25 insertions(+), 9 deletions(-)\n> \n> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst\n> index e3430f03..d7232e7c 100644\n> --- a/Documentation/guides/application-developer.rst\n> +++ b/Documentation/guides/application-developer.rst\n> @@ -384,13 +384,13 @@ Request completion events can be emitted for requests which have been canceled,\n> for example, by unexpected application shutdown. To avoid an application\n> processing invalid image data, it’s worth checking that the request has\n> completed successfully. The list of request completion statuses is available in\n> -the `Request::Status`_ class enum documentation.\n> +the `Request::Status`_ class enum documentation and `Request::result`_.\n> \n> .. _Request::Status: https://www.libcamera.org/api-html/classlibcamera_1_1Request.html#a2209ba8d51af8167b25f6e3e94d5c45b\n> \n> .. code:: cpp\n> \n> - if (request->status() == Request::RequestCancelled)\n> + if (request->status() == Request::RequestCancelled || request->result() != 0)\n> return;\n> \n> If the ``Request`` has completed successfully, applications can access the\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index ae693664..d02c1776 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1908,9 +1908,11 @@ void CameraDevice::requestComplete(Request *request)\n> \tCamera3RequestDescriptor *descriptor =\n> \t\treinterpret_cast(request->cookie());\n> \n> -\tif (request->status() != Request::RequestComplete) {\n> +\tif (request->status() != Request::RequestComplete ||\n> +\t request->result() != 0) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> -\t\t\t\t<< request->status();\n> +\t\t\t\t<< \"status=\" << request->status()\n> +\t\t\t\t<< \", result=\" << request->result();\n> \t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n\nDo we also need to return the error through the notify() callback ?\n\n> \t}\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 7b55fc67..24d0be32 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -165,8 +165,10 @@ int Capture::queueRequest(Request *request)\n> \n> void Capture::requestComplete(Request *request)\n> {\n> -\tif (request->status() == Request::RequestCancelled)\n> +\tif (request->status() == Request::RequestCancelled ||\n> +\t request->result() != 0) {\n\nI'm not sure this is right. Cancelled requests are ignored as they\nindicate we're stopping the camera, but for requests that complete with\nan error, shouldn't they be requeued ? It depends whether the error is\nfatal or not, and we don't have that information yet. All errors that we\ncurrently report would be caused by the pipeline handler failing to\naccept the request, which is fatal, but that's a bug in the pipeline\nhandler :-) I'm thus not sure what the best option is in application\ncode.\n\n> \t\treturn;\n> +\t}\n\nNo need for braces.\n\n> \n> \t/*\n> \t * Defer processing of the completed request to the event loop, to avoid\n> diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp\n> index 87246b40..b36ac33c 100644\n> --- a/src/gstreamer/gstlibcamerasrc.cpp\n> +++ b/src/gstreamer/gstlibcamerasrc.cpp\n> @@ -167,6 +167,10 @@ GstLibcameraSrcState::requestCompleted(Request *request)\n> \t\tGST_DEBUG_OBJECT(src_, \"Request was cancelled\");\n> \t\treturn;\n> \t}\n> +\tif (request->result() != 0) {\n> +\t\tGST_DEBUG_OBJECT(src_, \"Request was not completed successfully\");\n> +\t\treturn;\n> +\t}\n> \n> \tGstBuffer *buffer;\n> \tfor (GstPad *srcpad : srcpads_) {\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 39d034de..f90310f9 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -692,8 +692,10 @@ void MainWindow::processRaw(FrameBuffer *buffer,\n> \n> void MainWindow::requestComplete(Request *request)\n> {\n> -\tif (request->status() == Request::RequestCancelled)\n> +\tif (request->status() == Request::RequestCancelled ||\n> +\t request->result() != 0) {\n> \t\treturn;\n> +\t}\n> \n> \t/*\n> \t * We're running in the libcamera thread context, expensive operations\n> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp\n> index 97825c71..87153cd4 100644\n> --- a/src/v4l2/v4l2_camera.cpp\n> +++ b/src/v4l2/v4l2_camera.cpp\n> @@ -82,8 +82,10 @@ std::vector V4L2Camera::completedBuffers()\n> \n> void V4L2Camera::requestComplete(Request *request)\n> {\n> -\tif (request->status() == Request::RequestCancelled)\n> +\tif (request->status() == Request::RequestCancelled ||\n> +\t request->result() != 0) {\n> \t\treturn;\n\nHere I'm pretty sure that ignoring requests with an error isn't right.\nThe error needs to instead be reported to upper layers, all the way to\nthe application. The same may apply to the gstreamer element above.\n\n> +\t}\n> \n> \t/* We only have one stream at the moment. */\n> \tbufferLock_.lock();\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index 61f4eb92..8f4efa5e 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -47,8 +47,10 @@ protected:\n> \n> \tvoid requestComplete(Request *request)\n> \t{\n> -\t\tif (request->status() != Request::RequestComplete)\n> +\t\tif (request->status() != Request::RequestComplete ||\n> +\t\t request->result() != 0) {\n> \t\t\treturn;\n> +\t\t}\n> \n> \t\tconst Request::BufferMap &buffers = request->buffers();\n> \n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index c4bc2110..6cff9bb4 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -43,8 +43,10 @@ protected:\n> \n> \tvoid requestComplete(Request *request)\n> \t{\n> -\t\tif (request->status() != Request::RequestComplete)\n> +\t\tif (request->status() != Request::RequestComplete ||\n> +\t\t request->result() != 0) {\n> \t\t\treturn;\n> +\t\t}\n> \n> \t\tconst Request::BufferMap &buffers = request->buffers();\n>","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 7CFA3C32EF\n\tfor ;\n\tMon, 29 Mar 2021 05:13:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D48496877D;\n\tMon, 29 Mar 2021 07:13:01 +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 03951602D2\n\tfor ;\n\tMon, 29 Mar 2021 07:12:59 +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 60C2131A;\n\tMon, 29 Mar 2021 07:12:59 +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=\"ldBF5xnZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616994779;\n\tbh=Io3kwgTKN2y2HJshFcff8HZcrA+T/oR/mf83S6vRHCM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ldBF5xnZZMkX3FVySF23JE7uMtQn9jiMR2dYiR81wIHENqWLOK8oh9HHU5an1llwX\n\tCgAKh1ENs1svaOk9PiBDTAhO4pFC3oW/VGuuYhnwizFnukTjlwM2bdH5wMMR1szdoY\n\t943GLvLM+n/liJHPlcBBqN48mzgOE2CxzOuF95MU=","Date":"Mon, 29 Mar 2021 08:12:15 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210329002715.74403-1-hiroh@chromium.org>\n\t<20210329002715.74403-4-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210329002715.74403-4-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 3/3] Regard a request error in the\n\trequest completion","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\" "}}]