[{"id":19949,"web_url":"https://patchwork.libcamera.org/comment/19949/","msgid":"","date":"2021-09-28T21:30:08","subject":"Re: [libcamera-devel] [PATCH v2 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\nThank you for the patch.\n\nOn Tue, Sep 28, 2021 at 09:55:36PM +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> Signed-off-by: Umang Jain \n> ---\n> src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------\n> src/android/camera_device.h | 15 +++++++++++-\n> 2 files changed, 46 insertions(+), 13 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a3b8a549..83030366 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -861,11 +861,12 @@ 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> +\t\t\t\tcamera3_capture_request_t *request)\n\nI don't think I've seen a reply to my review comment in v1:\n\nCould this function take a Camera3RequestDescriptor pointer only ? It\nshould contain all the needed data. This can be done as a patch before\nthis one if desired, or here as it shouldn't be much extra work.\n\nThis function uses the num_output_buffers, frame_number and\noutput_buffers members of camera3_capture_request_t. Those are copied to\nthe Camera3RequestDescriptor buffers_ and frameNumber_ members which you\ncan use here.\n\nReviewed-by: Laurent Pinchart \n\n> {\n> \tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> \n> -\tcamera3_capture_result_t result = {};\n> +\tcamera3_capture_result_t &result = descriptor->captureResult_;\n> \tresult.num_output_buffers = request->num_output_buffers;\n> \tresult.frame_number = request->frame_number;\n> \tresult.partial_result = 0;\n> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)\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> @@ -1052,13 +1053,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> +\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(descriptors_.back().get(), camera3Request);\n> +\t\t{\n> +\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> +\t\t\tdescriptors_.push(std::move(descriptor));\n> +\t\t}\n> +\t\tsendCaptureResults();\n> \t\treturn 0;\n> \t}\n> \n> @@ -1116,7 +1123,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> @@ -1166,9 +1173,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> @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \tcaptureResult.result = resultMetadata->get();\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\tstd::unique_ptr descriptor =\n> +\t\t\tstd::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..dbfa7431 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,14 @@ 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> +\t\t\t camera3_capture_request_t *request);\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>","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 04614C3243\n\tfor ;\n\tTue, 28 Sep 2021 21:30:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26F7E69191;\n\tTue, 28 Sep 2021 23:30:12 +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 4CDB569188\n\tfor ;\n\tTue, 28 Sep 2021 23:30:10 +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 BF55F3F1;\n\tTue, 28 Sep 2021 23:30:09 +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=\"nfYdAaXc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632864609;\n\tbh=q3a88VirCMD8P/cIBri+34dLRS3fOJEMeVeUgmcmRMs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nfYdAaXcDnYqikbYn0JmNsblsxLo1mLtPHGdyQmYPGCps5AD1Bz25Di7nBEZUcE3d\n\tSK/SQWAMrz1qWEUA++OlIzFgGnPObmlwZ1TOx9rcbFfrSCX56pxNjDp+gB/IyPSzBs\n\tLVwziCQSRAZIZfCWvtGxycvy5+WPNwfC7pXYyiKo=","Date":"Wed, 29 Sep 2021 00:30:08 +0300","From":"Laurent Pinchart ","To":"Umang Jain ","Message-ID":"","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-4-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928162536.227475-4-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 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":19954,"web_url":"https://patchwork.libcamera.org/comment/19954/","msgid":"","date":"2021-09-28T23:45:24","subject":"Re: [libcamera-devel] [PATCH v2 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,\n\nOn Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart\n wrote:\n>\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Sep 28, 2021 at 09:55:36PM +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> > Signed-off-by: Umang Jain \n> > ---\n> > src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------\n> > src/android/camera_device.h | 15 +++++++++++-\n> > 2 files changed, 46 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index a3b8a549..83030366 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -861,11 +861,12 @@ 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> > + camera3_capture_request_t *request)\n>\n> I don't think I've seen a reply to my review comment in v1:\n>\n> Could this function take a Camera3RequestDescriptor pointer only ? It\n> should contain all the needed data. This can be done as a patch before\n> this one if desired, or here as it shouldn't be much extra work.\n>\n> This function uses the num_output_buffers, frame_number and\n> output_buffers members of camera3_capture_request_t. Those are copied to\n> the Camera3RequestDescriptor buffers_ and frameNumber_ members which you\n> can use here.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > {\n> > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> >\n> > - camera3_capture_result_t result = {};\n> > + camera3_capture_result_t &result = descriptor->captureResult_;\n> > result.num_output_buffers = request->num_output_buffers;\n> > result.frame_number = request->frame_number;\n> > result.partial_result = 0;\n> > @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > }\n> > result.output_buffers = resultBuffers.data();\n> >\n> > - callbacks_->process_capture_result(callbacks_, &result);\n> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > }\n> >\n> > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > @@ -1052,13 +1053,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(descriptors_.back().get(), camera3Request);\n> > + {\n> > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > + descriptors_.push(std::move(descriptor));\n> > + }\n\nCould you tell me what happens here?\nI think the current request, camear3Request needs to be cancelled.\nBut abortRequest(descriptors_.back().get(), camera3Request) is called\nbefore pushing.\nSo it aborts the last pushed request with the current request.\nI wonder if the correct code is below.\nif (state_ == State::Flushing)\n{\n {\n MutexLocker descriptorsLock(descriptorsMutex_);\n descriptors_.push(std::move(descriptor));\n abortRequest(descriptors_.back().get(), camera3Request);\n }\n sendCaptureResults();\n return 0;\n}\n\n> > + sendCaptureResults();\n> > return 0;\n> > }\n> >\n> > @@ -1116,7 +1123,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> > @@ -1166,9 +1173,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> > @@ -1234,10 +1241,23 @@ 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> > + std::unique_ptr descriptor =\n> > + std::move(descriptors_.front());\n\nnit: I would use auto here.\n\n> > + descriptors_.pop();\n> > +\n> > + lock.unlock();\n> > + callbacks_->process_capture_result(callbacks_,\n> > + &descriptor->captureResult_);\n> > + lock.lock();\n\nWhy is lock released during calling the callback? Is there any deadlock here?\nI wonder if it might be less efficient to release and re-acquire lock\nevery call than just holding a lock entirely.\n\n-Hiro\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..dbfa7431 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> > 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,14 @@ 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> > + camera3_capture_request_t *request);\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> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 78DC6BDC71\n\tfor ;\n\tTue, 28 Sep 2021 23:45:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C496469191;\n\tWed, 29 Sep 2021 01:45:36 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D6A786012C\n\tfor ;\n\tWed, 29 Sep 2021 01:45:35 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id x7so1511793edd.6\n\tfor ;\n\tTue, 28 Sep 2021 16:45:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"c1d7cZ0E\"; 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=CswJ52g7JlzJ9RI3tqFF3Qvyd9ZDMElMgqM2k88hzBc=;\n\tb=c1d7cZ0Ej+BqIEw2Df4rUalLVPTtZjcCD6rObVuvgo8Q9dqkZb+YHGuy1y6uYaCMfn\n\tw1wd2R0IN58uT8/zErdHpNtZ5/4V9q881tIRbmwgpkIutQJTqA/0yGMprWOQ4HMPOb00\n\tgDXF8nyvb/AHp7AkSFI+fCQR426snNvO9NPek=","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=CswJ52g7JlzJ9RI3tqFF3Qvyd9ZDMElMgqM2k88hzBc=;\n\tb=kfSyM7tDth6EvBC1JcLMWt8YSTP5pMDfff26/E96fB9sMYd0XSAWMDZVf49vihJgrw\n\tiFS6fVGmaGU898h1w+Z5DevV9ZuBXiLyoJqz91eOQ6pZ3r+KwouHXx2YD9abGI12tNsm\n\tTfFaKEPJMu84CzLi831ynw1K74NMiBcMzsjmEHkdl7Xzzg8G6NFLbaCTrAuW3/JAxdeV\n\t2VMEnZoxlRh1NxQoFGuPZ/AmEItHlXS9Xq0b/PjdsAVkuNIg8dsBilB2CR8hzPXDD365\n\tBo/FxyslnQDsxbl1yETlZbrRlBCWMD4qU65Ha8KnKNwmuzzLmMIvzuJsIKpLEkOCbwZX\n\t61Hw==","X-Gm-Message-State":"AOAM530yy1hZ6WLb8UnW4Iz2HDtQjf3GcjpSx6NJ1QjAVKqz9QjPA21M\n\tkdj6K5bv7HvFOGwYdiY2wSKgoXbb6HWZzfGuWHjVxL2ApxGL2w==","X-Google-Smtp-Source":"ABdhPJxit9ARllXZXu6uSRJ6/Qn9/4xtJWCy9eKnNe9YyVP/9jsIqMSUJXVTkEzCN7lixfq5l+79wlw4GY3yMKjs75s=","X-Received":"by 2002:a50:8161:: with SMTP id\n\t88mr11070492edc.276.1632872735307; \n\tTue, 28 Sep 2021 16:45:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-4-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Wed, 29 Sep 2021 08:45:24 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 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":19957,"web_url":"https://patchwork.libcamera.org/comment/19957/","msgid":"","date":"2021-09-29T05:43:47","subject":"Re: [libcamera-devel] [PATCH v2 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\n\nOn 9/29/21 3:00 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Sep 28, 2021 at 09:55:36PM +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>> Signed-off-by: Umang Jain \n>> ---\n>> src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------\n>> src/android/camera_device.h | 15 +++++++++++-\n>> 2 files changed, 46 insertions(+), 13 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index a3b8a549..83030366 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -861,11 +861,12 @@ 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>> +\t\t\t\tcamera3_capture_request_t *request)\n> I don't think I've seen a reply to my review comment in v1:\n\n\nOh, I think I did reply and had a brief discussion with you about it on \nIRC. Nevermind, I think I got confused a bit yesterday and now I  have \naddressed the comment locally here as well. Works fine.\n\n>\n> Could this function take a Camera3RequestDescriptor pointer only ? It\n> should contain all the needed data. This can be done as a patch before\n> this one if desired, or here as it shouldn't be much extra work.\n>\n> This function uses the num_output_buffers, frame_number and\n> output_buffers members of camera3_capture_request_t. Those are copied to\n> the Camera3RequestDescriptor buffers_ and frameNumber_ members which you\n> can use here.\n>\n> Reviewed-by: Laurent Pinchart \n>\n>> {\n>> \tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>> \n>> -\tcamera3_capture_result_t result = {};\n>> +\tcamera3_capture_result_t &result = descriptor->captureResult_;\n>> \tresult.num_output_buffers = request->num_output_buffers;\n>> \tresult.frame_number = request->frame_number;\n>> \tresult.partial_result = 0;\n>> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)\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>> @@ -1052,13 +1053,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>> +\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(descriptors_.back().get(), camera3Request);\n>> +\t\t{\n>> +\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n>> +\t\t\tdescriptors_.push(std::move(descriptor));\n>> +\t\t}\n>> +\t\tsendCaptureResults();\n>> \t\treturn 0;\n>> \t}\n>> \n>> @@ -1116,7 +1123,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>> @@ -1166,9 +1173,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>> @@ -1234,10 +1241,23 @@ void CameraDevice::requestComplete(Request *request)\n>> \t}\n>> \n>> \tcaptureResult.result = resultMetadata->get();\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\tstd::unique_ptr descriptor =\n>> +\t\t\tstd::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..dbfa7431 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,14 @@ 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>> +\t\t\t camera3_capture_request_t *request);\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>>","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 71BDAC3243\n\tfor ;\n\tWed, 29 Sep 2021 05:43:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7B6C69191;\n\tWed, 29 Sep 2021 07:43:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDEBF602DD\n\tfor ;\n\tWed, 29 Sep 2021 07:43:52 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EACFCD87;\n\tWed, 29 Sep 2021 07:43:51 +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=\"dNj4JBpv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632894232;\n\tbh=hJoWykdB6E9IMA9azWQrKR5ljBmmTXzG2/IBr+qwrTE=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=dNj4JBpvnkRQlTtJefJGZByi21JvMNNnG7OTKHa1NEWJJm7Nfodjr/7aGMV2kCzya\n\tOMKAjOZuYErwPO49V4DbEBmEjjKlNQ8zabQaLcy9TNGkZ1TUdYzriQE3OLWSfS2P1C\n\tzsaPgoG3yIZVsqATSpRXDXcewbdKXog0SpPZ4uCw=","To":"Laurent Pinchart ","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-4-umang.jain@ideasonboard.com>\n\t","From":"Umang Jain ","Message-ID":"","Date":"Wed, 29 Sep 2021 11:13:47 +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 v2 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":19962,"web_url":"https://patchwork.libcamera.org/comment/19962/","msgid":"","date":"2021-09-29T07:10:51","subject":"Re: [libcamera-devel] [PATCH v2 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 Hiro,\n\nOn Wed, Sep 29, 2021 at 08:45:24AM +0900, Hirokazu Honda wrote:\n> On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart wrote:\n> > On Tue, Sep 28, 2021 at 09:55:36PM +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> > > Signed-off-by: Umang Jain \n> > > ---\n> > > src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------\n> > > src/android/camera_device.h | 15 +++++++++++-\n> > > 2 files changed, 46 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index a3b8a549..83030366 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -861,11 +861,12 @@ 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> > > + camera3_capture_request_t *request)\n> >\n> > I don't think I've seen a reply to my review comment in v1:\n> >\n> > Could this function take a Camera3RequestDescriptor pointer only ? It\n> > should contain all the needed data. This can be done as a patch before\n> > this one if desired, or here as it shouldn't be much extra work.\n> >\n> > This function uses the num_output_buffers, frame_number and\n> > output_buffers members of camera3_capture_request_t. Those are copied to\n> > the Camera3RequestDescriptor buffers_ and frameNumber_ members which you\n> > can use here.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > {\n> > > notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > >\n> > > - camera3_capture_result_t result = {};\n> > > + camera3_capture_result_t &result = descriptor->captureResult_;\n> > > result.num_output_buffers = request->num_output_buffers;\n> > > result.frame_number = request->frame_number;\n> > > result.partial_result = 0;\n> > > @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > > }\n> > > result.output_buffers = resultBuffers.data();\n> > >\n> > > - callbacks_->process_capture_result(callbacks_, &result);\n> > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n> > > }\n> > >\n> > > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n> > > @@ -1052,13 +1053,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(descriptors_.back().get(), camera3Request);\n> > > + {\n> > > + MutexLocker descriptorsLock(descriptorsMutex_);\n> > > + descriptors_.push(std::move(descriptor));\n> > > + }\n> \n> Could you tell me what happens here?\n> I think the current request, camear3Request needs to be cancelled.\n> But abortRequest(descriptors_.back().get(), camera3Request) is called\n> before pushing.\n> So it aborts the last pushed request with the current request.\n\nOops indeed.\n\n> I wonder if the correct code is below.\n> if (state_ == State::Flushing)\n> {\n> {\n> MutexLocker descriptorsLock(descriptorsMutex_);\n> descriptors_.push(std::move(descriptor));\n> abortRequest(descriptors_.back().get(), camera3Request);\n> }\n> sendCaptureResults();\n> return 0;\n> }\n\nabortRequest() is supposed to operator on the request it receives as a\nparameter, so I don't think it needs to be covered by the lock. I think\n\n\t\tabortRequest(descriptor.get(), camera3Request);\n\t\t{\n\t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n\t\t\tdescriptors_.push(std::move(descriptor));\n\t\t}\n\t\tsendCaptureResults();\n\nmay be enough to fix the issue.\n\n> > > + sendCaptureResults();\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1116,7 +1123,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> > > @@ -1166,9 +1173,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> > > @@ -1234,10 +1241,23 @@ 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> > > + std::unique_ptr descriptor =\n> > > + std::move(descriptors_.front());\n> \n> nit: I would use auto here.\n> \n> > > + descriptors_.pop();\n> > > +\n> > > + lock.unlock();\n> > > + callbacks_->process_capture_result(callbacks_,\n> > > + &descriptor->captureResult_);\n> > > + lock.lock();\n> \n> Why is lock released during calling the callback? Is there any deadlock here?\n\nThe Android camera HAL API doesn't specify this as far as I know, so we\ndecided to minmize lock contention as the default rule.\n\n> I wonder if it might be less efficient to release and re-acquire lock\n> every call than just holding a lock entirely.\n\nThat's a good question, the only way to know would be to measure\nperformances for both options I suppose.\n\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..dbfa7431 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> > > 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,14 @@ 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> > > + camera3_capture_request_t *request);\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 782CAC3243\n\tfor ;\n\tWed, 29 Sep 2021 07:10:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CCF1C69193;\n\tWed, 29 Sep 2021 09:10: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 881456918A\n\tfor ;\n\tWed, 29 Sep 2021 09:10:55 +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 73DC03F0;\n\tWed, 29 Sep 2021 09:10:54 +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=\"l6TSi4XK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632899455;\n\tbh=9sM9axxhxO0HEs83ZbHko8qCmu03bPuRnaSSZ6CzJJA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l6TSi4XKkH1BjEZupuv8O3VJzP3iTd/D3Ubwp/acqYDZn3xHNyKAC1D3YNgXMjM9O\n\tfg0Kfn2GIwUhBj1EkCjlQWdDDi1JDnN+XqSrn9IaORW2yky6PZ5l+s1/Q2XjQTMvsN\n\tRhEMJfjBk693l2kbz11P6KGarPcUfPOIg6Cb5DNY=","Date":"Wed, 29 Sep 2021 10:10:51 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-4-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 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":19968,"web_url":"https://patchwork.libcamera.org/comment/19968/","msgid":"<1217c035-10e3-27ca-0ad5-cb55972f87af@ideasonboard.com>","date":"2021-09-29T07:34:57","subject":"Re: [libcamera-devel] [PATCH v2 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":"Hello,\n\nOn 9/29/21 12:40 PM, Laurent Pinchart wrote:\n> Hi Hiro,\n>\n> On Wed, Sep 29, 2021 at 08:45:24AM +0900, Hirokazu Honda wrote:\n>> On Wed, Sep 29, 2021 at 6:30 AM Laurent Pinchart wrote:\n>>> On Tue, Sep 28, 2021 at 09:55:36PM +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>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> src/android/camera_device.cpp | 44 +++++++++++++++++++++++++----------\n>>>> src/android/camera_device.h | 15 +++++++++++-\n>>>> 2 files changed, 46 insertions(+), 13 deletions(-)\n>>>>\n>>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>>> index a3b8a549..83030366 100644\n>>>> --- a/src/android/camera_device.cpp\n>>>> +++ b/src/android/camera_device.cpp\n>>>> @@ -861,11 +861,12 @@ 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>>>> + camera3_capture_request_t *request)\n>>> I don't think I've seen a reply to my review comment in v1:\n>>>\n>>> Could this function take a Camera3RequestDescriptor pointer only ? It\n>>> should contain all the needed data. This can be done as a patch before\n>>> this one if desired, or here as it shouldn't be much extra work.\n>>>\n>>> This function uses the num_output_buffers, frame_number and\n>>> output_buffers members of camera3_capture_request_t. Those are copied to\n>>> the Camera3RequestDescriptor buffers_ and frameNumber_ members which you\n>>> can use here.\n>>>\n>>> Reviewed-by: Laurent Pinchart \n>>>\n>>>> {\n>>>> notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n>>>>\n>>>> - camera3_capture_result_t result = {};\n>>>> + camera3_capture_result_t &result = descriptor->captureResult_;\n>>>> result.num_output_buffers = request->num_output_buffers;\n>>>> result.frame_number = request->frame_number;\n>>>> result.partial_result = 0;\n>>>> @@ -879,7 +880,7 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)\n>>>> }\n>>>> result.output_buffers = resultBuffers.data();\n>>>>\n>>>> - callbacks_->process_capture_result(callbacks_, &result);\n>>>> + descriptor->status_ = Camera3RequestDescriptor::Status::Error;\n>>>> }\n>>>>\n>>>> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const\n>>>> @@ -1052,13 +1053,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(descriptors_.back().get(), camera3Request);\n>>>> + {\n>>>> + MutexLocker descriptorsLock(descriptorsMutex_);\n>>>> + descriptors_.push(std::move(descriptor));\n>>>> + }\n>> Could you tell me what happens here?\n>> I think the current request, camear3Request needs to be cancelled.\n>> But abortRequest(descriptors_.back().get(), camera3Request) is called\n>> before pushing.\n>> So it aborts the last pushed request with the current request.\n> Oops indeed.\n>\n>> I wonder if the correct code is below.\n>> if (state_ == State::Flushing)\n>> {\n>> {\n>> MutexLocker descriptorsLock(descriptorsMutex_);\n>> descriptors_.push(std::move(descriptor));\n>> abortRequest(descriptors_.back().get(), camera3Request);\n>> }\n>> sendCaptureResults();\n>> return 0;\n>> }\n> abortRequest() is supposed to operator on the request it receives as a\n> parameter, so I don't think it needs to be covered by the lock. I think\n>\n> \t\tabortRequest(descriptor.get(), camera3Request);\n> \t\t{\n> \t\t\tMutexLocker descriptorsLock(descriptorsMutex_);\n> \t\t\tdescriptors_.push(std::move(descriptor));\n> \t\t}\n> \t\tsendCaptureResults();\n>\n> may be enough to fix the issue.\n\n\nYep, addressed in v3, thanks for pointing it out\n\n>\n>>>> + sendCaptureResults();\n>>>> return 0;\n>>>> }\n>>>>\n>>>> @@ -1116,7 +1123,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>>>> @@ -1166,9 +1173,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>>>> @@ -1234,10 +1241,23 @@ 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>>>> + std::unique_ptr descriptor =\n>>>> + std::move(descriptors_.front());\n>> nit: I would use auto here.\n>>\n>>>> + descriptors_.pop();\n>>>> +\n>>>> + lock.unlock();\n>>>> + callbacks_->process_capture_result(callbacks_,\n>>>> + &descriptor->captureResult_);\n>>>> + lock.lock();\n>> Why is lock released during calling the callback? Is there any deadlock here?\n> The Android camera HAL API doesn't specify this as far as I know, so we\n> decided to minmize lock contention as the default rule.\n>\n>> I wonder if it might be less efficient to release and re-acquire lock\n>> every call than just holding a lock entirely.\n> That's a good question, the only way to know would be to measure\n> performances for both options I suppose.\n\n\nThe process_capture_resultperformance is specified as:\n\n     * This is a non-blocking call. The framework will return this call \nin 5ms.\n     *\n     * The pipeline latency (see S7 for definition) should be less than \nor equal to\n     * 4 frame intervals, and must be less than or equal to 8 frame \nintervals.\n\nhttps://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#2781\n\nI don't think bring callback under the lock would have much impact on \nthe performance, fps-wise?\n\n>\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..dbfa7431 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>>>> 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,14 @@ 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>>>> + camera3_capture_request_t *request);\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 86CB6C3243\n\tfor ;\n\tWed, 29 Sep 2021 07:35:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2D7E69193;\n\tWed, 29 Sep 2021 09:35:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 58DD86918A\n\tfor ;\n\tWed, 29 Sep 2021 09:35:03 +0200 (CEST)","from [192.168.1.104] (unknown [103.251.226.9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E14223F0;\n\tWed, 29 Sep 2021 09:35:01 +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=\"CCuzyVLP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632900903;\n\tbh=5GJBI1dbaiusA8sHedSp/0Oz8S3BGbM3pk4Q3/qvTzQ=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=CCuzyVLPgtDvUq9pvc5XjhvFAbfP7HrwrAbviXNBEu5MifMtfxBSCNW28UEk02xfi\n\tZUpR+c3VnrSqoubt+CactSramzd/qqivJpVDnKeuHvvlTff9tcfiG/8uIutwiO9f5s\n\tmew+du+L1mdGhxy1CzE9FKmoRcpu3wRQy4dF61Cw=","To":"Laurent Pinchart ,\n\tHirokazu Honda ","References":"<20210928162536.227475-1-umang.jain@ideasonboard.com>\n\t<20210928162536.227475-4-umang.jain@ideasonboard.com>\n\t\n\t\n\t","From":"Umang Jain ","Message-ID":"<1217c035-10e3-27ca-0ad5-cb55972f87af@ideasonboard.com>","Date":"Wed, 29 Sep 2021 13:04:57 +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":"multipart/alternative;\n\tboundary=\"------------9A9CD31287C18418904DF37C\"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v2 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\" "}}]