[{"id":20433,"web_url":"https://patchwork.libcamera.org/comment/20433/","msgid":"<CAO5uPHPdG2WsR7JRqZU_B0rpgp71X2eS2KQrv5puNEby4d1wmA@mail.gmail.com>","date":"2021-10-25T12:29:15","subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","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 Sat, Oct 23, 2021 at 7:33 PM Umang Jain <umang.jain@ideasonboard.com> wrote:\n>\n> Introduce a dedicated worker class derived from libcamera::Thread.\n> The worker class maintains a queue for post-processing requests\n> and waits for a post-processing request to become available.\n> It will process them as per FIFO before de-queuing it from the\n> queue.\n>\n> The post processing streams loop in CameraDevice::requestComplete()\n> is tweaked a bit to better suit asynchronous needs of post processing.\n> Also, the entire iteration is locked under streamProcessMutex_ which\n> helps us to queue all the post-processing request at once, before any\n> of the post-processing completion slot (streamProcessingComplete())\n> is allowed to run for post-processing requests completing in parallel.\n> This helps us to manage both synchronous and asynchronous errors\n> encountered during the entire post processing operation.\n>\n> This patch also implements a flush() for the PostProcessorWorker\n> class which is responsible to purge post-processing requests\n> queued up while a camera is stopping/flushing. It is hooked with\n> CameraStream::flush(), which isn't used currently but will be\n> used when we handle flush/stop scenarios in greater detail\n> subsequently (in a different patchset).\n>\n> The libcamera request completion handler CameraDevice::requestComplete()\n> assumes that the request that has just completed is at the front of the\n> queue. Now that the post-processor runs asynchronously, this isn't true\n> anymore, a request being post-processed will stay in the queue and a new\n> libcamera request may complete. Remove that assumption, and use the\n> request cookie to obtain the Camera3RequestDescriptor.\n>\n> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/android/camera_device.cpp |  49 +++++------------\n>  src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n>  src/android/camera_stream.h   |  38 ++++++++++++-\n>  3 files changed, 148 insertions(+), 40 deletions(-)\n>\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 3114def0..dc39467b 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1026,29 +1026,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>\n>  void CameraDevice::requestComplete(Request *request)\n>  {\n> -       Camera3RequestDescriptor *descriptor;\n> -       {\n> -               MutexLocker descriptorsLock(descriptorsMutex_);\n> -               ASSERT(!descriptors_.empty());\n> -               descriptor = descriptors_.front().get();\n> -       }\n> -\n> -       if (descriptor->request_->cookie() != request->cookie()) {\n> -               /*\n> -                * \\todo Clarify if the Camera has to be closed on\n> -                * ERROR_DEVICE.\n> -                */\n> -               LOG(HAL, Error)\n> -                       << \"Out-of-order completion for request \"\n> -                       << utils::hex(request->cookie());\n> -\n> -               MutexLocker descriptorsLock(descriptorsMutex_);\n> -               descriptors_.pop();\n> -\n> -               notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> -\n> -               return;\n> -       }\n> +       Camera3RequestDescriptor *descriptor =\n> +               reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());\n>\n>         /*\n>          * Prepare the capture result for the Android camera stack.\n> @@ -1124,12 +1103,16 @@ void CameraDevice::requestComplete(Request *request)\n>\n>         /* Handle post-processing. */\n>         bool needPostProcessing = false;\n> +       MutexLocker locker(descriptor->streamsProcessMutex_);\n> +\n>         /*\n> -        * \\todo Protect the loop below with streamProcessMutex_ when post\n> -        * processor runs asynchronously.\n> +        * Queue all the post-processing streams request at once. The completion\n> +        * slot streamProcessingComplete() can only execute when we are out\n> +        * this critical section. This helps to handle synchronous errors here\n> +        * itself.\n>          */\n>         auto iter = descriptor->pendingStreamsToProcess_.begin();\n> -       while (descriptor->pendingStreamsToProcess_.size() > 0) {\n> +       while (iter != descriptor->pendingStreamsToProcess_.end()) {\n>                 CameraStream *stream = iter->first;\n>                 Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n>                 needPostProcessing = true;\n> @@ -1151,18 +1134,16 @@ void CameraDevice::requestComplete(Request *request)\n>         }\n>\n>         if (needPostProcessing) {\n> -               /*\n> -                * \\todo We will require to check if we failed to queue\n> -                * post-processing requests when we migrate to post-processor\n> -                * running asynchronously.\n> -                *\n> -                * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> -                *      completeDescriptor(descriptor);\n> -                */\n> +               if (descriptor->pendingStreamsToProcess_.size() == 0) {\n> +                       locker.unlock();\n> +                       completeDescriptor(descriptor);\n> +               }\n\nAs Laurent suggested in other patch, I think the code would be cleaner\nif we remove needPostProcessing.\n\n>\n>                 return;\n>         }\n>\n> +       locker.unlock();\n> +\n>         completeDescriptor(descriptor);\n>  }\n>\n> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> index 45d0607d..68b916e9 100644\n> --- a/src/android/camera_stream.cpp\n> +++ b/src/android/camera_stream.cpp\n> @@ -99,6 +99,7 @@ int CameraStream::configure()\n>                 if (ret)\n>                         return ret;\n>\n> +               worker_ = std::make_unique<PostProcessorWorker>(postProcessor_.get());\n>                 postProcessor_->processComplete.connect(\n>                         this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n>                                   PostProcessor::Status status) {\n> @@ -112,6 +113,8 @@ int CameraStream::configure()\n>                                 cameraDevice_->streamProcessingComplete(streamBuffer,\n>                                                                         bufferStatus);\n>                         });\n> +\n> +               worker_->start();\n>         }\n>\n>         if (type_ == Type::Internal) {\n> @@ -179,10 +182,6 @@ int CameraStream::process(const FrameBuffer &source,\n>\n>         ASSERT(type_ != Type::Direct);\n>\n> -       /*\n> -        * \\todo Buffer mapping and processing should be moved to a\n> -        * separate thread.\n> -        */\n>         const StreamConfiguration &output = configuration();\n>         dest.destBuffer = std::make_unique<CameraBuffer>(\n>                 *dest.camera3Buffer, output.pixelFormat, output.size,\n> @@ -194,11 +193,19 @@ int CameraStream::process(const FrameBuffer &source,\n>\n>         dest.srcBuffer = &source;\n>\n> -       postProcessor_->process(&dest);\n> +       worker_->queueRequest(&dest);\n>\n>         return 0;\n>  }\n>\n> +void CameraStream::flush()\n> +{\n> +       if (!postProcessor_)\n> +               return;\n> +\n> +       worker_->flush();\n> +}\n> +\n>  FrameBuffer *CameraStream::getBuffer()\n>  {\n>         if (!allocator_)\n> @@ -226,3 +233,87 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n>\n>         buffers_.push_back(buffer);\n>  }\n> +\n> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> +       : postProcessor_(postProcessor)\n> +{\n> +}\n> +\n> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> +{\n> +       {\n> +               libcamera::MutexLocker lock(mutex_);\n> +               state_ = State::Stopped;\n> +       }\n> +\n> +       cv_.notify_one();\n> +       wait();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::start()\n> +{\n> +       {\n> +               libcamera::MutexLocker lock(mutex_);\n> +               ASSERT(state_ != State::Running);\n> +               state_ = State::Running;\n> +       }\n> +\n> +       Thread::start();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n> +{\n> +       {\n> +               MutexLocker lock(mutex_);\n> +               ASSERT(state_ == State::Running);\n> +               requests_.push(dest);\n> +       }\n> +\n> +       cv_.notify_one();\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::run()\n> +{\n> +       MutexLocker locker(mutex_);\n> +\n> +       while (1) {\n> +               cv_.wait(locker, [&] {\n> +                       return state_ != State::Running || !requests_.empty();\n> +               });\n> +\n> +               if (state_ != State::Running)\n> +                       break;\n> +\n> +               Camera3RequestDescriptor::StreamBuffer *streamBuffer = requests_.front();\n> +               requests_.pop();\n> +               locker.unlock();\n> +\n> +               postProcessor_->process(streamBuffer);\n> +\n> +               locker.lock();\n> +       }\n> +\n> +       if (state_ == State::Flushing) {\n> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests =\n> +                       std::move(requests_);\n\nWhy is moving requests_ to a local variable necessary?\n\nReviewed-by: Hirokazu Honda<hiroh@chromium.org>\n> +               locker.unlock();\n> +\n> +               while (!requests.empty()) {\n> +                       postProcessor_->processComplete.emit(\n> +                               requests.front(), PostProcessor::Status::Error);\n> +                       requests.pop();\n> +               }\n> +\n> +               locker.lock();\n> +               state_ = State::Stopped;\n> +       }\n> +}\n> +\n> +void CameraStream::PostProcessorWorker::flush()\n> +{\n> +       libcamera::MutexLocker lock(mutex_);\n> +       state_ = State::Flushing;\n> +       lock.unlock();\n> +\n> +       cv_.notify_one();\n> +}\n> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> index e9c320b1..7c6e887c 100644\n> --- a/src/android/camera_stream.h\n> +++ b/src/android/camera_stream.h\n> @@ -7,12 +7,16 @@\n>  #ifndef __ANDROID_CAMERA_STREAM_H__\n>  #define __ANDROID_CAMERA_STREAM_H__\n>\n> +#include <condition_variable>\n>  #include <memory>\n>  #include <mutex>\n> +#include <queue>\n>  #include <vector>\n>\n>  #include <hardware/camera3.h>\n>\n> +#include <libcamera/base/thread.h>\n> +\n>  #include <libcamera/camera.h>\n>  #include <libcamera/framebuffer.h>\n>  #include <libcamera/framebuffer_allocator.h>\n> @@ -20,9 +24,9 @@\n>  #include <libcamera/pixel_format.h>\n>\n>  #include \"camera_request.h\"\n> +#include \"post_processor.h\"\n>\n>  class CameraDevice;\n> -class PostProcessor;\n>\n>  class CameraStream\n>  {\n> @@ -125,8 +129,38 @@ public:\n>                     Camera3RequestDescriptor::StreamBuffer &dest);\n>         libcamera::FrameBuffer *getBuffer();\n>         void putBuffer(libcamera::FrameBuffer *buffer);\n> +       void flush();\n>\n>  private:\n> +       class PostProcessorWorker : public libcamera::Thread\n> +       {\n> +       public:\n> +               enum class State {\n> +                       Stopped,\n> +                       Running,\n> +                       Flushing,\n> +               };\n> +\n> +               PostProcessorWorker(PostProcessor *postProcessor);\n> +               ~PostProcessorWorker();\n> +\n> +               void start();\n> +               void queueRequest(Camera3RequestDescriptor::StreamBuffer *request);\n> +               void flush();\n> +\n> +       protected:\n> +               void run() override;\n> +\n> +       private:\n> +               PostProcessor *postProcessor_;\n> +\n> +               libcamera::Mutex mutex_;\n> +               std::condition_variable cv_;\n> +\n> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests_;\n> +               State state_ = State::Stopped;\n> +       };\n> +\n>         int waitFence(int fence);\n>\n>         CameraDevice *const cameraDevice_;\n> @@ -143,6 +177,8 @@ private:\n>          */\n>         std::unique_ptr<std::mutex> mutex_;\n>         std::unique_ptr<PostProcessor> postProcessor_;\n> +\n> +       std::unique_ptr<PostProcessorWorker> worker_;\n>  };\n>\n>  #endif /* __ANDROID_CAMERA_STREAM__ */\n> --\n> 2.31.1\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E0D7DBF415\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 25 Oct 2021 12:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31B9264872;\n\tMon, 25 Oct 2021 14:29:32 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E870D60124\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 14:29:30 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id g8so10888591edb.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 05:29:30 -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=\"OHtBFsSR\"; 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=Q8VPBK9q/FOwgh1EV857Voz8BNFqOs+Cv9G3wWIN4aY=;\n\tb=OHtBFsSRpJf3zCirXpkxLXpzdXL4DqUg1XvvJaJfaqlqlTwxQRGLs3YjFSzNnygnUP\n\tM/hICT1R9HkacATLpPT557KcQbSQLYUuMDnJ45RpHWAMxTPQRS1XvNXcdcPlzzqUPgk6\n\tJmdeTJr5IqkrN3JsvG2pHCkCN5JxaPEy99ja8=","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=Q8VPBK9q/FOwgh1EV857Voz8BNFqOs+Cv9G3wWIN4aY=;\n\tb=CMSflLiEmczaKNp3OYnQVHFtphMSwzOsGo0f4kUgL3Z3BA6NIeThHh8aLTW9gq4rws\n\tnlqnBzYq4EwXe/f+jukaNAIU5FoIVjY69ro5R4Ctxoj/49KCfoWpgoszHIyFnq1FyA/Y\n\tRHU30UzE1GdvZxzs0i7z+hsjRpznraXwYWjHLLQhIHgt1Qnyjt1j8E+XBtsWYZMShGO6\n\t1jjpUx9prY1re5/B/R/TI7o9q0e6CmuDKf2ZQ4x6KmQLQ4JWv88UU+vtb5z787dj9aX1\n\tJttWXLXI9cT6SxOzjfp2jV1l8TckV9xKxTLaSpKLl0owOJy2uHixjwTKLO104tlVCQnu\n\tidoA==","X-Gm-Message-State":"AOAM530sNZeVviPAB8PuHCpQHR5tFuvMrJw3gbX/WoUU7Y9Y5+hJ4RZp\n\tCceIxiGYG7iqu9SbitmPass9XCfojWhqaa9xGp/12uvDBV4=","X-Google-Smtp-Source":"ABdhPJxiMPg0AyLeL/n/wbZj+jkdsN4EZcwQo1nygt2md0uA//zYsOmY1A2VRr4ZTgnQua0MMBXP6b8oRdJl7LK3iP0=","X-Received":"by 2002:a17:906:5801:: with SMTP id\n\tm1mr22634003ejq.296.1635164966385; \n\tMon, 25 Oct 2021 05:29:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-8-umang.jain@ideasonboard.com>","In-Reply-To":"<20211023103302.152769-8-umang.jain@ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Mon, 25 Oct 2021 21:29:15 +0900","Message-ID":"<CAO5uPHPdG2WsR7JRqZU_B0rpgp71X2eS2KQrv5puNEby4d1wmA@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20440,"web_url":"https://patchwork.libcamera.org/comment/20440/","msgid":"<2f6cb003-e183-39d9-8038-b15b35d14bc9@ideasonboard.com>","date":"2021-10-25T13:48:47","subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Hiro,\n\nOn 10/25/21 5:59 PM, Hirokazu Honda wrote:\n> Hi Umang, thank you for the patch.\n>\n> On Sat, Oct 23, 2021 at 7:33 PM Umang Jain <umang.jain@ideasonboard.com> wrote:\n>> Introduce a dedicated worker class derived from libcamera::Thread.\n>> The worker class maintains a queue for post-processing requests\n>> and waits for a post-processing request to become available.\n>> It will process them as per FIFO before de-queuing it from the\n>> queue.\n>>\n>> The post processing streams loop in CameraDevice::requestComplete()\n>> is tweaked a bit to better suit asynchronous needs of post processing.\n>> Also, the entire iteration is locked under streamProcessMutex_ which\n>> helps us to queue all the post-processing request at once, before any\n>> of the post-processing completion slot (streamProcessingComplete())\n>> is allowed to run for post-processing requests completing in parallel.\n>> This helps us to manage both synchronous and asynchronous errors\n>> encountered during the entire post processing operation.\n>>\n>> This patch also implements a flush() for the PostProcessorWorker\n>> class which is responsible to purge post-processing requests\n>> queued up while a camera is stopping/flushing. It is hooked with\n>> CameraStream::flush(), which isn't used currently but will be\n>> used when we handle flush/stop scenarios in greater detail\n>> subsequently (in a different patchset).\n>>\n>> The libcamera request completion handler CameraDevice::requestComplete()\n>> assumes that the request that has just completed is at the front of the\n>> queue. Now that the post-processor runs asynchronously, this isn't true\n>> anymore, a request being post-processed will stay in the queue and a new\n>> libcamera request may complete. Remove that assumption, and use the\n>> request cookie to obtain the Camera3RequestDescriptor.\n>>\n>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>> ---\n>>   src/android/camera_device.cpp |  49 +++++------------\n>>   src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n>>   src/android/camera_stream.h   |  38 ++++++++++++-\n>>   3 files changed, 148 insertions(+), 40 deletions(-)\n>>\n>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>> index 3114def0..dc39467b 100644\n>> --- a/src/android/camera_device.cpp\n>> +++ b/src/android/camera_device.cpp\n>> @@ -1026,29 +1026,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n>>\n>>   void CameraDevice::requestComplete(Request *request)\n>>   {\n>> -       Camera3RequestDescriptor *descriptor;\n>> -       {\n>> -               MutexLocker descriptorsLock(descriptorsMutex_);\n>> -               ASSERT(!descriptors_.empty());\n>> -               descriptor = descriptors_.front().get();\n>> -       }\n>> -\n>> -       if (descriptor->request_->cookie() != request->cookie()) {\n>> -               /*\n>> -                * \\todo Clarify if the Camera has to be closed on\n>> -                * ERROR_DEVICE.\n>> -                */\n>> -               LOG(HAL, Error)\n>> -                       << \"Out-of-order completion for request \"\n>> -                       << utils::hex(request->cookie());\n>> -\n>> -               MutexLocker descriptorsLock(descriptorsMutex_);\n>> -               descriptors_.pop();\n>> -\n>> -               notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n>> -\n>> -               return;\n>> -       }\n>> +       Camera3RequestDescriptor *descriptor =\n>> +               reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());\n>>\n>>          /*\n>>           * Prepare the capture result for the Android camera stack.\n>> @@ -1124,12 +1103,16 @@ void CameraDevice::requestComplete(Request *request)\n>>\n>>          /* Handle post-processing. */\n>>          bool needPostProcessing = false;\n>> +       MutexLocker locker(descriptor->streamsProcessMutex_);\n>> +\n>>          /*\n>> -        * \\todo Protect the loop below with streamProcessMutex_ when post\n>> -        * processor runs asynchronously.\n>> +        * Queue all the post-processing streams request at once. The completion\n>> +        * slot streamProcessingComplete() can only execute when we are out\n>> +        * this critical section. This helps to handle synchronous errors here\n>> +        * itself.\n>>           */\n>>          auto iter = descriptor->pendingStreamsToProcess_.begin();\n>> -       while (descriptor->pendingStreamsToProcess_.size() > 0) {\n>> +       while (iter != descriptor->pendingStreamsToProcess_.end()) {\n>>                  CameraStream *stream = iter->first;\n>>                  Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n>>                  needPostProcessing = true;\n>> @@ -1151,18 +1134,16 @@ void CameraDevice::requestComplete(Request *request)\n>>          }\n>>\n>>          if (needPostProcessing) {\n>> -               /*\n>> -                * \\todo We will require to check if we failed to queue\n>> -                * post-processing requests when we migrate to post-processor\n>> -                * running asynchronously.\n>> -                *\n>> -                * if (descriptor->pendingStreamsToProcess_.size() == 0)\n>> -                *      completeDescriptor(descriptor);\n>> -                */\n>> +               if (descriptor->pendingStreamsToProcess_.size() == 0) {\n>> +                       locker.unlock();\n>> +                       completeDescriptor(descriptor);\n>> +               }\n> As Laurent suggested in other patch, I think the code would be cleaner\n> if we remove needPostProcessing.\n>\n>>                  return;\n>>          }\n>>\n>> +       locker.unlock();\n>> +\n>>          completeDescriptor(descriptor);\n>>   }\n>>\n>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n>> index 45d0607d..68b916e9 100644\n>> --- a/src/android/camera_stream.cpp\n>> +++ b/src/android/camera_stream.cpp\n>> @@ -99,6 +99,7 @@ int CameraStream::configure()\n>>                  if (ret)\n>>                          return ret;\n>>\n>> +               worker_ = std::make_unique<PostProcessorWorker>(postProcessor_.get());\n>>                  postProcessor_->processComplete.connect(\n>>                          this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n>>                                    PostProcessor::Status status) {\n>> @@ -112,6 +113,8 @@ int CameraStream::configure()\n>>                                  cameraDevice_->streamProcessingComplete(streamBuffer,\n>>                                                                          bufferStatus);\n>>                          });\n>> +\n>> +               worker_->start();\n>>          }\n>>\n>>          if (type_ == Type::Internal) {\n>> @@ -179,10 +182,6 @@ int CameraStream::process(const FrameBuffer &source,\n>>\n>>          ASSERT(type_ != Type::Direct);\n>>\n>> -       /*\n>> -        * \\todo Buffer mapping and processing should be moved to a\n>> -        * separate thread.\n>> -        */\n>>          const StreamConfiguration &output = configuration();\n>>          dest.destBuffer = std::make_unique<CameraBuffer>(\n>>                  *dest.camera3Buffer, output.pixelFormat, output.size,\n>> @@ -194,11 +193,19 @@ int CameraStream::process(const FrameBuffer &source,\n>>\n>>          dest.srcBuffer = &source;\n>>\n>> -       postProcessor_->process(&dest);\n>> +       worker_->queueRequest(&dest);\n>>\n>>          return 0;\n>>   }\n>>\n>> +void CameraStream::flush()\n>> +{\n>> +       if (!postProcessor_)\n>> +               return;\n>> +\n>> +       worker_->flush();\n>> +}\n>> +\n>>   FrameBuffer *CameraStream::getBuffer()\n>>   {\n>>          if (!allocator_)\n>> @@ -226,3 +233,87 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n>>\n>>          buffers_.push_back(buffer);\n>>   }\n>> +\n>> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n>> +       : postProcessor_(postProcessor)\n>> +{\n>> +}\n>> +\n>> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n>> +{\n>> +       {\n>> +               libcamera::MutexLocker lock(mutex_);\n>> +               state_ = State::Stopped;\n>> +       }\n>> +\n>> +       cv_.notify_one();\n>> +       wait();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::start()\n>> +{\n>> +       {\n>> +               libcamera::MutexLocker lock(mutex_);\n>> +               ASSERT(state_ != State::Running);\n>> +               state_ = State::Running;\n>> +       }\n>> +\n>> +       Thread::start();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n>> +{\n>> +       {\n>> +               MutexLocker lock(mutex_);\n>> +               ASSERT(state_ == State::Running);\n>> +               requests_.push(dest);\n>> +       }\n>> +\n>> +       cv_.notify_one();\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::run()\n>> +{\n>> +       MutexLocker locker(mutex_);\n>> +\n>> +       while (1) {\n>> +               cv_.wait(locker, [&] {\n>> +                       return state_ != State::Running || !requests_.empty();\n>> +               });\n>> +\n>> +               if (state_ != State::Running)\n>> +                       break;\n>> +\n>> +               Camera3RequestDescriptor::StreamBuffer *streamBuffer = requests_.front();\n>> +               requests_.pop();\n>> +               locker.unlock();\n>> +\n>> +               postProcessor_->process(streamBuffer);\n>> +\n>> +               locker.lock();\n>> +       }\n>> +\n>> +       if (state_ == State::Flushing) {\n>> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests =\n>> +                       std::move(requests_);\n> Why is moving requests_ to a local variable necessary?\n\n\nTo avoid iterating under a lock.\n\n\n>\n> Reviewed-by: Hirokazu Honda<hiroh@chromium.org>\n>> +               locker.unlock();\n>> +\n>> +               while (!requests.empty()) {\n>> +                       postProcessor_->processComplete.emit(\n>> +                               requests.front(), PostProcessor::Status::Error);\n>> +                       requests.pop();\n>> +               }\n>> +\n>> +               locker.lock();\n>> +               state_ = State::Stopped;\n>> +       }\n>> +}\n>> +\n>> +void CameraStream::PostProcessorWorker::flush()\n>> +{\n>> +       libcamera::MutexLocker lock(mutex_);\n>> +       state_ = State::Flushing;\n>> +       lock.unlock();\n>> +\n>> +       cv_.notify_one();\n>> +}\n>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n>> index e9c320b1..7c6e887c 100644\n>> --- a/src/android/camera_stream.h\n>> +++ b/src/android/camera_stream.h\n>> @@ -7,12 +7,16 @@\n>>   #ifndef __ANDROID_CAMERA_STREAM_H__\n>>   #define __ANDROID_CAMERA_STREAM_H__\n>>\n>> +#include <condition_variable>\n>>   #include <memory>\n>>   #include <mutex>\n>> +#include <queue>\n>>   #include <vector>\n>>\n>>   #include <hardware/camera3.h>\n>>\n>> +#include <libcamera/base/thread.h>\n>> +\n>>   #include <libcamera/camera.h>\n>>   #include <libcamera/framebuffer.h>\n>>   #include <libcamera/framebuffer_allocator.h>\n>> @@ -20,9 +24,9 @@\n>>   #include <libcamera/pixel_format.h>\n>>\n>>   #include \"camera_request.h\"\n>> +#include \"post_processor.h\"\n>>\n>>   class CameraDevice;\n>> -class PostProcessor;\n>>\n>>   class CameraStream\n>>   {\n>> @@ -125,8 +129,38 @@ public:\n>>                      Camera3RequestDescriptor::StreamBuffer &dest);\n>>          libcamera::FrameBuffer *getBuffer();\n>>          void putBuffer(libcamera::FrameBuffer *buffer);\n>> +       void flush();\n>>\n>>   private:\n>> +       class PostProcessorWorker : public libcamera::Thread\n>> +       {\n>> +       public:\n>> +               enum class State {\n>> +                       Stopped,\n>> +                       Running,\n>> +                       Flushing,\n>> +               };\n>> +\n>> +               PostProcessorWorker(PostProcessor *postProcessor);\n>> +               ~PostProcessorWorker();\n>> +\n>> +               void start();\n>> +               void queueRequest(Camera3RequestDescriptor::StreamBuffer *request);\n>> +               void flush();\n>> +\n>> +       protected:\n>> +               void run() override;\n>> +\n>> +       private:\n>> +               PostProcessor *postProcessor_;\n>> +\n>> +               libcamera::Mutex mutex_;\n>> +               std::condition_variable cv_;\n>> +\n>> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests_;\n>> +               State state_ = State::Stopped;\n>> +       };\n>> +\n>>          int waitFence(int fence);\n>>\n>>          CameraDevice *const cameraDevice_;\n>> @@ -143,6 +177,8 @@ private:\n>>           */\n>>          std::unique_ptr<std::mutex> mutex_;\n>>          std::unique_ptr<PostProcessor> postProcessor_;\n>> +\n>> +       std::unique_ptr<PostProcessorWorker> worker_;\n>>   };\n>>\n>>   #endif /* __ANDROID_CAMERA_STREAM__ */\n>> --\n>> 2.31.1\n>>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 88BB0BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 25 Oct 2021 13:48:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F351664873;\n\tMon, 25 Oct 2021 15:48: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 D05D160126\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 15:48:51 +0200 (CEST)","from [192.168.1.106] (unknown [103.251.226.211])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 00BCCE0A;\n\tMon, 25 Oct 2021 15:48:50 +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=\"oQEJrdK7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1635169731;\n\tbh=dA9AIMkT90NAW/OIk+T+nlSCfasgVu4rJDj2vxUS26k=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=oQEJrdK7njOXU/1R83/gTOc+D1QsKz2OXaeCQSQ3hRGIGuKaql5MiVvIz6SoSBwq/\n\tm3whvkv0YOcThNXNJAXTTzKxk0+y6HqZ8I9zWw3mC1alBRLpVodnsjiqSfJiQS4BqR\n\tdDFU7Y6SSIW9qclzI4Ja+2/wHyM8hsATK/JdHpdI=","To":"Hirokazu Honda <hiroh@chromium.org>","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-8-umang.jain@ideasonboard.com>\n\t<CAO5uPHPdG2WsR7JRqZU_B0rpgp71X2eS2KQrv5puNEby4d1wmA@mail.gmail.com>","From":"Umang Jain <umang.jain@ideasonboard.com>","Message-ID":"<2f6cb003-e183-39d9-8038-b15b35d14bc9@ideasonboard.com>","Date":"Mon, 25 Oct 2021 19:18: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":"<CAO5uPHPdG2WsR7JRqZU_B0rpgp71X2eS2KQrv5puNEby4d1wmA@mail.gmail.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":20442,"web_url":"https://patchwork.libcamera.org/comment/20442/","msgid":"<CAO5uPHMQ7NZ_N8nPz0s_zQG60EQfzvJcZNuS+D7oQy1EWztnAA@mail.gmail.com>","date":"2021-10-25T13:52:16","subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Umang,\n\nOn Mon, Oct 25, 2021 at 10:48 PM Umang Jain <umang.jain@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> On 10/25/21 5:59 PM, Hirokazu Honda wrote:\n> > Hi Umang, thank you for the patch.\n> >\n> > On Sat, Oct 23, 2021 at 7:33 PM Umang Jain <umang.jain@ideasonboard.com> wrote:\n> >> Introduce a dedicated worker class derived from libcamera::Thread.\n> >> The worker class maintains a queue for post-processing requests\n> >> and waits for a post-processing request to become available.\n> >> It will process them as per FIFO before de-queuing it from the\n> >> queue.\n> >>\n> >> The post processing streams loop in CameraDevice::requestComplete()\n> >> is tweaked a bit to better suit asynchronous needs of post processing.\n> >> Also, the entire iteration is locked under streamProcessMutex_ which\n> >> helps us to queue all the post-processing request at once, before any\n> >> of the post-processing completion slot (streamProcessingComplete())\n> >> is allowed to run for post-processing requests completing in parallel.\n> >> This helps us to manage both synchronous and asynchronous errors\n> >> encountered during the entire post processing operation.\n> >>\n> >> This patch also implements a flush() for the PostProcessorWorker\n> >> class which is responsible to purge post-processing requests\n> >> queued up while a camera is stopping/flushing. It is hooked with\n> >> CameraStream::flush(), which isn't used currently but will be\n> >> used when we handle flush/stop scenarios in greater detail\n> >> subsequently (in a different patchset).\n> >>\n> >> The libcamera request completion handler CameraDevice::requestComplete()\n> >> assumes that the request that has just completed is at the front of the\n> >> queue. Now that the post-processor runs asynchronously, this isn't true\n> >> anymore, a request being post-processed will stay in the queue and a new\n> >> libcamera request may complete. Remove that assumption, and use the\n> >> request cookie to obtain the Camera3RequestDescriptor.\n> >>\n> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>\n> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >> ---\n> >>   src/android/camera_device.cpp |  49 +++++------------\n> >>   src/android/camera_stream.cpp | 101 ++++++++++++++++++++++++++++++++--\n> >>   src/android/camera_stream.h   |  38 ++++++++++++-\n> >>   3 files changed, 148 insertions(+), 40 deletions(-)\n> >>\n> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> >> index 3114def0..dc39467b 100644\n> >> --- a/src/android/camera_device.cpp\n> >> +++ b/src/android/camera_device.cpp\n> >> @@ -1026,29 +1026,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> >>\n> >>   void CameraDevice::requestComplete(Request *request)\n> >>   {\n> >> -       Camera3RequestDescriptor *descriptor;\n> >> -       {\n> >> -               MutexLocker descriptorsLock(descriptorsMutex_);\n> >> -               ASSERT(!descriptors_.empty());\n> >> -               descriptor = descriptors_.front().get();\n> >> -       }\n> >> -\n> >> -       if (descriptor->request_->cookie() != request->cookie()) {\n> >> -               /*\n> >> -                * \\todo Clarify if the Camera has to be closed on\n> >> -                * ERROR_DEVICE.\n> >> -                */\n> >> -               LOG(HAL, Error)\n> >> -                       << \"Out-of-order completion for request \"\n> >> -                       << utils::hex(request->cookie());\n> >> -\n> >> -               MutexLocker descriptorsLock(descriptorsMutex_);\n> >> -               descriptors_.pop();\n> >> -\n> >> -               notifyError(0, nullptr, CAMERA3_MSG_ERROR_DEVICE);\n> >> -\n> >> -               return;\n> >> -       }\n> >> +       Camera3RequestDescriptor *descriptor =\n> >> +               reinterpret_cast<Camera3RequestDescriptor *>(request->cookie());\n> >>\n> >>          /*\n> >>           * Prepare the capture result for the Android camera stack.\n> >> @@ -1124,12 +1103,16 @@ void CameraDevice::requestComplete(Request *request)\n> >>\n> >>          /* Handle post-processing. */\n> >>          bool needPostProcessing = false;\n> >> +       MutexLocker locker(descriptor->streamsProcessMutex_);\n> >> +\n> >>          /*\n> >> -        * \\todo Protect the loop below with streamProcessMutex_ when post\n> >> -        * processor runs asynchronously.\n> >> +        * Queue all the post-processing streams request at once. The completion\n> >> +        * slot streamProcessingComplete() can only execute when we are out\n> >> +        * this critical section. This helps to handle synchronous errors here\n> >> +        * itself.\n> >>           */\n> >>          auto iter = descriptor->pendingStreamsToProcess_.begin();\n> >> -       while (descriptor->pendingStreamsToProcess_.size() > 0) {\n> >> +       while (iter != descriptor->pendingStreamsToProcess_.end()) {\n> >>                  CameraStream *stream = iter->first;\n> >>                  Camera3RequestDescriptor::StreamBuffer *buffer = iter->second;\n> >>                  needPostProcessing = true;\n> >> @@ -1151,18 +1134,16 @@ void CameraDevice::requestComplete(Request *request)\n> >>          }\n> >>\n> >>          if (needPostProcessing) {\n> >> -               /*\n> >> -                * \\todo We will require to check if we failed to queue\n> >> -                * post-processing requests when we migrate to post-processor\n> >> -                * running asynchronously.\n> >> -                *\n> >> -                * if (descriptor->pendingStreamsToProcess_.size() == 0)\n> >> -                *      completeDescriptor(descriptor);\n> >> -                */\n> >> +               if (descriptor->pendingStreamsToProcess_.size() == 0) {\n> >> +                       locker.unlock();\n> >> +                       completeDescriptor(descriptor);\n> >> +               }\n> > As Laurent suggested in other patch, I think the code would be cleaner\n> > if we remove needPostProcessing.\n> >\n> >>                  return;\n> >>          }\n> >>\n> >> +       locker.unlock();\n> >> +\n> >>          completeDescriptor(descriptor);\n> >>   }\n> >>\n> >> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp\n> >> index 45d0607d..68b916e9 100644\n> >> --- a/src/android/camera_stream.cpp\n> >> +++ b/src/android/camera_stream.cpp\n> >> @@ -99,6 +99,7 @@ int CameraStream::configure()\n> >>                  if (ret)\n> >>                          return ret;\n> >>\n> >> +               worker_ = std::make_unique<PostProcessorWorker>(postProcessor_.get());\n> >>                  postProcessor_->processComplete.connect(\n> >>                          this, [&](Camera3RequestDescriptor::StreamBuffer *streamBuffer,\n> >>                                    PostProcessor::Status status) {\n> >> @@ -112,6 +113,8 @@ int CameraStream::configure()\n> >>                                  cameraDevice_->streamProcessingComplete(streamBuffer,\n> >>                                                                          bufferStatus);\n> >>                          });\n> >> +\n> >> +               worker_->start();\n> >>          }\n> >>\n> >>          if (type_ == Type::Internal) {\n> >> @@ -179,10 +182,6 @@ int CameraStream::process(const FrameBuffer &source,\n> >>\n> >>          ASSERT(type_ != Type::Direct);\n> >>\n> >> -       /*\n> >> -        * \\todo Buffer mapping and processing should be moved to a\n> >> -        * separate thread.\n> >> -        */\n> >>          const StreamConfiguration &output = configuration();\n> >>          dest.destBuffer = std::make_unique<CameraBuffer>(\n> >>                  *dest.camera3Buffer, output.pixelFormat, output.size,\n> >> @@ -194,11 +193,19 @@ int CameraStream::process(const FrameBuffer &source,\n> >>\n> >>          dest.srcBuffer = &source;\n> >>\n> >> -       postProcessor_->process(&dest);\n> >> +       worker_->queueRequest(&dest);\n> >>\n> >>          return 0;\n> >>   }\n> >>\n> >> +void CameraStream::flush()\n> >> +{\n> >> +       if (!postProcessor_)\n> >> +               return;\n> >> +\n> >> +       worker_->flush();\n> >> +}\n> >> +\n> >>   FrameBuffer *CameraStream::getBuffer()\n> >>   {\n> >>          if (!allocator_)\n> >> @@ -226,3 +233,87 @@ void CameraStream::putBuffer(FrameBuffer *buffer)\n> >>\n> >>          buffers_.push_back(buffer);\n> >>   }\n> >> +\n> >> +CameraStream::PostProcessorWorker::PostProcessorWorker(PostProcessor *postProcessor)\n> >> +       : postProcessor_(postProcessor)\n> >> +{\n> >> +}\n> >> +\n> >> +CameraStream::PostProcessorWorker::~PostProcessorWorker()\n> >> +{\n> >> +       {\n> >> +               libcamera::MutexLocker lock(mutex_);\n> >> +               state_ = State::Stopped;\n> >> +       }\n> >> +\n> >> +       cv_.notify_one();\n> >> +       wait();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::start()\n> >> +{\n> >> +       {\n> >> +               libcamera::MutexLocker lock(mutex_);\n> >> +               ASSERT(state_ != State::Running);\n> >> +               state_ = State::Running;\n> >> +       }\n> >> +\n> >> +       Thread::start();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::queueRequest(Camera3RequestDescriptor::StreamBuffer *dest)\n> >> +{\n> >> +       {\n> >> +               MutexLocker lock(mutex_);\n> >> +               ASSERT(state_ == State::Running);\n> >> +               requests_.push(dest);\n> >> +       }\n> >> +\n> >> +       cv_.notify_one();\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::run()\n> >> +{\n> >> +       MutexLocker locker(mutex_);\n> >> +\n> >> +       while (1) {\n> >> +               cv_.wait(locker, [&] {\n> >> +                       return state_ != State::Running || !requests_.empty();\n> >> +               });\n> >> +\n> >> +               if (state_ != State::Running)\n> >> +                       break;\n> >> +\n> >> +               Camera3RequestDescriptor::StreamBuffer *streamBuffer = requests_.front();\n> >> +               requests_.pop();\n> >> +               locker.unlock();\n> >> +\n> >> +               postProcessor_->process(streamBuffer);\n> >> +\n> >> +               locker.lock();\n> >> +       }\n> >> +\n> >> +       if (state_ == State::Flushing) {\n> >> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests =\n> >> +                       std::move(requests_);\n> > Why is moving requests_ to a local variable necessary?\n>\n>\n> To avoid iterating under a lock.\n>\n\nI got it. Thanks.\n\n>\n> >\n> > Reviewed-by: Hirokazu Honda<hiroh@chromium.org>\n> >> +               locker.unlock();\n> >> +\n> >> +               while (!requests.empty()) {\n> >> +                       postProcessor_->processComplete.emit(\n> >> +                               requests.front(), PostProcessor::Status::Error);\n> >> +                       requests.pop();\n> >> +               }\n> >> +\n> >> +               locker.lock();\n> >> +               state_ = State::Stopped;\n> >> +       }\n> >> +}\n> >> +\n> >> +void CameraStream::PostProcessorWorker::flush()\n> >> +{\n> >> +       libcamera::MutexLocker lock(mutex_);\n> >> +       state_ = State::Flushing;\n> >> +       lock.unlock();\n> >> +\n> >> +       cv_.notify_one();\n> >> +}\n> >> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h\n> >> index e9c320b1..7c6e887c 100644\n> >> --- a/src/android/camera_stream.h\n> >> +++ b/src/android/camera_stream.h\n> >> @@ -7,12 +7,16 @@\n> >>   #ifndef __ANDROID_CAMERA_STREAM_H__\n> >>   #define __ANDROID_CAMERA_STREAM_H__\n> >>\n> >> +#include <condition_variable>\n> >>   #include <memory>\n> >>   #include <mutex>\n> >> +#include <queue>\n> >>   #include <vector>\n> >>\n> >>   #include <hardware/camera3.h>\n> >>\n> >> +#include <libcamera/base/thread.h>\n> >> +\n> >>   #include <libcamera/camera.h>\n> >>   #include <libcamera/framebuffer.h>\n> >>   #include <libcamera/framebuffer_allocator.h>\n> >> @@ -20,9 +24,9 @@\n> >>   #include <libcamera/pixel_format.h>\n> >>\n> >>   #include \"camera_request.h\"\n> >> +#include \"post_processor.h\"\n> >>\n> >>   class CameraDevice;\n> >> -class PostProcessor;\n> >>\n> >>   class CameraStream\n> >>   {\n> >> @@ -125,8 +129,38 @@ public:\n> >>                      Camera3RequestDescriptor::StreamBuffer &dest);\n> >>          libcamera::FrameBuffer *getBuffer();\n> >>          void putBuffer(libcamera::FrameBuffer *buffer);\n> >> +       void flush();\n> >>\n> >>   private:\n> >> +       class PostProcessorWorker : public libcamera::Thread\n> >> +       {\n> >> +       public:\n> >> +               enum class State {\n> >> +                       Stopped,\n> >> +                       Running,\n> >> +                       Flushing,\n> >> +               };\n> >> +\n> >> +               PostProcessorWorker(PostProcessor *postProcessor);\n> >> +               ~PostProcessorWorker();\n> >> +\n> >> +               void start();\n> >> +               void queueRequest(Camera3RequestDescriptor::StreamBuffer *request);\n> >> +               void flush();\n> >> +\n> >> +       protected:\n> >> +               void run() override;\n> >> +\n> >> +       private:\n> >> +               PostProcessor *postProcessor_;\n> >> +\n> >> +               libcamera::Mutex mutex_;\n> >> +               std::condition_variable cv_;\n> >> +\n> >> +               std::queue<Camera3RequestDescriptor::StreamBuffer *> requests_;\n> >> +               State state_ = State::Stopped;\n> >> +       };\n> >> +\n> >>          int waitFence(int fence);\n> >>\n> >>          CameraDevice *const cameraDevice_;\n> >> @@ -143,6 +177,8 @@ private:\n> >>           */\n> >>          std::unique_ptr<std::mutex> mutex_;\n> >>          std::unique_ptr<PostProcessor> postProcessor_;\n> >> +\n> >> +       std::unique_ptr<PostProcessorWorker> worker_;\n> >>   };\n> >>\n> >>   #endif /* __ANDROID_CAMERA_STREAM__ */\n> >> --\n> >> 2.31.1\n> >>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 C5251BDB1C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 25 Oct 2021 13:55:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C86464873;\n\tMon, 25 Oct 2021 15:55:07 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1FA660126\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 15:55:05 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id r4so18524388edi.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 25 Oct 2021 06:55:05 -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=\"CthF3Uv4\"; 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=FThyHP85HihxhIb6LkUmDVyJ4ZmYf3l5xdiNkui26TY=;\n\tb=CthF3Uv40itAqfclgt/KqSENei1upZV7ji4s04ANexBlrBc7bzdOUM5Kq3sYKSK/p4\n\tOl4zcWWiTlDgqF648PHxiI+oeArCwR0YqYq9bXZb2beBrGM6Y/dLmTaXFCLSsjqHdn1F\n\tIeXPGW8ni3E+s7LVno9cRr6YSz628Vw2adNpw=","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=FThyHP85HihxhIb6LkUmDVyJ4ZmYf3l5xdiNkui26TY=;\n\tb=W8fKqwglWlXOuGjiOy/N+NSWgaM1IYRGrAdg4gq+CEE4vGauc1HExSwrWq4j//u55P\n\tTFt6+fEQhgx9F87xseXhVi+DChfXcTaABoyAafnKKgluJbwwcP+M8VnjggXfLNVEa9gu\n\tvf7yaau6q+WZ+8jf6729ke2K6n9JaTDrWnCKoaTLf4TddGUsMOtF86uXLadTYyouw9vl\n\tsQO0nCgjx+xA1wNT2yxw0+AUhwZlxNZV55xm4RY+0i994CBm0MhAQVZJdbHi6bl32z5/\n\tqOkSfeH3Izn28QxCVOcDBRz4B9a1PJl6ttPtIZBX3rdHPUpMxpr2WQoPqRLfV+jexwVU\n\tEAuA==","X-Gm-Message-State":"AOAM532m3Juk8JBRebOXHAsNf/xydt9hOvzMec6yWVreLN4NYYxkbifA\n\tGwCa5E+oU4nDeAGIzsDNmzRpLzfSWwcGjzEWnerhyRAKe9Q=","X-Google-Smtp-Source":"ABdhPJzEKTOPsBJgCqlhY481kvn4lPgkOyV2jpVV4+O7/ROnzLgVz0Jey62skUJBBdKMBNsWg6t4dKggtXM0V6kGAz8=","X-Received":"by 2002:a05:6402:42ce:: with SMTP id\n\ti14mr934978edc.276.1635169948078; \n\tMon, 25 Oct 2021 06:52:28 -0700 (PDT)","MIME-Version":"1.0","References":"<20211023103302.152769-1-umang.jain@ideasonboard.com>\n\t<20211023103302.152769-8-umang.jain@ideasonboard.com>\n\t<CAO5uPHPdG2WsR7JRqZU_B0rpgp71X2eS2KQrv5puNEby4d1wmA@mail.gmail.com>\n\t<2f6cb003-e183-39d9-8038-b15b35d14bc9@ideasonboard.com>","In-Reply-To":"<2f6cb003-e183-39d9-8038-b15b35d14bc9@ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Mon, 25 Oct 2021 22:52:16 +0900","Message-ID":"<CAO5uPHMQ7NZ_N8nPz0s_zQG60EQfzvJcZNuS+D7oQy1EWztnAA@mail.gmail.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 7/7] android: post_processor: Make\n\tpost processing async","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]