[{"id":19966,"web_url":"https://patchwork.libcamera.org/comment/19966/","msgid":"","date":"2021-09-29T07:29:17","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang, thank you for the patch.\n\nOn Wed, Sep 29, 2021 at 4:17 PM Umang Jain wrote:\n>\n> There is a possibility that an out-of-order completion of capture\n> request happens by calling process_capture_result() directly on error\n> paths. The framework expects that errors should be notified as soon as\n> possible, but the request completion order should remain intact.\n> An existing instance of this is abortRequest(), which sends the capture\n> results on flushing state, without considering order-of-completion.\n>\n> Since we have a queue of Camera3RequestDescriptor tracking each\n> capture request placed by framework to libcamera HAL, we should be only\n> sending back capture results from a single location, by inspecting\n> the queue. As per the patch, this now happens in\n> CameraDevice::sendCaptureResults().\n>\n> Each descriptor is now equipped with its own status to denote whether\n> the capture request is complete and ready to send back to the framework\n> or needs to be waited upon. This ensures that the order of completion is\n> respected for the requests.\n>\n> Since we are fixing out-of-order request completion in abortRequest(),\n> change the function to read from the Camera3RequestDescriptor directly,\n> instead of camera3_capture_request_t. The descriptor should have all the\n> information necessary to set the request buffers' state to error.\n>\n> Signed-off-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n> src/android/camera_device.h | 14 +++++++++-\n> 2 files changed, 48 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 499baec8..fab5a854 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> return 0;\n> }\n>\n> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n> {\n> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>\n> - camera3_capture_result_t result = {};\n> - result.num_output_buffers = request->num_output_buffers;\n> - result.frame_number = request->frame_number;\n> + camera3_capture_result_t &result = descriptor->captureResult_;\n> + result.num_output_buffers = descriptor->buffers_.size();\n> + result.frame_number = descriptor->frameNumber_;\n> result.partial_result = 0;\n>\n> std::vector resultBuffers(result.num_output_buffers);\n> for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> - buffer = request->output_buffers[i];\n> - buffer.release_fence = request->output_buffers[i].acquire_fence;\n> + buffer = descriptor->buffers_[i];\n> + buffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> buffer.acquire_fence = -1;\n> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> }\n> result.output_buffers = resultBuffers.data();\n>\n> - callbacks_->process_capture_result(callbacks_, &result);\n> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> }\n\nLooks like abortRequest() does no longer touch any member variable. I\nwould at least makes this const or aggressively static const or move\nto anonymous space.\n\n>\n> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> return ret;\n>\n> /*\n> - * If flush is in progress abort the request. If the camera has been\n> - * stopped we have to re-start it to be able to process the request.\n> + * If flush is in progress abort the request and push the descriptor to\n> + * the queue. If the camera has been stopped we have to re-start it to\n> + * be able to process the request.\n> */\n> MutexLocker stateLock(stateMutex_);\n>\n> if (state_ == State::Flushing) {\n> - abortRequest(camera3Request);\n> + abortRequest(descriptor.get());\n> + {\n> + MutexLocker descriptorsLock(descriptorsMutex_);\n> + descriptors_.push(std::move(descriptor));\n> + }\n> + sendCaptureResults();\n> return 0;\n> }\n>\n> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n> * The buffer status is set to OK and later changed to ERROR if\n> * post-processing/compression fails.\n> */\n> - camera3_capture_result_t captureResult = {};\n> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n> captureResult.frame_number = descriptor->frameNumber_;\n> captureResult.num_output_buffers = descriptor->buffers_.size();\n> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> }\n>\n> - callbacks_->process_capture_result(callbacks_, &captureResult);\n> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> + sendCaptureResults();\n>\n> - descriptors_.pop();\n> return;\n> }\n>\n> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n> }\n>\n> captureResult.result = resultMetadata->get();\n> - callbacks_->process_capture_result(callbacks_, &captureResult);\n> + descriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> + sendCaptureResults();\n> +}\n>\n> - MutexLocker descriptorsLock(descriptorsMutex_);\n> - descriptors_.pop();\n> +void CameraDevice::sendCaptureResults()\n> +{\n> + MutexLocker lock(descriptorsMutex_);\n> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> + auto descriptor = std::move(descriptors_.front());\n> + descriptors_.pop();\n> +\n\nCan you leave a TODO about trying to measure the performance coarse\nvs. grained lock?\n> + lock.unlock();\n> + callbacks_->process_capture_result(callbacks_,\n> + &descriptor->captureResult_);\n> + lock.lock();\n> + }\n> }\n>\n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 9ec510d5..69c78f08 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -74,17 +74,28 @@ private:\n> CameraDevice(unsigned int id, std::shared_ptr camera);\n>\n> struct Camera3RequestDescriptor {\n> + enum class Status {\n> + Pending,\n> + Success,\n> + Error,\n> + };\n> +\n> Camera3RequestDescriptor() = default;\n> ~Camera3RequestDescriptor() = default;\n\nside note: Since we don't do this map, I think this default\nconstructor can be removed.\n\nWith these suggestions,\nReviewed-by: Hirokazu Honda \n\n-Hiro\n> Camera3RequestDescriptor(libcamera::Camera *camera,\n> const camera3_capture_request_t *camera3Request);\n> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>\n> + bool isPending() const { return status_ == Status::Pending; }\n> +\n> uint32_t frameNumber_ = 0;\n> std::vector buffers_;\n> std::vector> frameBuffers_;\n> CameraMetadata settings_;\n> std::unique_ptr request_;\n> +\n> + camera3_capture_result_t captureResult_ = {};\n> + Status status_ = Status::Pending;\n> };\n>\n> enum class State {\n> @@ -99,12 +110,13 @@ private:\n> createFrameBuffer(const buffer_handle_t camera3buffer,\n> libcamera::PixelFormat pixelFormat,\n> const libcamera::Size &size);\n> - void abortRequest(camera3_capture_request_t *request);\n> + void abortRequest(Camera3RequestDescriptor *descriptor);\n> bool isValidRequest(camera3_capture_request_t *request) const;\n> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> camera3_error_msg_code code);\n> int processControls(Camera3RequestDescriptor *descriptor);\n> + void sendCaptureResults();\n> std::unique_ptr getResultMetadata(\n> const Camera3RequestDescriptor &descriptor) const;\n>\n> --\n> 2.31.0\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 9A121BDC71\n\tfor ;\n\tWed, 29 Sep 2021 07:29:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6B10D6918A;\n\tWed, 29 Sep 2021 09:29:30 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2BBD26918A\n\tfor ;\n\tWed, 29 Sep 2021 09:29:28 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id ba1so5007019edb.4\n\tfor ;\n\tWed, 29 Sep 2021 00:29:28 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"Vy84dLms\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=9Umm4ZzWTUYiKHP9VhGHspRFrAkpNPO9N3im4o8OHr8=;\n\tb=Vy84dLmsNycbmGhMmIoBsV9uczggLZyM/eKkojolGmjcHUpEU4ffJ6qwuHMH+gK1f/\n\tP2Bvyweg0IK2WHZzKp8HVmQuNngtnhyqUsSpdqrPHWOF7e0bBpue+NbI9sivSPGds4g1\n\t0MCtEN+uuFlAkMQ5KkNjLE9SGnQDnSrBbdKMU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=9Umm4ZzWTUYiKHP9VhGHspRFrAkpNPO9N3im4o8OHr8=;\n\tb=Gqxf8YW4Tg1VLA/Z22URB69/3BOGt2iSgUTYfE2vZh5q35kQRK5alWnV/R9Z/66Kov\n\topOyzM+xD0phumky19yf3iYv7ATeVtbB+XsomXWBtHGAQJ8+vdVT68XOqNKSzeaTpF5B\n\tVaed/PaPODgmpvNqOullnil4fWVefOC7RW5XDxka/krwuvGnbe8tqyW+fsoJvNLCHrET\n\tGTJkOg/9kM/1MaVveTNWd4+ogmnVWQGNhGOy43LOsC1ZSRm1zVjR8R1Rj5igOz2Q/psl\n\t51G0U5GsqoXt45gD3uBTil7r5gziw2GbP46/20RT0lP++A9IHNuc8xd7el0a1eh1Wv94\n\tSayw==","X-Gm-Message-State":"AOAM533HT0XAe57zqclxZpu+T3el8hnRL02U6Go/PGGegKMdZPTapv/h\n\ttimTZpEnoeMYbOi3r12BlNGlknV68pbjxs9V3E/eQbRbsXc=","X-Google-Smtp-Source":"ABdhPJzSPiCgH/dTRQ4HF7kV/kFMKSUvhAD/LOIDqigHSx4uabWT8JlRdzLi10gL7iOMe81fQpyDXo/4ZaJ/wcAnEMU=","X-Received":"by 2002:a17:906:2691:: with SMTP id\n\tt17mr11530839ejc.522.1632900567794; \n\tWed, 29 Sep 2021 00:29:27 -0700 (PDT)","MIME-Version":"1.0","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>","In-Reply-To":"<20210929071708.299946-4-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Wed, 29 Sep 2021 16:29:17 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19973,"web_url":"https://patchwork.libcamera.org/comment/19973/","msgid":"<20210929081706.chm2ao4b5lzb3ma3@uno.localdomain>","date":"2021-09-29T08:17:06","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, Sep 29, 2021 at 12:47:08PM +0530, Umang Jain wrote:\n> There is a possibility that an out-of-order completion of capture\n> request happens by calling process_capture_result() directly on error\n> paths. The framework expects that errors should be notified as soon as\n> possible, but the request completion order should remain intact.\n> An existing instance of this is abortRequest(), which sends the capture\n> results on flushing state, without considering order-of-completion.\n>\n> Since we have a queue of Camera3RequestDescriptor tracking each\n> capture request placed by framework to libcamera HAL, we should be only\n> sending back capture results from a single location, by inspecting\n> the queue. As per the patch, this now happens in\n> CameraDevice::sendCaptureResults().\n>\n> Each descriptor is now equipped with its own status to denote whether\n> the capture request is complete and ready to send back to the framework\n> or needs to be waited upon. This ensures that the order of completion is\n> respected for the requests.\n>\n> Since we are fixing out-of-order request completion in abortRequest(),\n> change the function to read from the Camera3RequestDescriptor directly,\n> instead of camera3_capture_request_t. The descriptor should have all the\n> information necessary to set the request buffers' state to error.\n>\n> Signed-off-by: Umang Jain \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n> src/android/camera_device.h | 14 +++++++++-\n> 2 files changed, 48 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 499baec8..fab5a854 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> \treturn 0;\n> }\n>\n> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n> {\n> -\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> +\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>\n> -\tcamera3_capture_result_t result = {};\n> -\tresult.num_output_buffers = request->num_output_buffers;\n> -\tresult.frame_number = request->frame_number;\n> +\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> +\tresult.num_output_buffers = descriptor->buffers_.size();\n> +\tresult.frame_number = descriptor->frameNumber_;\n> \tresult.partial_result = 0;\n>\n> \tstd::vector resultBuffers(result.num_output_buffers);\n> \tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> -\t\tbuffer = request->output_buffers[i];\n> -\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> +\t\tbuffer = descriptor->buffers_[i];\n> +\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> \t\tbuffer.acquire_fence = -1;\n> \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t}\n> \tresult.output_buffers = resultBuffers.data();\n>\n> -\tcallbacks_->process_capture_result(callbacks_, &result);\n> +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> }\n>\n> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \t\treturn ret;\n>\n> \t/*\n> -\t * If flush is in progress abort the request. If the camera has been\n> -\t * stopped we have to re-start it to be able to process the request.\n> +\t * If flush is in progress abort the request and push the descriptor to\n> +\t * the queue. If the camera has been stopped we have to re-start it to\n\nI would\n\n\t * If flush is in progress set the request status to error and place it\n * on the queue to be later completed. If the camera has been stopped\n * we have to re-start it to...\n\n> +\t * be able to process the request.\n> \t */\n> \tMutexLocker stateLock(stateMutex_);\n>\n> \tif (state_ == State::Flushing) {\n> -\t\tabortRequest(camera3Request);\n> +\t\tabortRequest(descriptor.get());\n> +\t\t{\n> +\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\t\tdescriptors_.push(std::move(descriptor));\n> +\t\t}\n> +\t\tsendCaptureResults();\n\nEmpty lines around sendCaptureResults() maybe to give the code some\nmore room to breath\n\n> \t\treturn 0;\n> \t}\n>\n> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n> \t * The buffer status is set to OK and later changed to ERROR if\n> \t * post-processing/compression fails.\n> \t */\n> -\tcamera3_capture_result_t captureResult = {};\n> +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n> \tcaptureResult.frame_number = descriptor->frameNumber_;\n> \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> \t\t}\n>\n> -\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> +\t\tsendCaptureResults();\n>\n> -\t\tdescriptors_.pop();\n> \t\treturn;\n> \t}\n>\n> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n>\n> \tcaptureResult.result = resultMetadata->get();\n\nThis now becomes a problem. Before this patch we called back the\nframework right here, and we were guaranteed resultMetadata was still\nin scope. During the callback the framework copied the result metadata\nand when process_capture_result() returned captureResult went out\nof scope and got released.\n\nNow that a request sits in the queue and can be completed at a later\ntime, when it is passed to the framework captureResult.result points\nto a memory location that has been invalidated because the here\ndeclared unique_ptr<> has gone out of scope.\n\nI think we should make just store a\n std::unique_ptr resultMetadata;\nin the Camera3RequestDescriptor and set captureResult.result before\ncalling the process_capture_result callback.\n\nThanks\n j\n\n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> +\tsendCaptureResults();\n> +}\n>\n> -\tMutexLocker descriptorsLock(descriptorsMutex_);\n> -\tdescriptors_.pop();\n> +void CameraDevice::sendCaptureResults()\n> +{\n> +\tMutexLocker lock(descriptorsMutex_);\n> +\twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> +\t\tauto descriptor = std::move(descriptors_.front());\n> +\t\tdescriptors_.pop();\n> +\n> +\t\tlock.unlock();\n> +\t\tcallbacks_->process_capture_result(callbacks_,\n> +\t\t\t\t\t\t &descriptor->captureResult_);\n> +\t\tlock.lock();\n> +\t}\n> }\n>\n> std::string CameraDevice::logPrefix() const\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 9ec510d5..69c78f08 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -74,17 +74,28 @@ private:\n> \tCameraDevice(unsigned int id, std::shared_ptr camera);\n>\n> \tstruct Camera3RequestDescriptor {\n> +\t\tenum class Status {\n> +\t\t\tPending,\n> +\t\t\tSuccess,\n> +\t\t\tError,\n> +\t\t};\n> +\n> \t\tCamera3RequestDescriptor() = default;\n> \t\t~Camera3RequestDescriptor() = default;\n> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> \t\t\t\t\t const camera3_capture_request_t *camera3Request);\n> \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>\n> +\t\tbool isPending() const { return status_ == Status::Pending; }\n> +\n> \t\tuint32_t frameNumber_ = 0;\n> \t\tstd::vector buffers_;\n> \t\tstd::vector> frameBuffers_;\n> \t\tCameraMetadata settings_;\n> \t\tstd::unique_ptr request_;\n> +\n> +\t\tcamera3_capture_result_t captureResult_ = {};\n> +\t\tStatus status_ = Status::Pending;\n> \t};\n>\n> \tenum class State {\n> @@ -99,12 +110,13 @@ private:\n> \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n> \t\t\t libcamera::PixelFormat pixelFormat,\n> \t\t\t const libcamera::Size &size);\n> -\tvoid abortRequest(camera3_capture_request_t *request);\n> +\tvoid abortRequest(Camera3RequestDescriptor *descriptor);\n> \tbool isValidRequest(camera3_capture_request_t *request) const;\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> \t\t\t camera3_error_msg_code code);\n> \tint processControls(Camera3RequestDescriptor *descriptor);\n> +\tvoid sendCaptureResults();\n> \tstd::unique_ptr getResultMetadata(\n> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>\n> --\n> 2.31.0\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 7A4ACC3243\n\tfor ;\n\tWed, 29 Sep 2021 08:16:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E56D9691AE;\n\tWed, 29 Sep 2021 10:16:21 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 085DD6919D\n\tfor ;\n\tWed, 29 Sep 2021 10:16:20 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 780E82001B;\n\tWed, 29 Sep 2021 08:16:19 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 10:17:06 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210929081706.chm2ao4b5lzb3ma3@uno.localdomain>","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210929071708.299946-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19974,"web_url":"https://patchwork.libcamera.org/comment/19974/","msgid":"","date":"2021-09-29T08:34:50","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 9/29/21 1:47 PM, Jacopo Mondi wrote:\n> Hi Umang,\n>\n> On Wed, Sep 29, 2021 at 12:47:08PM +0530, Umang Jain wrote:\n>> There is a possibility that an out-of-order completion of capture\n>> request happens by calling process_capture_result() directly on error\n>> paths. The framework expects that errors should be notified as soon as\n>> possible, but the request completion order should remain intact.\n>> An existing instance of this is abortRequest(), which sends the capture\n>> results on flushing state, without considering order-of-completion.\n>>\n>> Since we have a queue of Camera3RequestDescriptor tracking each\n>> capture request placed by framework to libcamera HAL, we should be only\n>> sending back capture results from a single location, by inspecting\n>> the queue. As per the patch, this now happens in\n>> CameraDevice::sendCaptureResults().\n>>\n>> Each descriptor is now equipped with its own status to denote whether\n>> the capture request is complete and ready to send back to the framework\n>> or needs to be waited upon. This ensures that the order of completion is\n>> respected for the requests.\n>>\n>> Since we are fixing out-of-order request completion in abortRequest(),\n>> change the function to read from the Camera3RequestDescriptor directly,\n>> instead of camera3_capture_request_t. The descriptor should have all the\n>> information necessary to set the request buffers' state to error.\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n>> src/android/camera_device.h | 14 +++++++++-\n>> 2 files changed, 48 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 499baec8..fab5a854 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>> \treturn 0;\n>> }\n>>\n>> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n>> {\n>> -\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>> +\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>\n>> -\tcamera3_capture_result_t result = {};\n>> -\tresult.num_output_buffers = request->num_output_buffers;\n>> -\tresult.frame_number = request->frame_number;\n>> +\tcamera3_capture_result_t &result = descriptor->captureResult_;\n>> +\tresult.num_output_buffers = descriptor->buffers_.size();\n>> +\tresult.frame_number = descriptor->frameNumber_;\n>> \tresult.partial_result = 0;\n>>\n>> \tstd::vector resultBuffers(result.num_output_buffers);\n>> \tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n>> -\t\tbuffer = request->output_buffers[i];\n>> -\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n>> +\t\tbuffer = descriptor->buffers_[i];\n>> +\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n>> \t\tbuffer.acquire_fence = -1;\n>> \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> \t}\n>> \tresult.output_buffers = resultBuffers.data();\n>>\n>> -\tcallbacks_->process_capture_result(callbacks_, &result);\n>> +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> }\n>>\n>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n>> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> \t\treturn ret;\n>>\n>> \t/*\n>> -\t * If flush is in progress abort the request. If the camera has been\n>> -\t * stopped we have to re-start it to be able to process the request.\n>> +\t * If flush is in progress abort the request and push the descriptor to\n>> +\t * the queue. If the camera has been stopped we have to re-start it to\n> I would\n>\n> \t * If flush is in progress set the request status to error and place it\n> * on the queue to be later completed. If the camera has been stopped\n> * we have to re-start it to...\n>\n>> +\t * be able to process the request.\n>> \t */\n>> \tMutexLocker stateLock(stateMutex_);\n>>\n>> \tif (state_ == State::Flushing) {\n>> -\t\tabortRequest(camera3Request);\n>> +\t\tabortRequest(descriptor.get());\n>> +\t\t{\n>> +\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> +\t\t\tdescriptors_.push(std::move(descriptor));\n>> +\t\t}\n>> +\t\tsendCaptureResults();\n> Empty lines around sendCaptureResults() maybe to give the code some\n> more room to breath\n>\n>> \t\treturn 0;\n>> \t}\n>>\n>> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n>> \t * The buffer status is set to OK and later changed to ERROR if\n>> \t * post-processing/compression fails.\n>> \t */\n>> -\tcamera3_capture_result_t captureResult = {};\n>> +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n>> \tcaptureResult.frame_number = descriptor->frameNumber_;\n>> \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n>> \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n>> \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> \t\t}\n>>\n>> -\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> +\t\tsendCaptureResults();\n>>\n>> -\t\tdescriptors_.pop();\n>> \t\treturn;\n>> \t}\n>>\n>> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n>> \t}\n>>\n>> \tcaptureResult.result = resultMetadata->get();\n> This now becomes a problem. Before this patch we called back the\n> framework right here, and we were guaranteed resultMetadata was still\n> in scope. During the callback the framework copied the result metadata\n> and when process_capture_result() returned captureResult went out\n> of scope and got released.\n>\n> Now that a request sits in the queue and can be completed at a later\n> time, when it is passed to the framework captureResult.result points\n\n\nI have considered this. Please look at the sendCaptureResults() right \nafter we set the state to ::Success\n\nThe \"at a later time\" scenario, according to me, emerges when something \nis asynchronous, making a descriptor block while other ::Success \ndescriptor queued. Currently everything is synchronous and every \ndescriptor is synchronously processed as soon as we set a state on it. \nSo, the resultMetdata should be valid until sendCaptureResults() returns.\n\nHence, we should be handling this in upcoming post-processing series \nrather than this?  Does it make sense?\n\n> to a memory location that has been invalidated because the here\n> declared unique_ptr<> has gone out of scope.\n>\n> I think we should make just store a\n> std::unique_ptr resultMetadata;\n> in the Camera3RequestDescriptor and set captureResult.result before\n> calling the process_capture_result callback.\n\n\nYep, that's what should be happening as here:\n\nhttps://patchwork.libcamera.org/patch/13869/\n\n>\n> Thanks\n> j\n>\n>> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n>> +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>> +\tsendCaptureResults();\n>> +}\n>>\n>> -\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> -\tdescriptors_.pop();\n>> +void CameraDevice::sendCaptureResults()\n>> +{\n>> +\tMutexLocker lock(descriptorsMutex_);\n>> +\twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>> +\t\tauto descriptor = std::move(descriptors_.front());\n>> +\t\tdescriptors_.pop();\n>> +\n>> +\t\tlock.unlock();\n>> +\t\tcallbacks_->process_capture_result(callbacks_,\n>> +\t\t\t\t\t\t &descriptor->captureResult_);\n>> +\t\tlock.lock();\n>> +\t}\n>> }\n>>\n>> std::string CameraDevice::logPrefix() const\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 9ec510d5..69c78f08 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -74,17 +74,28 @@ private:\n>> \tCameraDevice(unsigned int id, std::shared_ptr camera);\n>>\n>> \tstruct Camera3RequestDescriptor {\n>> +\t\tenum class Status {\n>> +\t\t\tPending,\n>> +\t\t\tSuccess,\n>> +\t\t\tError,\n>> +\t\t};\n>> +\n>> \t\tCamera3RequestDescriptor() = default;\n>> \t\t~Camera3RequestDescriptor() = default;\n>> \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n>> \t\t\t\t\t const camera3_capture_request_t *camera3Request);\n>> \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>>\n>> +\t\tbool isPending() const { return status_ == Status::Pending; }\n>> +\n>> \t\tuint32_t frameNumber_ = 0;\n>> \t\tstd::vector buffers_;\n>> \t\tstd::vector> frameBuffers_;\n>> \t\tCameraMetadata settings_;\n>> \t\tstd::unique_ptr request_;\n>> +\n>> +\t\tcamera3_capture_result_t captureResult_ = {};\n>> +\t\tStatus status_ = Status::Pending;\n>> \t};\n>>\n>> \tenum class State {\n>> @@ -99,12 +110,13 @@ private:\n>> \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n>> \t\t\t libcamera::PixelFormat pixelFormat,\n>> \t\t\t const libcamera::Size &size);\n>> -\tvoid abortRequest(camera3_capture_request_t *request);\n>> +\tvoid abortRequest(Camera3RequestDescriptor *descriptor);\n>> \tbool isValidRequest(camera3_capture_request_t *request) const;\n>> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>> \t\t\t camera3_error_msg_code code);\n>> \tint processControls(Camera3RequestDescriptor *descriptor);\n>> +\tvoid sendCaptureResults();\n>> \tstd::unique_ptr getResultMetadata(\n>> \t\tconst Camera3RequestDescriptor &descriptor) const;\n>>\n>> --\n>> 2.31.0\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 76BB9BDC71\n\tfor ;\n\tWed, 29 Sep 2021 08:34:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D1853691AC;\n\tWed, 29 Sep 2021 10:34:56 +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 D22936919D\n\tfor ;\n\tWed, 29 Sep 2021 10:34:55 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2447DEE;\n\tWed, 29 Sep 2021 10:34:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RzaYRtRM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632904495;\n\tbh=EQm6bp0cQy0JQcDoU9FioyK7KAh+T26f1375Zg6QYuw=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=RzaYRtRM5AAt65PMazpm+P9DFZrATAvbPn0Y/9TRjoXr+RME5BvNjOlMM3bqg+jka\n\tHkVUQR0JoIfdIdKBDCSlgq0w/o58L6DScpKyGOfFledHnHSCGlqCiPYJkDdTCJPtOf\n\tScROUyamg1plpMvCXwPxbHOx2Uat8NQqPveDMBDU=","To":"Jacopo Mondi ","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>\n\t<20210929081706.chm2ao4b5lzb3ma3@uno.localdomain>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 29 Sep 2021 14:04:50 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20210929081706.chm2ao4b5lzb3ma3@uno.localdomain>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19977,"web_url":"https://patchwork.libcamera.org/comment/19977/","msgid":"<20210929090446.la4jpg734fq3fg7r@uno.localdomain>","date":"2021-09-29T09:04:46","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Umang,\n\nOn Wed, Sep 29, 2021 at 02:04:50PM +0530, Umang Jain wrote:\n> Hi Jacopo,\n>\n> On 9/29/21 1:47 PM, Jacopo Mondi wrote:\n> > Hi Umang,\n> >\n> > On Wed, Sep 29, 2021 at 12:47:08PM +0530, Umang Jain wrote:\n> > > There is a possibility that an out-of-order completion of capture\n> > > request happens by calling process_capture_result() directly on error\n> > > paths. The framework expects that errors should be notified as soon as\n> > > possible, but the request completion order should remain intact.\n> > > An existing instance of this is abortRequest(), which sends the capture\n> > > results on flushing state, without considering order-of-completion.\n> > >\n> > > Since we have a queue of Camera3RequestDescriptor tracking each\n> > > capture request placed by framework to libcamera HAL, we should be only\n> > > sending back capture results from a single location, by inspecting\n> > > the queue. As per the patch, this now happens in\n> > > CameraDevice::sendCaptureResults().\n> > >\n> > > Each descriptor is now equipped with its own status to denote whether\n> > > the capture request is complete and ready to send back to the framework\n> > > or needs to be waited upon. This ensures that the order of completion is\n> > > respected for the requests.\n> > >\n> > > Since we are fixing out-of-order request completion in abortRequest(),\n> > > change the function to read from the Camera3RequestDescriptor directly,\n> > > instead of camera3_capture_request_t. The descriptor should have all the\n> > > information necessary to set the request buffers' state to error.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > Reviewed-by: Laurent Pinchart \n> > > ---\n> > > src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n> > > src/android/camera_device.h | 14 +++++++++-\n> > > 2 files changed, 48 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 499baec8..fab5a854 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > \treturn 0;\n> > > }\n> > >\n> > > -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n> > > {\n> > > -\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > > +\tnotifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > -\tcamera3_capture_result_t result = {};\n> > > -\tresult.num_output_buffers = request->num_output_buffers;\n> > > -\tresult.frame_number = request->frame_number;\n> > > +\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> > > +\tresult.num_output_buffers = descriptor->buffers_.size();\n> > > +\tresult.frame_number = descriptor->frameNumber_;\n> > > \tresult.partial_result = 0;\n> > >\n> > > \tstd::vector resultBuffers(result.num_output_buffers);\n> > > \tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > -\t\tbuffer = request->output_buffers[i];\n> > > -\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> > > +\t\tbuffer = descriptor->buffers_[i];\n> > > +\t\tbuffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> > > \t\tbuffer.acquire_fence = -1;\n> > > \t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > \t}\n> > > \tresult.output_buffers = resultBuffers.data();\n> > >\n> > > -\tcallbacks_->process_capture_result(callbacks_, &result);\n> > > +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > >\n> > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > > @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \t\treturn ret;\n> > >\n> > > \t/*\n> > > -\t * If flush is in progress abort the request. If the camera has been\n> > > -\t * stopped we have to re-start it to be able to process the request.\n> > > +\t * If flush is in progress abort the request and push the descriptor to\n> > > +\t * the queue. If the camera has been stopped we have to re-start it to\n> > I would\n> >\n> > \t * If flush is in progress set the request status to error and place it\n> > * on the queue to be later completed. If the camera has been stopped\n> > * we have to re-start it to...\n> >\n> > > +\t * be able to process the request.\n> > > \t */\n> > > \tMutexLocker stateLock(stateMutex_);\n> > >\n> > > \tif (state_ == State::Flushing) {\n> > > -\t\tabortRequest(camera3Request);\n> > > +\t\tabortRequest(descriptor.get());\n> > > +\t\t{\n> > > +\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > > +\t\t\tdescriptors_.push(std::move(descriptor));\n> > > +\t\t}\n> > > +\t\tsendCaptureResults();\n> > Empty lines around sendCaptureResults() maybe to give the code some\n> > more room to breath\n> >\n> > > \t\treturn 0;\n> > > \t}\n> > >\n> > > @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t * The buffer status is set to OK and later changed to ERROR if\n> > > \t * post-processing/compression fails.\n> > > \t */\n> > > -\tcamera3_capture_result_t captureResult = {};\n> > > +\tcamera3_capture_result_t &captureResult = descriptor->captureResult_;\n> > > \tcaptureResult.frame_number = descriptor->frameNumber_;\n> > > \tcaptureResult.num_output_buffers = descriptor->buffers_.size();\n> > > \tfor (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> > > @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > \t\t}\n> > >\n> > > -\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > +\t\tdescriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > +\t\tsendCaptureResults();\n> > >\n> > > -\t\tdescriptors_.pop();\n> > > \t\treturn;\n> > > \t}\n> > >\n> > > @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n> > > \t}\n> > >\n> > > \tcaptureResult.result = resultMetadata->get();\n> > This now becomes a problem. Before this patch we called back the\n> > framework right here, and we were guaranteed resultMetadata was still\n> > in scope. During the callback the framework copied the result metadata\n> > and when process_capture_result() returned captureResult went out\n> > of scope and got released.\n> >\n> > Now that a request sits in the queue and can be completed at a later\n> > time, when it is passed to the framework captureResult.result points\n>\n>\n> I have considered this. Please look at the sendCaptureResults() right after\n> we set the state to ::Success\n>\n> The \"at a later time\" scenario, according to me, emerges when something is\n> asynchronous, making a descriptor block while other ::Success descriptor\n> queued. Currently everything is synchronous and every descriptor is\n> synchronously processed as soon as we set a state on it. So, the\n> resultMetdata should be valid until sendCaptureResults() returns.\n>\n> Hence, we should be handling this in upcoming post-processing series rather\n> than this?  Does it make sense?\n\nIndeed it does, sorry I was thinking already in the async\npost-processing mode :)\n\nAs long as this is under your radar when moving the post-processing to\nbe async, it's all good!\n\nThanks\n j\n>\n> > to a memory location that has been invalidated because the here\n> > declared unique_ptr<> has gone out of scope.\n> >\n> > I think we should make just store a\n> > std::unique_ptr resultMetadata;\n> > in the Camera3RequestDescriptor and set captureResult.result before\n> > calling the process_capture_result callback.\n>\n>\n> Yep, that's what should be happening as here:\n>\n> https://patchwork.libcamera.org/patch/13869/\n>\n> >\n> > Thanks\n> > j\n> >\n> > > -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> > > +\tdescriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> > > +\tsendCaptureResults();\n> > > +}\n> > >\n> > > -\tMutexLocker descriptorsLock(descriptorsMutex_);\n> > > -\tdescriptors_.pop();\n> > > +void CameraDevice::sendCaptureResults()\n> > > +{\n> > > +\tMutexLocker lock(descriptorsMutex_);\n> > > +\twhile (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> > > +\t\tauto descriptor = std::move(descriptors_.front());\n> > > +\t\tdescriptors_.pop();\n> > > +\n> > > +\t\tlock.unlock();\n> > > +\t\tcallbacks_->process_capture_result(callbacks_,\n> > > +\t\t\t\t\t\t &descriptor->captureResult_);\n> > > +\t\tlock.lock();\n> > > +\t}\n> > > }\n> > >\n> > > std::string CameraDevice::logPrefix() const\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 9ec510d5..69c78f08 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -74,17 +74,28 @@ private:\n> > > \tCameraDevice(unsigned int id, std::shared_ptr camera);\n> > >\n> > > \tstruct Camera3RequestDescriptor {\n> > > +\t\tenum class Status {\n> > > +\t\t\tPending,\n> > > +\t\t\tSuccess,\n> > > +\t\t\tError,\n> > > +\t\t};\n> > > +\n> > > \t\tCamera3RequestDescriptor() = default;\n> > > \t\t~Camera3RequestDescriptor() = default;\n> > > \t\tCamera3RequestDescriptor(libcamera::Camera *camera,\n> > > \t\t\t\t\t const camera3_capture_request_t *camera3Request);\n> > > \t\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> > >\n> > > +\t\tbool isPending() const { return status_ == Status::Pending; }\n> > > +\n> > > \t\tuint32_t frameNumber_ = 0;\n> > > \t\tstd::vector buffers_;\n> > > \t\tstd::vector> frameBuffers_;\n> > > \t\tCameraMetadata settings_;\n> > > \t\tstd::unique_ptr request_;\n> > > +\n> > > +\t\tcamera3_capture_result_t captureResult_ = {};\n> > > +\t\tStatus status_ = Status::Pending;\n> > > \t};\n> > >\n> > > \tenum class State {\n> > > @@ -99,12 +110,13 @@ private:\n> > > \tcreateFrameBuffer(const buffer_handle_t camera3buffer,\n> > > \t\t\t libcamera::PixelFormat pixelFormat,\n> > > \t\t\t const libcamera::Size &size);\n> > > -\tvoid abortRequest(camera3_capture_request_t *request);\n> > > +\tvoid abortRequest(Camera3RequestDescriptor *descriptor);\n> > > \tbool isValidRequest(camera3_capture_request_t *request) const;\n> > > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> > > \t\t\t camera3_error_msg_code code);\n> > > \tint processControls(Camera3RequestDescriptor *descriptor);\n> > > +\tvoid sendCaptureResults();\n> > > \tstd::unique_ptr getResultMetadata(\n> > > \t\tconst Camera3RequestDescriptor &descriptor) const;\n> > >\n> > > --\n> > > 2.31.0\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 A7D8FBDC71\n\tfor ;\n\tWed, 29 Sep 2021 09:04:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B3F2691AC;\n\tWed, 29 Sep 2021 11:04:00 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EF8956919D\n\tfor ;\n\tWed, 29 Sep 2021 11:03:58 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 8C9BFC0003;\n\tWed, 29 Sep 2021 09:03:58 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 11:04:46 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210929090446.la4jpg734fq3fg7r@uno.localdomain>","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>\n\t<20210929081706.chm2ao4b5lzb3ma3@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19978,"web_url":"https://patchwork.libcamera.org/comment/19978/","msgid":"","date":"2021-09-29T09:04:50","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro\n\nOn 9/29/21 12:59 PM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Wed, Sep 29, 2021 at 4:17 PM Umang Jain wrote:\n>> There is a possibility that an out-of-order completion of capture\n>> request happens by calling process_capture_result() directly on error\n>> paths. The framework expects that errors should be notified as soon as\n>> possible, but the request completion order should remain intact.\n>> An existing instance of this is abortRequest(), which sends the capture\n>> results on flushing state, without considering order-of-completion.\n>>\n>> Since we have a queue of Camera3RequestDescriptor tracking each\n>> capture request placed by framework to libcamera HAL, we should be only\n>> sending back capture results from a single location, by inspecting\n>> the queue. As per the patch, this now happens in\n>> CameraDevice::sendCaptureResults().\n>>\n>> Each descriptor is now equipped with its own status to denote whether\n>> the capture request is complete and ready to send back to the framework\n>> or needs to be waited upon. This ensures that the order of completion is\n>> respected for the requests.\n>>\n>> Since we are fixing out-of-order request completion in abortRequest(),\n>> change the function to read from the Camera3RequestDescriptor directly,\n>> instead of camera3_capture_request_t. The descriptor should have all the\n>> information necessary to set the request buffers' state to error.\n>>\n>> Signed-off-by: Umang Jain \n>> Reviewed-by: Laurent Pinchart \n>> ---\n>> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n>> src/android/camera_device.h | 14 +++++++++-\n>> 2 files changed, 48 insertions(+), 18 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 499baec8..fab5a854 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>> return 0;\n>> }\n>>\n>> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n>> {\n>> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>\n>> - camera3_capture_result_t result = {};\n>> - result.num_output_buffers = request->num_output_buffers;\n>> - result.frame_number = request->frame_number;\n>> + camera3_capture_result_t &result = descriptor->captureResult_;\n>> + result.num_output_buffers = descriptor->buffers_.size();\n>> + result.frame_number = descriptor->frameNumber_;\n>> result.partial_result = 0;\n>>\n>> std::vector resultBuffers(result.num_output_buffers);\n>> for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n>> - buffer = request->output_buffers[i];\n>> - buffer.release_fence = request->output_buffers[i].acquire_fence;\n>> + buffer = descriptor->buffers_[i];\n>> + buffer.release_fence = descriptor->buffers_[i].acquire_fence;\n>> buffer.acquire_fence = -1;\n>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> }\n>> result.output_buffers = resultBuffers.data();\n>>\n>> - callbacks_->process_capture_result(callbacks_, &result);\n>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> }\n> Looks like abortRequest() does no longer touch any member variable. I\n> would at least makes this const or aggressively static const or move\n> to anonymous space.\n\n\nI don't think it should go to anonymous space.\n\nI would opt for const, with making notifyError() as const as well, \notherwise it will complain. Possible candidate for separate patch?\n\n>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n>> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>> return ret;\n>>\n>> /*\n>> - * If flush is in progress abort the request. If the camera has been\n>> - * stopped we have to re-start it to be able to process the request.\n>> + * If flush is in progress abort the request and push the descriptor to\n>> + * the queue. If the camera has been stopped we have to re-start it to\n>> + * be able to process the request.\n>> */\n>> MutexLocker stateLock(stateMutex_);\n>>\n>> if (state_ == State::Flushing) {\n>> - abortRequest(camera3Request);\n>> + abortRequest(descriptor.get());\n>> + {\n>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>> + descriptors_.push(std::move(descriptor));\n>> + }\n>> + sendCaptureResults();\n>> return 0;\n>> }\n>>\n>> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n>> * The buffer status is set to OK and later changed to ERROR if\n>> * post-processing/compression fails.\n>> */\n>> - camera3_capture_result_t captureResult = {};\n>> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n>> captureResult.frame_number = descriptor->frameNumber_;\n>> captureResult.num_output_buffers = descriptor->buffers_.size();\n>> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>> }\n>>\n>> - callbacks_->process_capture_result(callbacks_, &captureResult);\n>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>> + sendCaptureResults();\n>>\n>> - descriptors_.pop();\n>> return;\n>> }\n>>\n>> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n>> }\n>>\n>> captureResult.result = resultMetadata->get();\n>> - callbacks_->process_capture_result(callbacks_, &captureResult);\n>> + descriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>> + sendCaptureResults();\n>> +}\n>>\n>> - MutexLocker descriptorsLock(descriptorsMutex_);\n>> - descriptors_.pop();\n>> +void CameraDevice::sendCaptureResults()\n>> +{\n>> + MutexLocker lock(descriptorsMutex_);\n>> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>> + auto descriptor = std::move(descriptors_.front());\n>> + descriptors_.pop();\n>> +\n> Can you leave a TODO about trying to measure the performance coarse\n> vs. grained lock?\n\n\nThe process_capture_result() documentation seems to state the\n\n         This is a non-blocking call. The framework will return this \ncall in 5ms.\n\nI'll try to give some thought to performance implications, if it turns \nout to be rabbit hole, I'll discuss with Jacopo and leave a \\todo in \nthat case.\n\n\n>> + lock.unlock();\n>> + callbacks_->process_capture_result(callbacks_,\n>> + &descriptor->captureResult_);\n>> + lock.lock();\n>> + }\n>> }\n>>\n>> std::string CameraDevice::logPrefix() const\n>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>> index 9ec510d5..69c78f08 100644\n>> --- a/src/android/camera_device.h\n>> +++ b/src/android/camera_device.h\n>> @@ -74,17 +74,28 @@ private:\n>> CameraDevice(unsigned int id, std::shared_ptr camera);\n>>\n>> struct Camera3RequestDescriptor {\n>> + enum class Status {\n>> + Pending,\n>> + Success,\n>> + Error,\n>> + };\n>> +\n>> Camera3RequestDescriptor() = default;\n>> ~Camera3RequestDescriptor() = default;\n> side note: Since we don't do this map, I think this default\n> constructor can be removed.\n\n\nPossible candidate for separate patch\n\n>\n> With these suggestions,\n> Reviewed-by: Hirokazu Honda \n>\n> -Hiro\n>> Camera3RequestDescriptor(libcamera::Camera *camera,\n>> const camera3_capture_request_t *camera3Request);\n>> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>>\n>> + bool isPending() const { return status_ == Status::Pending; }\n>> +\n>> uint32_t frameNumber_ = 0;\n>> std::vector buffers_;\n>> std::vector> frameBuffers_;\n>> CameraMetadata settings_;\n>> std::unique_ptr request_;\n>> +\n>> + camera3_capture_result_t captureResult_ = {};\n>> + Status status_ = Status::Pending;\n>> };\n>>\n>> enum class State {\n>> @@ -99,12 +110,13 @@ private:\n>> createFrameBuffer(const buffer_handle_t camera3buffer,\n>> libcamera::PixelFormat pixelFormat,\n>> const libcamera::Size &size);\n>> - void abortRequest(camera3_capture_request_t *request);\n>> + void abortRequest(Camera3RequestDescriptor *descriptor);\n>> bool isValidRequest(camera3_capture_request_t *request) const;\n>> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>> camera3_error_msg_code code);\n>> int processControls(Camera3RequestDescriptor *descriptor);\n>> + void sendCaptureResults();\n>> std::unique_ptr getResultMetadata(\n>> const Camera3RequestDescriptor &descriptor) const;\n>>\n>> --\n>> 2.31.0\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 1B51FBDC71\n\tfor ;\n\tWed, 29 Sep 2021 09:04:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9113691AD;\n\tWed, 29 Sep 2021 11:04:56 +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 3FF186919D\n\tfor ;\n\tWed, 29 Sep 2021 11:04:55 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7C75C3F0;\n\tWed, 29 Sep 2021 11:04:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UB7HjHmt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632906295;\n\tbh=PsOyQP2jYZi6pjGqdedAfNSSmqLfORPG+Iuh5fs33Ss=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=UB7HjHmt+asdfOEwK2O7h9aifLKHXbCJuybctxOeXhY3CuVO/j4hEaIrU8MYCYkvu\n\tDUQ9Wbu8AIJwpOi5y6eCZ8obU45WF6G1QJf+oCjMbU1o0t5e+jqvL4s5uPkuyWF4bp\n\tKCTIcV2BD7Vc8gmxDd/gsEekyMtyYr4/PSqlIM8k=","To":"Hirokazu Honda ","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 29 Sep 2021 14:34:50 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19979,"web_url":"https://patchwork.libcamera.org/comment/19979/","msgid":"","date":"2021-09-29T09:09:49","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Wed, Sep 29, 2021 at 02:34:50PM +0530, Umang Jain wrote:\n> On 9/29/21 12:59 PM, Hirokazu Honda wrote:\n> > On Wed, Sep 29, 2021 at 4:17 PM Umang Jain wrote:\n> >> There is a possibility that an out-of-order completion of capture\n> >> request happens by calling process_capture_result() directly on error\n> >> paths. The framework expects that errors should be notified as soon as\n> >> possible, but the request completion order should remain intact.\n> >> An existing instance of this is abortRequest(), which sends the capture\n> >> results on flushing state, without considering order-of-completion.\n> >>\n> >> Since we have a queue of Camera3RequestDescriptor tracking each\n> >> capture request placed by framework to libcamera HAL, we should be only\n> >> sending back capture results from a single location, by inspecting\n> >> the queue. As per the patch, this now happens in\n> >> CameraDevice::sendCaptureResults().\n> >>\n> >> Each descriptor is now equipped with its own status to denote whether\n> >> the capture request is complete and ready to send back to the framework\n> >> or needs to be waited upon. This ensures that the order of completion is\n> >> respected for the requests.\n> >>\n> >> Since we are fixing out-of-order request completion in abortRequest(),\n> >> change the function to read from the Camera3RequestDescriptor directly,\n> >> instead of camera3_capture_request_t. The descriptor should have all the\n> >> information necessary to set the request buffers' state to error.\n> >>\n> >> Signed-off-by: Umang Jain \n> >> Reviewed-by: Laurent Pinchart \n> >> ---\n> >> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n> >> src/android/camera_device.h | 14 +++++++++-\n> >> 2 files changed, 48 insertions(+), 18 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 499baec8..fab5a854 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> >> return 0;\n> >> }\n> >>\n> >> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> >> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n> >> {\n> >> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> >> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> >>\n> >> - camera3_capture_result_t result = {};\n> >> - result.num_output_buffers = request->num_output_buffers;\n> >> - result.frame_number = request->frame_number;\n> >> + camera3_capture_result_t &result = descriptor->captureResult_;\n> >> + result.num_output_buffers = descriptor->buffers_.size();\n> >> + result.frame_number = descriptor->frameNumber_;\n> >> result.partial_result = 0;\n> >>\n> >> std::vector resultBuffers(result.num_output_buffers);\n> >> for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> >> - buffer = request->output_buffers[i];\n> >> - buffer.release_fence = request->output_buffers[i].acquire_fence;\n> >> + buffer = descriptor->buffers_[i];\n> >> + buffer.release_fence = descriptor->buffers_[i].acquire_fence;\n> >> buffer.acquire_fence = -1;\n> >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >> }\n> >> result.output_buffers = resultBuffers.data();\n> >>\n> >> - callbacks_->process_capture_result(callbacks_, &result);\n> >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >> }\n> >\n> > Looks like abortRequest() does no longer touch any member variable. I\n> > would at least makes this const or aggressively static const or move\n> > to anonymous space.\n> \n> I don't think it should go to anonymous space.\n> \n> I would opt for const, with making notifyError() as const as well, \n> otherwise it will complain. Possible candidate for separate patch?\n\nIf you post a v4 of this, I'd make abortRequest() const already. In a\nfuture series, I'd move abortRequest() to Camera3RequestDescriptor and\nrename it to abort().\n\n> >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> >> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >> return ret;\n> >>\n> >> /*\n> >> - * If flush is in progress abort the request. If the camera has been\n> >> - * stopped we have to re-start it to be able to process the request.\n> >> + * If flush is in progress abort the request and push the descriptor to\n> >> + * the queue. If the camera has been stopped we have to re-start it to\n> >> + * be able to process the request.\n> >> */\n> >> MutexLocker stateLock(stateMutex_);\n> >>\n> >> if (state_ == State::Flushing) {\n> >> - abortRequest(camera3Request);\n> >> + abortRequest(descriptor.get());\n> >> + {\n> >> + MutexLocker descriptorsLock(descriptorsMutex_);\n> >> + descriptors_.push(std::move(descriptor));\n> >> + }\n> >> + sendCaptureResults();\n> >> return 0;\n> >> }\n> >>\n> >> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n> >> * The buffer status is set to OK and later changed to ERROR if\n> >> * post-processing/compression fails.\n> >> */\n> >> - camera3_capture_result_t captureResult = {};\n> >> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n> >> captureResult.frame_number = descriptor->frameNumber_;\n> >> captureResult.num_output_buffers = descriptor->buffers_.size();\n> >> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n> >> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n> >> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> >> }\n> >>\n> >> - callbacks_->process_capture_result(callbacks_, &captureResult);\n> >> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> >> + sendCaptureResults();\n> >>\n> >> - descriptors_.pop();\n> >> return;\n> >> }\n> >>\n> >> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n> >> }\n> >>\n> >> captureResult.result = resultMetadata->get();\n> >> - callbacks_->process_capture_result(callbacks_, &captureResult);\n> >> + descriptor->status_ = Camera3RequestDescriptor::Status::Success;\n> >> + sendCaptureResults();\n> >> +}\n> >>\n> >> - MutexLocker descriptorsLock(descriptorsMutex_);\n> >> - descriptors_.pop();\n> >> +void CameraDevice::sendCaptureResults()\n> >> +{\n> >> + MutexLocker lock(descriptorsMutex_);\n> >> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n> >> + auto descriptor = std::move(descriptors_.front());\n> >> + descriptors_.pop();\n> >> +\n> >\n> > Can you leave a TODO about trying to measure the performance coarse\n> > vs. grained lock?\n> \n> The process_capture_result() documentation seems to state the\n> \n>         This is a non-blocking call. The framework will return this \n> call in 5ms.\n> \n> I'll try to give some thought to performance implications, if it turns \n> out to be rabbit hole, I'll discuss with Jacopo and leave a \\todo in \n> that case.\n> \n> >> + lock.unlock();\n> >> + callbacks_->process_capture_result(callbacks_,\n> >> + &descriptor->captureResult_);\n> >> + lock.lock();\n> >> + }\n> >> }\n> >>\n> >> std::string CameraDevice::logPrefix() const\n> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> >> index 9ec510d5..69c78f08 100644\n> >> --- a/src/android/camera_device.h\n> >> +++ b/src/android/camera_device.h\n> >> @@ -74,17 +74,28 @@ private:\n> >> CameraDevice(unsigned int id, std::shared_ptr camera);\n> >>\n> >> struct Camera3RequestDescriptor {\n> >> + enum class Status {\n> >> + Pending,\n> >> + Success,\n> >> + Error,\n> >> + };\n> >> +\n> >> Camera3RequestDescriptor() = default;\n> >> ~Camera3RequestDescriptor() = default;\n> >\n> > side note: Since we don't do this map, I think this default\n> > constructor can be removed.\n> \n> Possible candidate for separate patch\n> \n> > With these suggestions,\n> > Reviewed-by: Hirokazu Honda \n> >\n> >> Camera3RequestDescriptor(libcamera::Camera *camera,\n> >> const camera3_capture_request_t *camera3Request);\n> >> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> >>\n> >> + bool isPending() const { return status_ == Status::Pending; }\n> >> +\n> >> uint32_t frameNumber_ = 0;\n> >> std::vector buffers_;\n> >> std::vector> frameBuffers_;\n> >> CameraMetadata settings_;\n> >> std::unique_ptr request_;\n> >> +\n> >> + camera3_capture_result_t captureResult_ = {};\n> >> + Status status_ = Status::Pending;\n> >> };\n> >>\n> >> enum class State {\n> >> @@ -99,12 +110,13 @@ private:\n> >> createFrameBuffer(const buffer_handle_t camera3buffer,\n> >> libcamera::PixelFormat pixelFormat,\n> >> const libcamera::Size &size);\n> >> - void abortRequest(camera3_capture_request_t *request);\n> >> + void abortRequest(Camera3RequestDescriptor *descriptor);\n> >> bool isValidRequest(camera3_capture_request_t *request) const;\n> >> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> >> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n> >> camera3_error_msg_code code);\n> >> int processControls(Camera3RequestDescriptor *descriptor);\n> >> + void sendCaptureResults();\n> >> std::unique_ptr getResultMetadata(\n> >> const Camera3RequestDescriptor &descriptor) const;\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 47FF7C3243\n\tfor ;\n\tWed, 29 Sep 2021 09:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B6910691AE;\n\tWed, 29 Sep 2021 11:09:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9F6106919D\n\tfor ;\n\tWed, 29 Sep 2021 11:09:51 +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 1685A3F0;\n\tWed, 29 Sep 2021 11:09:51 +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=\"T5e6BApx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632906591;\n\tbh=pjW6I3EoJX8TXX2mLQnbFHPKBJIxpCzGeO5Ul2p/OUc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T5e6BApxo7E1GYQ3/+i52R+mK+Kkha+s/LbcLITSreluD24lbVNL0yOsjAKl2k7jn\n\tFaahg1FdkYtJhsgmthq3GcdJ1EAefEAArIS9NH3XKegfh80DmhBlPXerpYPxS9Bcog\n\tdgI/DF6GQcEsZ9gm4TLV23RbLdnnL8qLbrlfm+00=","Date":"Wed, 29 Sep 2021 12:09:49 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19980,"web_url":"https://patchwork.libcamera.org/comment/19980/","msgid":"<17966acf-7241-8806-5b68-4d667d6d34ed@ideasonboard.com>","date":"2021-09-29T09:13:52","subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 9/29/21 2:39 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Wed, Sep 29, 2021 at 02:34:50PM +0530, Umang Jain wrote:\n>> On 9/29/21 12:59 PM, Hirokazu Honda wrote:\n>>> On Wed, Sep 29, 2021 at 4:17 PM Umang Jain wrote:\n>>>> There is a possibility that an out-of-order completion of capture\n>>>> request happens by calling process_capture_result() directly on error\n>>>> paths. The framework expects that errors should be notified as soon as\n>>>> possible, but the request completion order should remain intact.\n>>>> An existing instance of this is abortRequest(), which sends the capture\n>>>> results on flushing state, without considering order-of-completion.\n>>>>\n>>>> Since we have a queue of Camera3RequestDescriptor tracking each\n>>>> capture request placed by framework to libcamera HAL, we should be only\n>>>> sending back capture results from a single location, by inspecting\n>>>> the queue. As per the patch, this now happens in\n>>>> CameraDevice::sendCaptureResults().\n>>>>\n>>>> Each descriptor is now equipped with its own status to denote whether\n>>>> the capture request is complete and ready to send back to the framework\n>>>> or needs to be waited upon. This ensures that the order of completion is\n>>>> respected for the requests.\n>>>>\n>>>> Since we are fixing out-of-order request completion in abortRequest(),\n>>>> change the function to read from the Camera3RequestDescriptor directly,\n>>>> instead of camera3_capture_request_t. The descriptor should have all the\n>>>> information necessary to set the request buffers' state to error.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> Reviewed-by: Laurent Pinchart \n>>>> ---\n>>>> src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------\n>>>> src/android/camera_device.h | 14 +++++++++-\n>>>> 2 files changed, 48 insertions(+), 18 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index 499baec8..fab5a854 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n>>>> return 0;\n>>>> }\n>>>>\n>>>> -void CameraDevice::abortRequest(camera3_capture_request_t *request)\n>>>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)\n>>>> {\n>>>> - notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>>> + notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>>>\n>>>> - camera3_capture_result_t result = {};\n>>>> - result.num_output_buffers = request->num_output_buffers;\n>>>> - result.frame_number = request->frame_number;\n>>>> + camera3_capture_result_t &result = descriptor->captureResult_;\n>>>> + result.num_output_buffers = descriptor->buffers_.size();\n>>>> + result.frame_number = descriptor->frameNumber_;\n>>>> result.partial_result = 0;\n>>>>\n>>>> std::vector resultBuffers(result.num_output_buffers);\n>>>> for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n>>>> - buffer = request->output_buffers[i];\n>>>> - buffer.release_fence = request->output_buffers[i].acquire_fence;\n>>>> + buffer = descriptor->buffers_[i];\n>>>> + buffer.release_fence = descriptor->buffers_[i].acquire_fence;\n>>>> buffer.acquire_fence = -1;\n>>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> }\n>>>> result.output_buffers = resultBuffers.data();\n>>>>\n>>>> - callbacks_->process_capture_result(callbacks_, &result);\n>>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>>>> }\n>>> Looks like abortRequest() does no longer touch any member variable. I\n>>> would at least makes this const or aggressively static const or move\n>>> to anonymous space.\n>> I don't think it should go to anonymous space.\n>>\n>> I would opt for const, with making notifyError() as const as well,\n>> otherwise it will complain. Possible candidate for separate patch?\n> If you post a v4 of this, I'd make abortRequest() const already. In a\n> future series, I'd move abortRequest() to Camera3RequestDescriptor and\n> rename it to abort().\n\n\nI was already thinking this and noted down for future series (separate \nofcourse)\n\nThough we will need to move notifyError() to Camera3RequestDescriptor as \nwell, which is currently inside CameraDevice & possibly  devise a way to \naccess callbacks_ !\n\nSo, will need some thinking there, which I don't want to get into right now\n\nFor v4, lets move to const already, makes sense.\n\n>\n>>>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n>>>> @@ -1051,13 +1051,19 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>>> return ret;\n>>>>\n>>>> /*\n>>>> - * If flush is in progress abort the request. If the camera has been\n>>>> - * stopped we have to re-start it to be able to process the request.\n>>>> + * If flush is in progress abort the request and push the descriptor to\n>>>> + * the queue. If the camera has been stopped we have to re-start it to\n>>>> + * be able to process the request.\n>>>> */\n>>>> MutexLocker stateLock(stateMutex_);\n>>>>\n>>>> if (state_ == State::Flushing) {\n>>>> - abortRequest(camera3Request);\n>>>> + abortRequest(descriptor.get());\n>>>> + {\n>>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> + descriptors_.push(std::move(descriptor));\n>>>> + }\n>>>> + sendCaptureResults();\n>>>> return 0;\n>>>> }\n>>>>\n>>>> @@ -1115,7 +1121,7 @@ void CameraDevice::requestComplete(Request *request)\n>>>> * The buffer status is set to OK and later changed to ERROR if\n>>>> * post-processing/compression fails.\n>>>> */\n>>>> - camera3_capture_result_t captureResult = {};\n>>>> + camera3_capture_result_t &captureResult = descriptor->captureResult_;\n>>>> captureResult.frame_number = descriptor->frameNumber_;\n>>>> captureResult.num_output_buffers = descriptor->buffers_.size();\n>>>> for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {\n>>>> @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)\n>>>> buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n>>>> }\n>>>>\n>>>> - callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>>>> + sendCaptureResults();\n>>>>\n>>>> - descriptors_.pop();\n>>>> return;\n>>>> }\n>>>>\n>>>> @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)\n>>>> }\n>>>>\n>>>> captureResult.result = resultMetadata->get();\n>>>> - callbacks_->process_capture_result(callbacks_, &captureResult);\n>>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Success;\n>>>> + sendCaptureResults();\n>>>> +}\n>>>>\n>>>> - MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> - descriptors_.pop();\n>>>> +void CameraDevice::sendCaptureResults()\n>>>> +{\n>>>> + MutexLocker lock(descriptorsMutex_);\n>>>> + while (!descriptors_.empty() && !descriptors_.front()->isPending()) {\n>>>> + auto descriptor = std::move(descriptors_.front());\n>>>> + descriptors_.pop();\n>>>> +\n>>> Can you leave a TODO about trying to measure the performance coarse\n>>> vs. grained lock?\n>> The process_capture_result() documentation seems to state the\n>>\n>>         This is a non-blocking call. The framework will return this\n>> call in 5ms.\n>>\n>> I'll try to give some thought to performance implications, if it turns\n>> out to be rabbit hole, I'll discuss with Jacopo and leave a \\todo in\n>> that case.\n>>\n>>>> + lock.unlock();\n>>>> + callbacks_->process_capture_result(callbacks_,\n>>>> + &descriptor->captureResult_);\n>>>> + lock.lock();\n>>>> + }\n>>>> }\n>>>>\n>>>> std::string CameraDevice::logPrefix() const\n>>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>>> index 9ec510d5..69c78f08 100644\n>>>> --- a/src/android/camera_device.h\n>>>> +++ b/src/android/camera_device.h\n>>>> @@ -74,17 +74,28 @@ private:\n>>>> CameraDevice(unsigned int id, std::shared_ptr camera);\n>>>>\n>>>> struct Camera3RequestDescriptor {\n>>>> + enum class Status {\n>>>> + Pending,\n>>>> + Success,\n>>>> + Error,\n>>>> + };\n>>>> +\n>>>> Camera3RequestDescriptor() = default;\n>>>> ~Camera3RequestDescriptor() = default;\n>>> side note: Since we don't do this map, I think this default\n>>> constructor can be removed.\n>> Possible candidate for separate patch\n>>\n>>> With these suggestions,\n>>> Reviewed-by: Hirokazu Honda \n>>>\n>>>> Camera3RequestDescriptor(libcamera::Camera *camera,\n>>>> const camera3_capture_request_t *camera3Request);\n>>>> Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n>>>>\n>>>> + bool isPending() const { return status_ == Status::Pending; }\n>>>> +\n>>>> uint32_t frameNumber_ = 0;\n>>>> std::vector buffers_;\n>>>> std::vector> frameBuffers_;\n>>>> CameraMetadata settings_;\n>>>> std::unique_ptr request_;\n>>>> +\n>>>> + camera3_capture_result_t captureResult_ = {};\n>>>> + Status status_ = Status::Pending;\n>>>> };\n>>>>\n>>>> enum class State {\n>>>> @@ -99,12 +110,13 @@ private:\n>>>> createFrameBuffer(const buffer_handle_t camera3buffer,\n>>>> libcamera::PixelFormat pixelFormat,\n>>>> const libcamera::Size &size);\n>>>> - void abortRequest(camera3_capture_request_t *request);\n>>>> + void abortRequest(Camera3RequestDescriptor *descriptor);\n>>>> bool isValidRequest(camera3_capture_request_t *request) const;\n>>>> void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>>>> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,\n>>>> camera3_error_msg_code code);\n>>>> int processControls(Camera3RequestDescriptor *descriptor);\n>>>> + void sendCaptureResults();\n>>>> std::unique_ptr getResultMetadata(\n>>>> const Camera3RequestDescriptor &descriptor) const;\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 A378DBDC71\n\tfor ;\n\tWed, 29 Sep 2021 09:13:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E9D5C691AC;\n\tWed, 29 Sep 2021 11:13:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 318926919D\n\tfor ;\n\tWed, 29 Sep 2021 11:13:57 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 123B03F0;\n\tWed, 29 Sep 2021 11:13: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=\"ma3rbKBl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632906836;\n\tbh=3Z9IOUcQOwCDBAlc6cCY5VGjpMeQVOMvEnlmtKsjWbo=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=ma3rbKBlSD5jJ6hhhqXTBi22OKGQ0SKPyh5B/XStQuo9/MHCPk7KHpRhLrZA+Drii\n\tSDyEZp9tvdmfL+ib0fyb4wXOfkJOOM2DRXi54dztiInLZ0RTJ/G/1TpjazXCDZqU/F\n\tvznwUWyi4QL15lHOnpDCMT8EsnN/gvscb8UofIJs=","To":"Laurent Pinchart ","References":"<20210929071708.299946-1-umang.jain@ideasonboard.com>\n\t<20210929071708.299946-4-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<17966acf-7241-8806-5b68-4d667d6d34ed@ideasonboard.com>","Date":"Wed, 29 Sep 2021 14:43:52 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"8bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v3 3/3] android: camera_device: Send\n\tcapture results by inspecting the queue","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]