[{"id":16125,"web_url":"https://patchwork.libcamera.org/comment/16125/","msgid":"<20210406132418.caaoxy6d3mvglzgq@uno.localdomain>","date":"2021-04-06T13:24:18","subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote:\n> This implements camera3_device_ops::flush().\n>\n\nIs this enough ? reading the flush() operation documentation in\ncamera3.h it seems it can be called concurrently to\nprocess_capture_request() and requests yet to be queued shall be\nreturned immediately. Should that be handled as well ?\n\n> Signed-off-by: Hirokazu Honda \n> ---\n> src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++----\n> src/android/camera_device.h | 3 +++\n> src/android/camera_ops.cpp | 3 ++-\n> 3 files changed, 48 insertions(+), 6 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 4a3f6f8e..5a56fe4b 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request)\n> \tcamera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> \tstd::unique_ptr resultMetadata;\n>\n> -\tdecltype(descriptors_)::node_type node;\n> +\tCamera3RequestDescriptor descriptor;\n> \t{\n> \t\tstd::scoped_lock lock(mutex_);\n> \t\tauto it = descriptors_.find(request->cookie());\n> \t\tif (it == descriptors_.end()) {\n> \t\t\tLOG(HAL, Fatal)\n> \t\t\t\t<< \"Unknown request: \" << request->cookie();\n> -\t\t\tstatus = CAMERA3_BUFFER_STATUS_ERROR;\n> \t\t\treturn;\n> \t\t}\n>\n> -\t\tnode = descriptors_.extract(it);\n> +\t\tdescriptor = std::move(it->second);\n> +\t\t/* Restore frameNumber_ because it is used in the case of flush\n> +\t\t * timeout case.\n> +\t\t */\n> +\t\tit->second.frameNumber_ = descriptor.frameNumber_;\n\nIs accessing it->second after having moved it valid ? also, aren't\nthe lhs and rhs referring to the same variable ?\n\n> \t}\n> -\tCamera3RequestDescriptor &descriptor = node.mapped();\n>\n> \tif (request->status() != Request::RequestComplete) {\n> \t\tLOG(HAL, Error) << \"Request not successfully completed: \"\n> @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request)\n> \t\t\t descriptor.buffers_[0].stream);\n> \t}\n>\n> -\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t{\n> +\t\tstd::scoped_lock lock(mutex_);\n> +\t\tif (descriptors_.erase(request->cookie()) == 0) {\n> +\t\t\t// flush() cleans up the descriptor due to time out\n> +\t\t\t// during a post processing.\n\nNo C++ comments please\n\n> +\t\t\treturn;\n> +\t\t}\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t\tconditionVariable_.notify_one();\n\naren't this function and flush() being run in the same thread ? Won't\nthis contend the mutex ?\n\nTo be honest I would have expected flush to stop the libcamera::Camera\nto have all requests in-flight be immediately be completed with\nerrors. As said flush also prevents calls to\nprocess_capture_request() to be queued if flush is called concurrently\n(which makes me think my assumption on the flush calls happening on the same\nthread to be wrong).\n\nAnother question is how to interrupt post-processing to happen once it\nwill be moved to a separate thread. If flush() times out and delete\nthe descriptor, won't post-processor then tries to access a non-valid\nmemory address ?\n\nThanks\n j\n\n> +\t}\n> +}\n> +\n> +int CameraDevice::flush()\n> +{\n> +\tstd::unique_lock lock(mutex_);\n> +\t/* flush() should return in less than one second. Sets the timeout to 900ms. */\n> +\tauto flushTimeout =\n> +\t\tstd::chrono::system_clock::now() + std::chrono::milliseconds(900);\n> +\tbool completeAllRequests = conditionVariable_.wait_until(\n> +\t\tlock, flushTimeout, [this]() { return descriptors_.empty(); });\n> +\n> +\tif (!completeAllRequests) {\n> +\t\tfor (auto &node : descriptors_) {\n> +\t\t\tauto &descriptor = node.second;\n> +\t\t\tcamera3_capture_result_t captureResult{};\n> +\t\t\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\t\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\t\t\tbuffer.acquire_fence = -1;\n> +\t\t\t\tbuffer.release_fence = -1;\n> +\t\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t\t}\n> +\t\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n> +\t\t}\n> +\t\tdescriptors_.clear();\n> +\t}\n> +\n> +\treturn 0;\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 c63e8e21..dd5336ee 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -7,6 +7,7 @@\n> #ifndef __ANDROID_CAMERA_DEVICE_H__\n> #define __ANDROID_CAMERA_DEVICE_H__\n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -62,6 +63,7 @@ public:\n> \tint configureStreams(camera3_stream_configuration_t *stream_list);\n> \tint processCaptureRequest(camera3_capture_request_t *request);\n> \tvoid requestComplete(libcamera::Request *request);\n> +\tint flush();\n>\n> protected:\n> \tstd::string logPrefix() const override;\n> @@ -128,6 +130,7 @@ private:\n> \tstd::vector streams_;\n>\n> \tstd::mutex mutex_; /* Protect descriptors_ */\n> +\tstd::condition_variable conditionVariable_;\n> \tstd::map descriptors_;\n>\n> \tstd::string maker_;\n> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e8043..1b73af13 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n>\n> static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> {\n> -\treturn 0;\n> +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> +\treturn camera->flush();\n> }\n>\n> int hal_dev_close(hw_device_t *hw_device)\n> --\n> 2.31.0.208.g409f899ff0-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 91201BD695\n\tfor ;\n\tTue, 6 Apr 2021 13:23:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E361868786;\n\tTue, 6 Apr 2021 15:23:44 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 414C860518\n\tfor ;\n\tTue, 6 Apr 2021 15:23:43 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id B23F3E0003;\n\tTue, 6 Apr 2021 13:23:42 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 6 Apr 2021 15:24:18 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210406132418.caaoxy6d3mvglzgq@uno.localdomain>","References":"<20210406091334.3823748-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210406091334.3823748-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16154,"web_url":"https://patchwork.libcamera.org/comment/16154/","msgid":"","date":"2021-04-08T09:17:00","subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Thanks Jacopo for comments,\nYou're right. I missed the description. I will fix it.\n\nOn Tue, Apr 6, 2021 at 10:23 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote:\n> > This implements camera3_device_ops::flush().\n> >\n>\n> Is this enough ? reading the flush() operation documentation in\n> camera3.h it seems it can be called concurrently to\n> process_capture_request() and requests yet to be queued shall be\n> returned immediately. Should that be handled as well ?\n>\n> > Signed-off-by: Hirokazu Honda \n> > ---\n> > src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++----\n> > src/android/camera_device.h | 3 +++\n> > src/android/camera_ops.cpp | 3 ++-\n> > 3 files changed, 48 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 4a3f6f8e..5a56fe4b 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request)\n> > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > std::unique_ptr resultMetadata;\n> >\n> > - decltype(descriptors_)::node_type node;\n> > + Camera3RequestDescriptor descriptor;\n> > {\n> > std::scoped_lock lock(mutex_);\n> > auto it = descriptors_.find(request->cookie());\n> > if (it == descriptors_.end()) {\n> > LOG(HAL, Fatal)\n> > << \"Unknown request: \" << request->cookie();\n> > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > return;\n> > }\n> >\n> > - node = descriptors_.extract(it);\n> > + descriptor = std::move(it->second);\n> > + /* Restore frameNumber_ because it is used in the case of flush\n> > + * timeout case.\n> > + */\n> > + it->second.frameNumber_ = descriptor.frameNumber_;\n>\n> Is accessing it->second after having moved it valid ? also, aren't\n> the lhs and rhs referring to the same variable ?\n>\n> > }\n> > - Camera3RequestDescriptor &descriptor = node.mapped();\n> >\n> > if (request->status() != Request::RequestComplete) {\n> > LOG(HAL, Error) << \"Request not successfully completed: \"\n> > @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request)\n> > descriptor.buffers_[0].stream);\n> > }\n> >\n> > - callbacks_->process_capture_result(callbacks_, &captureResult);\n> > + {\n> > + std::scoped_lock lock(mutex_);\n> > + if (descriptors_.erase(request->cookie()) == 0) {\n> > + // flush() cleans up the descriptor due to time out\n> > + // during a post processing.\n>\n> No C++ comments please\n>\n> > + return;\n> > + }\n> > + callbacks_->process_capture_result(callbacks_, &captureResult);\n> > + conditionVariable_.notify_one();\n>\n> aren't this function and flush() being run in the same thread ? Won't\n> this contend the mutex ?\n>\n> To be honest I would have expected flush to stop the libcamera::Camera\n> to have all requests in-flight be immediately be completed with\n> errors. As said flush also prevents calls to\n> process_capture_request() to be queued if flush is called concurrently\n> (which makes me think my assumption on the flush calls happening on the same\n> thread to be wrong).\n>\n> Another question is how to interrupt post-processing to happen once it\n> will be moved to a separate thread. If flush() times out and delete\n> the descriptor, won't post-processor then tries to access a non-valid\n> memory address ?\n>\n> Thanks\n> j\n>\n> > + }\n> > +}\n> > +\n> > +int CameraDevice::flush()\n> > +{\n> > + std::unique_lock lock(mutex_);\n> > + /* flush() should return in less than one second. Sets the timeout to 900ms. */\n> > + auto flushTimeout =\n> > + std::chrono::system_clock::now() + std::chrono::milliseconds(900);\n> > + bool completeAllRequests = conditionVariable_.wait_until(\n> > + lock, flushTimeout, [this]() { return descriptors_.empty(); });\n> > +\n> > + if (!completeAllRequests) {\n> > + for (auto &node : descriptors_) {\n> > + auto &descriptor = node.second;\n> > + camera3_capture_result_t captureResult{};\n> > + captureResult.frame_number = descriptor.frameNumber_;\n> > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > + buffer.acquire_fence = -1;\n> > + buffer.release_fence = -1;\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + }\n> > + callbacks_->process_capture_result(callbacks_, &captureResult);\n> > + }\n> > + descriptors_.clear();\n> > + }\n> > +\n> > + return 0;\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 c63e8e21..dd5336ee 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -7,6 +7,7 @@\n> > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > #define __ANDROID_CAMERA_DEVICE_H__\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -62,6 +63,7 @@ public:\n> > int configureStreams(camera3_stream_configuration_t *stream_list);\n> > int processCaptureRequest(camera3_capture_request_t *request);\n> > void requestComplete(libcamera::Request *request);\n> > + int flush();\n> >\n> > protected:\n> > std::string logPrefix() const override;\n> > @@ -128,6 +130,7 @@ private:\n> > std::vector streams_;\n> >\n> > std::mutex mutex_; /* Protect descriptors_ */\n> > + std::condition_variable conditionVariable_;\n> > std::map descriptors_;\n> >\n> > std::string maker_;\n> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e8043..1b73af13 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> >\n> > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > {\n> > - return 0;\n> > + CameraDevice *camera = reinterpret_cast(dev->priv);\n> > + return camera->flush();\n> > }\n> >\n> > int hal_dev_close(hw_device_t *hw_device)\n> > --\n> > 2.31.0.208.g409f899ff0-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","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 E9A8ABD1F6\n\tfor ;\n\tThu, 8 Apr 2021 09:17:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 74685687F5;\n\tThu, 8 Apr 2021 11:17:13 +0200 (CEST)","from mail-ed1-x531.google.com (mail-ed1-x531.google.com\n\t[IPv6:2a00:1450:4864:20::531])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 287E8687F2\n\tfor ;\n\tThu, 8 Apr 2021 11:17:11 +0200 (CEST)","by mail-ed1-x531.google.com with SMTP id r22so1506196edq.9\n\tfor ;\n\tThu, 08 Apr 2021 02:17:11 -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=\"b+KaxRku\"; 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=CZVaSngb7WOPZqy4d1aPPWVcQND24EIC0/+569fZsBg=;\n\tb=b+KaxRkuF+OzzxGQ4ItUxKynBGx+Fttu0+TmWuhc272tAr/01ToMqsFl8nVc+QOef1\n\tElIOhlhlk1FKXVKfxyruF36um5BjtyH/zy9kcJOqc6Thw+8Kvwlshkcfo2/UPINDpfFV\n\tdvBSG1dgOvwY2qkzCm8nO9JrTdHJC0YFrGDZk=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=CZVaSngb7WOPZqy4d1aPPWVcQND24EIC0/+569fZsBg=;\n\tb=SU3oL/zxo/1D9OPrDahHc0KqvHzlb3e8UkI5FA3krHI1V79xTpCEOICk42bivLubbE\n\tDTrNbJxTtwv5ZjrsGTzoh1+K2qNCavaLvAtxSzk/wwAc+nxwc1XnbzFJz9uuFZvo/ghg\n\t4YHxa5dD44/F9a5zpUhm3Xoo7dlOTD2LsSV4XRtCtTgAn/tMTEsIB/uWMooCxmvJRvmF\n\tdiy7nPKxz2igPy9Dv6qi+pLcuePShtjBLjOEZdme7q3t3t267wRRNlKco+x1kq3t5PJu\n\tikhFpOA0idhpfxXMjunVca0MWAj+XqTrxyPFvCQypJ9pypu7XGB4IMvlbylPf2l8qlot\n\tGwfQ==","X-Gm-Message-State":"AOAM533RDR/bAKd5Z8HiV/pgwk+7zhWWsFCQfcm/KIC17msyLrqFTSKv\n\taRspmuLLpMJ2XYLVk4oQkar59Rd8jIl+gs3BQO8Vmg==","X-Google-Smtp-Source":"ABdhPJyzM8HVgaah4RK/tLcGsxcqWpazBz1x8mD13jkARg/BjDCg2KUfaHc+qBUXH0p0OtpPgG4gexjNONfMV+YtWq4=","X-Received":"by 2002:a05:6402:5211:: with SMTP id\n\ts17mr10021385edd.327.1617873430826; \n\tThu, 08 Apr 2021 02:17:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20210406091334.3823748-1-hiroh@chromium.org>\n\t<20210406132418.caaoxy6d3mvglzgq@uno.localdomain>","In-Reply-To":"<20210406132418.caaoxy6d3mvglzgq@uno.localdomain>","From":"Hirokazu Honda ","Date":"Thu, 8 Apr 2021 18:17:00 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16209,"web_url":"https://patchwork.libcamera.org/comment/16209/","msgid":"","date":"2021-04-13T03:06:22","subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Thu, Apr 08, 2021 at 06:17:00PM +0900, Hirokazu Honda wrote:\n> Thanks Jacopo for comments,\n> You're right. I missed the description. I will fix it.\n> \n> On Tue, Apr 6, 2021 at 10:23 PM Jacopo Mondi wrote:\n> > On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote:\n> > > This implements camera3_device_ops::flush().\n> >\n> > Is this enough ? reading the flush() operation documentation in\n> > camera3.h it seems it can be called concurrently to\n> > process_capture_request() and requests yet to be queued shall be\n> > returned immediately. Should that be handled as well ?\n\nThe documentation states\n\n * flush() may be called concurrently to processCaptureRequest(), with the\n * expectation that processCaptureRequest returns quickly and the\n * request submitted in that processCaptureRequest call is treated like\n * all other in-flight requests. Due to concurrency issues, it is possible\n * that from the HAL's point of view, a processCaptureRequest() call may\n * be started after flush has been invoked but has not returned yet. If such\n * a call happens before flush() returns, the HAL must treat the new\n * capture request like other in-flight pending requests (see #4 below).\n\nThis seems to be a fairly ill-defined behaviour, as in theory, the late\nprocessCaptureRequest() call that races flush() could arrive after\nflush() returns, and be interpreted as an instruction to restart the\ncamera. This should ideally be clarified with Android developers.\n\n> > > Signed-off-by: Hirokazu Honda \n> > > ---\n> > > src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++----\n> > > src/android/camera_device.h | 3 +++\n> > > src/android/camera_ops.cpp | 3 ++-\n> > > 3 files changed, 48 insertions(+), 6 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 4a3f6f8e..5a56fe4b 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request)\n> > > camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;\n> > > std::unique_ptr resultMetadata;\n> > >\n> > > - decltype(descriptors_)::node_type node;\n> > > + Camera3RequestDescriptor descriptor;\n> > > {\n> > > std::scoped_lock lock(mutex_);\n> > > auto it = descriptors_.find(request->cookie());\n> > > if (it == descriptors_.end()) {\n> > > LOG(HAL, Fatal)\n> > > << \"Unknown request: \" << request->cookie();\n> > > - status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > return;\n> > > }\n> > >\n> > > - node = descriptors_.extract(it);\n> > > + descriptor = std::move(it->second);\n> > > + /* Restore frameNumber_ because it is used in the case of flush\n> > > + * timeout case.\n> > > + */\n> > > + it->second.frameNumber_ = descriptor.frameNumber_;\n> >\n> > Is accessing it->second after having moved it valid ? also, aren't\n> > the lhs and rhs referring to the same variable ?\n> >\n> > > }\n> > > - Camera3RequestDescriptor &descriptor = node.mapped();\n> > >\n> > > if (request->status() != Request::RequestComplete) {\n> > > LOG(HAL, Error) << \"Request not successfully completed: \"\n> > > @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request)\n> > > descriptor.buffers_[0].stream);\n> > > }\n> > >\n> > > - callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > + {\n> > > + std::scoped_lock lock(mutex_);\n> > > + if (descriptors_.erase(request->cookie()) == 0) {\n> > > + // flush() cleans up the descriptor due to time out\n> > > + // during a post processing.\n> >\n> > No C++ comments please\n> >\n> > > + return;\n> > > + }\n> > > + callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > + conditionVariable_.notify_one();\n\nThe condition variable should be signalled without he lock held.\n\n> >\n> > aren't this function and flush() being run in the same thread ? Won't\n> > this contend the mutex ?\n> >\n> > To be honest I would have expected flush to stop the libcamera::Camera\n> > to have all requests in-flight be immediately be completed with\n> > errors. As said flush also prevents calls to\n> > process_capture_request() to be queued if flush is called concurrently\n> > (which makes me think my assumption on the flush calls happening on the same\n> > thread to be wrong).\n\nAgreed, I would also expect flush() to call Camera::stop(). The hardware\nwill effectively have to stop, as there will be no buffers anymore.\n\n> > Another question is how to interrupt post-processing to happen once it\n> > will be moved to a separate thread. If flush() times out and delete\n> > the descriptor, won't post-processor then tries to access a non-valid\n> > memory address ?\n\nWe'll have to wait on the post-processing tasks, possibly with\ncancellation points in the implementation (it would be interesting to\nadd cancellation points to libyuv).\n\n> > > + }\n> > > +}\n> > > +\n> > > +int CameraDevice::flush()\n> > > +{\n> > > + std::unique_lock lock(mutex_);\n> > > + /* flush() should return in less than one second. Sets the timeout to 900ms. */\n\nAs Jacopo pointed out, this is dangerous. I'd rather print a warning if\nwe take too long than risk a crash.\n\n> > > + auto flushTimeout =\n> > > + std::chrono::system_clock::now() + std::chrono::milliseconds(900);\n> > > + bool completeAllRequests = conditionVariable_.wait_until(\n> > > + lock, flushTimeout, [this]() { return descriptors_.empty(); });\n> > > +\n> > > + if (!completeAllRequests) {\n> > > + for (auto &node : descriptors_) {\n> > > + auto &descriptor = node.second;\n> > > + camera3_capture_result_t captureResult{};\n> > > + captureResult.frame_number = descriptor.frameNumber_;\n> > > + for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > + buffer.acquire_fence = -1;\n> > > + buffer.release_fence = -1;\n> > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > + }\n> > > + callbacks_->process_capture_result(callbacks_, &captureResult);\n> > > + }\n> > > + descriptors_.clear();\n> > > + }\n> > > +\n> > > + return 0;\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 c63e8e21..dd5336ee 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -7,6 +7,7 @@\n> > > #ifndef __ANDROID_CAMERA_DEVICE_H__\n> > > #define __ANDROID_CAMERA_DEVICE_H__\n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -62,6 +63,7 @@ public:\n> > > int configureStreams(camera3_stream_configuration_t *stream_list);\n> > > int processCaptureRequest(camera3_capture_request_t *request);\n> > > void requestComplete(libcamera::Request *request);\n> > > + int flush();\n> > >\n> > > protected:\n> > > std::string logPrefix() const override;\n> > > @@ -128,6 +130,7 @@ private:\n> > > std::vector streams_;\n> > >\n> > > std::mutex mutex_; /* Protect descriptors_ */\n> > > + std::condition_variable conditionVariable_;\n> > > std::map descriptors_;\n> > >\n> > > std::string maker_;\n> > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > index 696e8043..1b73af13 100644\n> > > --- a/src/android/camera_ops.cpp\n> > > +++ b/src/android/camera_ops.cpp\n> > > @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > >\n> > > static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > > {\n> > > - return 0;\n> > > + CameraDevice *camera = reinterpret_cast(dev->priv);\n> > > + return camera->flush();\n> > > }\n> > >\n> > > int hal_dev_close(hw_device_t *hw_device)","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 EF864BD1F6\n\tfor ;\n\tTue, 13 Apr 2021 03:07:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 614DB687FF;\n\tTue, 13 Apr 2021 05:07:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0399B687F4\n\tfor ;\n\tTue, 13 Apr 2021 05:07:11 +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 6AF456F2;\n\tTue, 13 Apr 2021 05:07:11 +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=\"C8ZHdy2b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1618283231;\n\tbh=HlnuagR1gB1I925D5Qq33lzLjx/I2C5wf5p0tN8y7TY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=C8ZHdy2bmI1Ks9nT54RhRpzOMvN/W/FSG6SkqyD7jCh99oD9tG8kDlgkYwn2id1oz\n\tnYd0IkOirSm+p/5yW/WlbMrRpVUpZ+pL78C+qzKeMzZ1ofutuFdNBpSaA3SVp0dlEd\n\tkz/io73EbYnVbz8OzKafwrNTBMwFX16uyw0kss1Q=","Date":"Tue, 13 Apr 2021 06:06:22 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210406091334.3823748-1-hiroh@chromium.org>\n\t<20210406132418.caaoxy6d3mvglzgq@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: Implement flush()","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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]