[{"id":17339,"web_url":"https://patchwork.libcamera.org/comment/17339/","msgid":"","date":"2021-05-28T07:33:13","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2021-05-28 00:03:59 +0200, Jacopo Mondi wrote:\n> Implement the flush() camera operation in the CameraDevice class\n> and make it available to the camera framework by implementing the\n> operation wrapper in camera_ops.cpp.\n> \n> Introduce a new camera state State::Flushing to handle concurrent\n> flush() and process_capture_request() calls.\n> \n> As flush() can race with processCaptureRequest() protect it\n> by introducing a new State::Flushing state that\n> processCaptureRequest() inspects before queuing the Request to the\n> Camera. If flush() is in progress while processCaptureRequest() is\n> called, return the current Request immediately in error state. If\n> flush() has completed and a new call to processCaptureRequest() is\n> made just after, start the camera again before queuing the request.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> src/android/camera_device.h | 3 ++\n> src/android/camera_ops.cpp | 8 +++-\n> 3 files changed, 83 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -797,6 +797,23 @@ void CameraDevice::close()\n> \tcamera_->release();\n> }\n> \n> +void CameraDevice::flush()\n> +{\n> +\t{\n> +\t\tMutexLocker stateLock(stateMutex_);\n> +\t\tif (state_ != State::Running)\n> +\t\t\treturn;\n> +\n> +\t\tstate_ = State::Flushing;\n> +\t}\n> +\n> +\tworker_.stop();\n> +\tcamera_->stop();\n> +\n> +\tMutexLocker stateLock(stateMutex_);\n> +\tstate_ = State::Stopped;\n> +}\n> +\n> void CameraDevice::stop()\n> {\n> \tMutexLocker stateLock(stateMutex_);\n> @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> \treturn 0;\n> }\n> \n> +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> +{\n> +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> +\n> +\tcamera3_capture_result_t result = {};\n> +\tresult.num_output_buffers = request->num_output_buffers;\n> +\tresult.frame_number = request->frame_number;\n> +\tresult.partial_result = 0;\n> +\n> +\tstd::vector resultBuffers(result.num_output_buffers);\n> +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> +\t\tbuffer = request->output_buffers[i];\n> +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t}\n> +\tresult.output_buffers = resultBuffers.data();\n> +\n> +\tcallbacks_->process_capture_result(callbacks_, &result);\n> +}\n> +\n> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> {\n> \tif (!isValidRequest(camera3Request))\n> \t\treturn -EINVAL;\n> \n> \t{\n> +\t\t/*\n> +\t\t * Start the camera if that's the first request we handle after\n> +\t\t * a configuration or after a flush.\n> +\t\t *\n> +\t\t * If flush is in progress, return the pending request\n> +\t\t * immediately in error state.\n> +\t\t */\n> \t\tMutexLocker stateLock(stateMutex_);\n> +\t\tif (state_ == State::Flushing) {\n> +\t\t\tabortRequest(camera3Request);\n> +\t\t\treturn 0;\n> +\t\t}\n> \n> -\t\t/* Start the camera if that's the first request we handle. */\n> \t\tif (state_ == State::Stopped) {\n> \t\t\tworker_.start();\n> \n> @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/*\n> +\t * Just before queuing the request, make sure flush() has not\n> +\t * been called while this function was running. If flush is in progress\n> +\t * abort the request. If flush has completed and has stopped the camera\n> +\t * we have to re-start it to be able to process the request.\n> +\t */\n> +\tMutexLocker stateLock(stateMutex_);\n> +\tif (state_ == State::Flushing) {\n> +\t\tabortRequest(camera3Request);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (state_ == State::Stopped) {\n> +\t\tworker_.start();\n> +\n> +\t\tret = camera_->start();\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tstate_ = State::Running;\n> +\t}\n> +\n> \tworker_.queueRequest(descriptor.request_.get());\n> \n> \t{\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index c949fa509ca4..4aadb27c562c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -43,6 +43,7 @@ public:\n> \n> \tint open(const hw_module_t *hardwareModule);\n> \tvoid close();\n> +\tvoid flush();\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> @@ -92,6 +93,7 @@ private:\n> \n> \tenum class State {\n> \t\tStopped,\n> +\t\tFlushing,\n> \t\tRunning,\n> \t};\n> \n> @@ -106,6 +108,7 @@ private:\n> \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> \n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> +\tvoid abortRequest(camera3_capture_request_t *request);\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> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e80436821..8a3cfa175ff5 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> {\n> }\n> \n> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> +static int hal_dev_flush(const struct camera3_device *dev)\n> {\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> +\tcamera->flush();\n> +\n> \treturn 0;\n> }\n> \n> -- \n> 2.31.1\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 D9938C3206\n\tfor ;\n\tFri, 28 May 2021 07:33:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F3AA268924;\n\tFri, 28 May 2021 09:33:16 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B16F6891E\n\tfor ;\n\tFri, 28 May 2021 09:33:15 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id a5so4033677lfm.0\n\tfor ;\n\tFri, 28 May 2021 00:33:15 -0700 (PDT)","from localhost (h-62-63-236-217.A463.priv.bahnhof.se.\n\t[62.63.236.217]) by smtp.gmail.com with ESMTPSA id\n\to21sm399837lfg.147.2021.05.28.00.33.13\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 28 May 2021 00:33:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"M7TvoWCJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=48WGoLZFu7h987oIlP/gJpyPThSDFmw0/kmBOrOYbms=;\n\tb=M7TvoWCJLHqeFLJldeS9svEnXxh0DQucv6HcQ2+FglPmZdDCD47FFOixTe1i5WX0Hv\n\tM1no8gk0iNQ1UvXf4AlbCsxwTDHZkqtvosM9EWO8Wg5fAq2/s/iv0yOYdgV8WbRS8aob\n\tvThi5bSiYE6/JztadGASEET5UebACehkh3eVjArHXiE0usOS9SOLwQQCK3JxzSRyZvvQ\n\tETdrzb9EQySJYEvt6B+A2VGlKnCCJk9+ZJNULGxfI3Esz92EhhmttspRQZi/r5VSQYjp\n\ti/k3VcTIAq3QcS7pU0LUZPc23iXKCqKjBnYtLzQBGf9J0ZnTmdzjixSsl1kRSL22apSX\n\tdXNQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=48WGoLZFu7h987oIlP/gJpyPThSDFmw0/kmBOrOYbms=;\n\tb=qpmq5D4CnXswVjJBwNiUc23bk87WsKk2CzZ0+SksvcfVH5uBaXEnvmbDsGQpiSmw9V\n\tCAgcXJjsiCT6nFWO7vwD7kunKN+aG2/p+8KkhZjmLOtg93ZcAyC4hTna0C3wnsfPwNEN\n\tbc1xHs19uRMT+zwJ+FsmhiQhyTTr4+wvaSgWZmdVuOLTG7zukHwrxWOhX+LG4gRr/Z2X\n\t4a0oEXLUt7o1C9HepGWHvDDUTLSR3ozOa0nGGWBdu5V73QPCmzIYOJVZ6b0cVbnbQkMB\n\tt3nr9AZG25WHBeitxnRk+fHupW/MyaKbKuqTYItN5Z8PiZodOiG1PYG6JsMsESGcN8h8\n\t79dg==","X-Gm-Message-State":"AOAM53236UymQhtNt9eVThmNSNi0cW6Q91BSQP3EXxFTTPwisXU1gaP5\n\tkQlSbh5zY9F53Oeo/BSfavDVafO5O0LJxg==","X-Google-Smtp-Source":"ABdhPJz9FjtGF39qL7quS5aprpT7MpkoQKU8XFLSaoukveqZH2jmUuomFEYtYUimXnJ8c1GZuQgLRA==","X-Received":"by 2002:ac2:5f6f:: with SMTP id\n\tc15mr5001243lfc.194.1622187194533; \n\tFri, 28 May 2021 00:33:14 -0700 (PDT)","Date":"Fri, 28 May 2021 09:33:13 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20210527220359.30127-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17340,"web_url":"https://patchwork.libcamera.org/comment/17340/","msgid":"","date":"2021-05-28T07:37:42","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo, thank you for the work.\n\nOn Fri, May 28, 2021 at 4:33 PM Niklas Söderlund <\nniklas.soderlund@ragnatech.se> wrote:\n\n> Hi Jacopo,\n>\n> Thanks for your work.\n>\n> On 2021-05-28 00:03:59 +0200, Jacopo Mondi wrote:\n> > Implement the flush() camera operation in the CameraDevice class\n> > and make it available to the camera framework by implementing the\n> > operation wrapper in camera_ops.cpp.\n> >\n> > Introduce a new camera state State::Flushing to handle concurrent\n> > flush() and process_capture_request() calls.\n> >\n> > As flush() can race with processCaptureRequest() protect it\n> > by introducing a new State::Flushing state that\n> > processCaptureRequest() inspects before queuing the Request to the\n> > Camera. If flush() is in progress while processCaptureRequest() is\n> > called, return the current Request immediately in error state. If\n> > flush() has completed and a new call to processCaptureRequest() is\n> > made just after, start the camera again before queuing the request.\n> >\n> > Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Niklas Söderlund \n>\n>\nReviewed-by: Hirokazu Honda \n\n\n> > ---\n> > src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> > src/android/camera_device.h | 3 ++\n> > src/android/camera_ops.cpp | 8 +++-\n> > 3 files changed, 83 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > camera_->release();\n> > }\n> >\n> > +void CameraDevice::flush()\n> > +{\n> > + {\n> > + MutexLocker stateLock(stateMutex_);\n> > + if (state_ != State::Running)\n> > + return;\n> > +\n> > + state_ = State::Flushing;\n> > + }\n> > +\n> > + worker_.stop();\n> > + camera_->stop();\n> > +\n> > + MutexLocker stateLock(stateMutex_);\n> > + state_ = State::Stopped;\n> > +}\n> > +\n> > void CameraDevice::stop()\n> > {\n> > MutexLocker stateLock(stateMutex_);\n> > @@ -1894,15 +1911,46 @@ int\n> CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > return 0;\n> > }\n> >\n> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > +{\n> > + notifyError(request->frame_number, nullptr,\n> CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > + camera3_capture_result_t result = {};\n> > + result.num_output_buffers = request->num_output_buffers;\n> > + result.frame_number = request->frame_number;\n> > + result.partial_result = 0;\n> > +\n> > + std::vector\n> resultBuffers(result.num_output_buffers);\n> > + for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > + buffer = request->output_buffers[i];\n> > + buffer.release_fence =\n> request->output_buffers[i].acquire_fence;\n> > + buffer.acquire_fence = -1;\n> > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > + }\n> > + result.output_buffers = resultBuffers.data();\n> > +\n> > + callbacks_->process_capture_result(callbacks_, &result);\n> > +}\n> > +\n> > int CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Request)\n> > {\n> > if (!isValidRequest(camera3Request))\n> > return -EINVAL;\n> >\n> > {\n> > + /*\n> > + * Start the camera if that's the first request we handle\n> after\n> > + * a configuration or after a flush.\n> > + *\n> > + * If flush is in progress, return the pending request\n> > + * immediately in error state.\n> > + */\n> > MutexLocker stateLock(stateMutex_);\n> > + if (state_ == State::Flushing) {\n> > + abortRequest(camera3Request);\n> > + return 0;\n> > + }\n> >\n> > - /* Start the camera if that's the first request we handle.\n> */\n> > if (state_ == State::Stopped) {\n> > worker_.start();\n> >\n> > @@ -2004,6 +2052,30 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > if (ret)\n> > return ret;\n> >\n> > + /*\n> > + * Just before queuing the request, make sure flush() has not\n> > + * been called while this function was running. If flush is in\n> progress\n> > + * abort the request. If flush has completed and has stopped the\n> camera\n> > + * we have to re-start it to be able to process the request.\n> > + */\n> > + MutexLocker stateLock(stateMutex_);\n> > + if (state_ == State::Flushing) {\n> > + abortRequest(camera3Request);\n> > + return 0;\n> > + }\n> > +\n> > + if (state_ == State::Stopped) {\n> > + worker_.start();\n> > +\n> > + ret = camera_->start();\n> > + if (ret) {\n> > + LOG(HAL, Error) << \"Failed to start camera\";\n> > + return ret;\n> > + }\n> > +\n> > + state_ = State::Running;\n> > + }\n> > +\n> > worker_.queueRequest(descriptor.request_.get());\n> >\n> > {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index c949fa509ca4..4aadb27c562c 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -43,6 +43,7 @@ public:\n> >\n> > int open(const hw_module_t *hardwareModule);\n> > void close();\n> > + void flush();\n> >\n> > unsigned int id() const { return id_; }\n> > camera3_device_t *camera3Device() { return &camera3Device_; }\n> > @@ -92,6 +93,7 @@ private:\n> >\n> > enum class State {\n> > Stopped,\n> > + Flushing,\n> > Running,\n> > };\n> >\n> > @@ -106,6 +108,7 @@ private:\n> > getRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> >\n> > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t\n> camera3buffer);\n> > + void abortRequest(camera3_capture_request_t *request);\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> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e80436821..8a3cfa175ff5 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> struct camera3_device *dev,\n> > {\n> > }\n> >\n> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device\n> *dev)\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > {\n> > + if (!dev)\n> > + return -EINVAL;\n> > +\n> > + CameraDevice *camera = reinterpret_cast(dev->priv);\n> > + camera->flush();\n> > +\n> > return 0;\n> > }\n> >\n> > --\n> > 2.31.1\n> >\n>\n> --\n> Regards,\n> Niklas Söderlund\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 C6772C3205\n\tfor ;\n\tFri, 28 May 2021 07:37:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4BAF16891E;\n\tFri, 28 May 2021 09:37:54 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3B0A06891E\n\tfor ;\n\tFri, 28 May 2021 09:37:53 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id e12so3958017ejt.3\n\tfor ;\n\tFri, 28 May 2021 00:37:53 -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=\"oX4cmRR9\"; 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=8Hw5HkZx2zLQtG4LFv3EMwcKkMzc5J1pCceiX5ux7iw=;\n\tb=oX4cmRR9IpwxolIr28cKBBE9hPpGtTSSBC/oEpffVsRZYDoHDSXOaF1JDdIhhN0U97\n\tfEtH36APBWJUIamiprbI2+Hvr7L6zeQM1pmSEJpvJ9IwX5knMLvtQMbmYuGnXRVuokUj\n\tfC2E0Cq1iM5/bkJHeN6w5m6kngcZP/bTe9IWI=","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=8Hw5HkZx2zLQtG4LFv3EMwcKkMzc5J1pCceiX5ux7iw=;\n\tb=JR23yXcmfpimKmCznp2vpj3nzOEBbrBjHgDxUpmWxhMmTHGo7uqwiwtc8MDemOGrFi\n\tnIGN6UJUqm9exwI/1QWddshgxREOjb29Qhsi6zRT5zgHRjamNHY2Mbu4Xw5RYgDKEERk\n\tkw+v8BIxW4bAOEhUilcKAJdyZnnDkbjetpyNjN3Hgrfy+91D4bKCetgqOx8sQ/k4eKu4\n\twrH3OHgBfIi0ROmDkjT4M2VsHt0hcwNbWaDUiYFVmMwrxJHuYlHbqopzTl+Np6xarxux\n\tp9NysmIIPpmutzr8oWZJBjPUnKnLF0kr/02uUpcRjYqa/fdPnN6oJqQeLyTzZs4qPPW2\n\t66Tg==","X-Gm-Message-State":"AOAM533jxzksdDxGavWjeJslTE4qhIeBOHosHPSL6Yj+3UnkpkDtqIE7\n\ta9yL1M9MqOrYOqhrMXs8Cb2OSduPVMKl8FDYcZRLfGiNSw0=","X-Google-Smtp-Source":"ABdhPJwnDId2lSvn0URMVByAFsJ3DnwgrSDC36jMo6Knuo4nbB7akUjUgS/4MHaL+Gpd+A/hygJX3T3d53tO/2+Gbkg=","X-Received":"by 2002:a17:906:3a04:: with SMTP id\n\tz4mr7812070eje.221.1622187472797; \n\tFri, 28 May 2021 00:37:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Fri, 28 May 2021 16:37:42 +0900","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Content-Type":"multipart/alternative; boundary=\"00000000000013594b05c35ef479\"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17341,"web_url":"https://patchwork.libcamera.org/comment/17341/","msgid":"","date":"2021-05-28T07:53:27","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> Implement the flush() camera operation in the CameraDevice class\n> and make it available to the camera framework by implementing the\n> operation wrapper in camera_ops.cpp.\n> \n> Introduce a new camera state State::Flushing to handle concurrent\n> flush() and process_capture_request() calls.\n> \n> As flush() can race with processCaptureRequest() protect it\n> by introducing a new State::Flushing state that\n> processCaptureRequest() inspects before queuing the Request to the\n> Camera. If flush() is in progress while processCaptureRequest() is\n> called, return the current Request immediately in error state. If\n> flush() has completed and a new call to processCaptureRequest() is\n> made just after, start the camera again before queuing the request.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> src/android/camera_device.h | 3 ++\n> src/android/camera_ops.cpp | 8 +++-\n> 3 files changed, 83 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -797,6 +797,23 @@ void CameraDevice::close()\n> \tcamera_->release();\n> }\n> \n> +void CameraDevice::flush()\n> +{\n> +\t{\n> +\t\tMutexLocker stateLock(stateMutex_);\n> +\t\tif (state_ != State::Running)\n> +\t\t\treturn;\n> +\n> +\t\tstate_ = State::Flushing;\n> +\t}\n> +\n> +\tworker_.stop();\n> +\tcamera_->stop();\n> +\n> +\tMutexLocker stateLock(stateMutex_);\n> +\tstate_ = State::Stopped;\n> +}\n> +\n> void CameraDevice::stop()\n> {\n> \tMutexLocker stateLock(stateMutex_);\n> @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> \treturn 0;\n> }\n> \n> +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> +{\n> +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> +\n> +\tcamera3_capture_result_t result = {};\n> +\tresult.num_output_buffers = request->num_output_buffers;\n> +\tresult.frame_number = request->frame_number;\n> +\tresult.partial_result = 0;\n> +\n> +\tstd::vector resultBuffers(result.num_output_buffers);\n> +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> +\t\tbuffer = request->output_buffers[i];\n> +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t}\n> +\tresult.output_buffers = resultBuffers.data();\n> +\n> +\tcallbacks_->process_capture_result(callbacks_, &result);\n> +}\n> +\n> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> {\n> \tif (!isValidRequest(camera3Request))\n> \t\treturn -EINVAL;\n> \n> \t{\n> +\t\t/*\n> +\t\t * Start the camera if that's the first request we handle after\n> +\t\t * a configuration or after a flush.\n> +\t\t *\n> +\t\t * If flush is in progress, return the pending request\n> +\t\t * immediately in error state.\n> +\t\t */\n> \t\tMutexLocker stateLock(stateMutex_);\n> +\t\tif (state_ == State::Flushing) {\n> +\t\t\tabortRequest(camera3Request);\n> +\t\t\treturn 0;\n> +\t\t}\n> \n> -\t\t/* Start the camera if that's the first request we handle. */\n> \t\tif (state_ == State::Stopped) {\n> \t\t\tworker_.start();\n> \n> @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> \tif (ret)\n> \t\treturn ret;\n> \n> +\t/*\n> +\t * Just before queuing the request, make sure flush() has not\n> +\t * been called while this function was running. If flush is in progress\n> +\t * abort the request. If flush has completed and has stopped the camera\n> +\t * we have to re-start it to be able to process the request.\n> +\t */\n> +\tMutexLocker stateLock(stateMutex_);\n> +\tif (state_ == State::Flushing) {\n> +\t\tabortRequest(camera3Request);\n> +\t\treturn 0;\n> +\t}\n\nIt seems possibly overkill to do this check twice, but it shouldn't\nhurt. I suspect we'll rework this code later, possibly by adding a\nCamera::flush() in the libcamera native API, although I'm not entirely\nsure what benefit it would bring compared to a stop/start. For now this\nshould be fine.\n\n> +\n> +\tif (state_ == State::Stopped) {\n> +\t\tworker_.start();\n> +\n> +\t\tret = camera_->start();\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tstate_ = State::Running;\n> +\t}\n\nThis, however, bothers me a bit. Why do we need to start the camera in\ntwo different locations ? Could we drop the first start above ? And if\nwe do so, given that preparing the request should be a short operation,\nI wonder if we shouldn't also drop the first Flushing check at the top\nof this function.\n\nThe rest of the patch looks good to me.\n\n> +\n> \tworker_.queueRequest(descriptor.request_.get());\n> \n> \t{\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index c949fa509ca4..4aadb27c562c 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -43,6 +43,7 @@ public:\n> \n> \tint open(const hw_module_t *hardwareModule);\n> \tvoid close();\n> +\tvoid flush();\n> \n> \tunsigned int id() const { return id_; }\n> \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> @@ -92,6 +93,7 @@ private:\n> \n> \tenum class State {\n> \t\tStopped,\n> +\t\tFlushing,\n> \t\tRunning,\n> \t};\n> \n> @@ -106,6 +108,7 @@ private:\n> \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> \n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> +\tvoid abortRequest(camera3_capture_request_t *request);\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> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e80436821..8a3cfa175ff5 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> {\n> }\n> \n> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> +static int hal_dev_flush(const struct camera3_device *dev)\n> {\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> +\tcamera->flush();\n> +\n> \treturn 0;\n> }\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 53283C3206\n\tfor ;\n\tFri, 28 May 2021 07:53:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3D2B68924;\n\tFri, 28 May 2021 09:53:36 +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 546E66891E\n\tfor ;\n\tFri, 28 May 2021 09:53:34 +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 B82528C7;\n\tFri, 28 May 2021 09:53:33 +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=\"nW/Vl2Ur\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622188413;\n\tbh=XryLAEnoukwiHx6z+1moD9d7RfiLojGcKnhpCZvCLOs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nW/Vl2UrtReVCL+LzgaSArTs/Ai+09OIzlQpoo6QP4yatu8TD8k4u/5bB/lknkh5n\n\tkz0ySUuof69D5KxMOq/clqcMLwPkk0f3qN3nTfEilxz/AKmUr7eP2DnRFWHygLWiI2\n\t60VeIjL565f8dhz/hOofg8Pz8aJ5SFkAGhZYs+Yc=","Date":"Fri, 28 May 2021 10:53:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210527220359.30127-9-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17349,"web_url":"https://patchwork.libcamera.org/comment/17349/","msgid":"","date":"2021-05-31T00:59:51","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > Implement the flush() camera operation in the CameraDevice class\n> > and make it available to the camera framework by implementing the\n> > operation wrapper in camera_ops.cpp.\n> > \n> > Introduce a new camera state State::Flushing to handle concurrent\n> > flush() and process_capture_request() calls.\n> > \n> > As flush() can race with processCaptureRequest() protect it\n> > by introducing a new State::Flushing state that\n> > processCaptureRequest() inspects before queuing the Request to the\n> > Camera. If flush() is in progress while processCaptureRequest() is\n> > called, return the current Request immediately in error state. If\n> > flush() has completed and a new call to processCaptureRequest() is\n> > made just after, start the camera again before queuing the request.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> > src/android/camera_device.h | 3 ++\n> > src/android/camera_ops.cpp | 8 +++-\n> > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > \tcamera_->release();\n> > }\n> > \n> > +void CameraDevice::flush()\n> > +{\n> > +\t{\n> > +\t\tMutexLocker stateLock(stateMutex_);\n> > +\t\tif (state_ != State::Running)\n> > +\t\t\treturn;\n> > +\n> > +\t\tstate_ = State::Flushing;\n> > +\t}\n> > +\n> > +\tworker_.stop();\n> > +\tcamera_->stop();\n> > +\n> > +\tMutexLocker stateLock(stateMutex_);\n> > +\tstate_ = State::Stopped;\n> > +}\n> > +\n> > void CameraDevice::stop()\n> > {\n> > \tMutexLocker stateLock(stateMutex_);\n> > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > \treturn 0;\n> > }\n> > \n> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > +{\n> > +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > +\n> > +\tcamera3_capture_result_t result = {};\n> > +\tresult.num_output_buffers = request->num_output_buffers;\n> > +\tresult.frame_number = request->frame_number;\n> > +\tresult.partial_result = 0;\n> > +\n> > +\tstd::vector resultBuffers(result.num_output_buffers);\n> > +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > +\t\tbuffer = request->output_buffers[i];\n> > +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> > +\t\tbuffer.acquire_fence = -1;\n> > +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +\t}\n> > +\tresult.output_buffers = resultBuffers.data();\n> > +\n> > +\tcallbacks_->process_capture_result(callbacks_, &result);\n> > +}\n> > +\n> > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > {\n> > \tif (!isValidRequest(camera3Request))\n> > \t\treturn -EINVAL;\n> > \n> > \t{\n> > +\t\t/*\n> > +\t\t * Start the camera if that's the first request we handle after\n> > +\t\t * a configuration or after a flush.\n> > +\t\t *\n> > +\t\t * If flush is in progress, return the pending request\n> > +\t\t * immediately in error state.\n> > +\t\t */\n> > \t\tMutexLocker stateLock(stateMutex_);\n> > +\t\tif (state_ == State::Flushing) {\n> > +\t\t\tabortRequest(camera3Request);\n> > +\t\t\treturn 0;\n> > +\t\t}\n> > \n> > -\t\t/* Start the camera if that's the first request we handle. */\n> > \t\tif (state_ == State::Stopped) {\n> > \t\t\tworker_.start();\n> > \n> > @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > \tif (ret)\n> > \t\treturn ret;\n> > \n> > +\t/*\n> > +\t * Just before queuing the request, make sure flush() has not\n> > +\t * been called while this function was running. If flush is in progress\n> > +\t * abort the request. If flush has completed and has stopped the camera\n> > +\t * we have to re-start it to be able to process the request.\n> > +\t */\n> > +\tMutexLocker stateLock(stateMutex_);\n> > +\tif (state_ == State::Flushing) {\n> > +\t\tabortRequest(camera3Request);\n> > +\t\treturn 0;\n> > +\t}\n> \n> It seems possibly overkill to do this check twice, but it shouldn't\n> hurt. I suspect we'll rework this code later, possibly by adding a\n> Camera::flush() in the libcamera native API, although I'm not entirely\n> sure what benefit it would bring compared to a stop/start. For now this\n> should be fine.\n> \n> > +\n> > +\tif (state_ == State::Stopped) {\n> > +\t\tworker_.start();\n> > +\n> > +\t\tret = camera_->start();\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tstate_ = State::Running;\n> > +\t}\n> \n> This, however, bothers me a bit. Why do we need to start the camera in\n> two different locations ? Could we drop the first start above ? And if\n> we do so, given that preparing the request should be a short operation,\n> I wonder if we shouldn't also drop the first Flushing check at the top\n> of this function.\n\nThis shouldn't be a blocker though, so I'll merge the series after\nrunning tests. We can address the issue on top.\n\n> The rest of the patch looks good to me.\n> \n> > +\n> > \tworker_.queueRequest(descriptor.request_.get());\n> > \n> > \t{\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index c949fa509ca4..4aadb27c562c 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -43,6 +43,7 @@ public:\n> > \n> > \tint open(const hw_module_t *hardwareModule);\n> > \tvoid close();\n> > +\tvoid flush();\n> > \n> > \tunsigned int id() const { return id_; }\n> > \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> > @@ -92,6 +93,7 @@ private:\n> > \n> > \tenum class State {\n> > \t\tStopped,\n> > +\t\tFlushing,\n> > \t\tRunning,\n> > \t};\n> > \n> > @@ -106,6 +108,7 @@ private:\n> > \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> > \n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > +\tvoid abortRequest(camera3_capture_request_t *request);\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> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e80436821..8a3cfa175ff5 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > {\n> > }\n> > \n> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > +static int hal_dev_flush(const struct camera3_device *dev)\n> > {\n> > +\tif (!dev)\n> > +\t\treturn -EINVAL;\n> > +\n> > +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> > +\tcamera->flush();\n> > +\n> > \treturn 0;\n> > }\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 56BA5C3205\n\tfor ;\n\tMon, 31 May 2021 01:00:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 47FF168922;\n\tMon, 31 May 2021 03:00:05 +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 412CF6891E\n\tfor ;\n\tMon, 31 May 2021 03:00:02 +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 A39DA1254;\n\tMon, 31 May 2021 03:00: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=\"VJnj9Sdg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622422801;\n\tbh=AaOWWxTAOXa5E99so/qIHZ4BzVzAY6Lwv6YnYUotWcU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VJnj9SdgeKmkcv56z8Skn5o+zSOl+Kd4DVfnj9thNQi3MoKYVdZvUdNaOWcZ5HafJ\n\tf1lr08aapwAAl6P/NZ9BVkn+2nHpkp6qkvapt0xvZQ8GDaDVatrUiTe0nS0UXr+Gac\n\tRJhnWn5/s95QE7ehfI5ykm5JFppPglJKyk3Y/frw=","Date":"Mon, 31 May 2021 03:59:51 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17368,"web_url":"https://patchwork.libcamera.org/comment/17368/","msgid":"","date":"2021-06-02T00:58:47","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > Implement the flush() camera operation in the CameraDevice class\n> > > and make it available to the camera framework by implementing the\n> > > operation wrapper in camera_ops.cpp.\n> > > \n> > > Introduce a new camera state State::Flushing to handle concurrent\n> > > flush() and process_capture_request() calls.\n> > > \n> > > As flush() can race with processCaptureRequest() protect it\n> > > by introducing a new State::Flushing state that\n> > > processCaptureRequest() inspects before queuing the Request to the\n> > > Camera. If flush() is in progress while processCaptureRequest() is\n> > > called, return the current Request immediately in error state. If\n> > > flush() has completed and a new call to processCaptureRequest() is\n> > > made just after, start the camera again before queuing the request.\n> > > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> > > src/android/camera_device.h | 3 ++\n> > > src/android/camera_ops.cpp | 8 +++-\n> > > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > \tcamera_->release();\n> > > }\n> > > \n> > > +void CameraDevice::flush()\n> > > +{\n> > > +\t{\n> > > +\t\tMutexLocker stateLock(stateMutex_);\n> > > +\t\tif (state_ != State::Running)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tstate_ = State::Flushing;\n> > > +\t}\n> > > +\n> > > +\tworker_.stop();\n> > > +\tcamera_->stop();\n> > > +\n> > > +\tMutexLocker stateLock(stateMutex_);\n> > > +\tstate_ = State::Stopped;\n> > > +}\n> > > +\n> > > void CameraDevice::stop()\n> > > {\n> > > \tMutexLocker stateLock(stateMutex_);\n> > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > \treturn 0;\n> > > }\n> > > \n> > > +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > > +{\n> > > +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > > +\n> > > +\tcamera3_capture_result_t result = {};\n> > > +\tresult.num_output_buffers = request->num_output_buffers;\n> > > +\tresult.frame_number = request->frame_number;\n> > > +\tresult.partial_result = 0;\n> > > +\n> > > +\tstd::vector resultBuffers(result.num_output_buffers);\n> > > +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > +\t\tbuffer = request->output_buffers[i];\n> > > +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> > > +\t\tbuffer.acquire_fence = -1;\n> > > +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +\t}\n> > > +\tresult.output_buffers = resultBuffers.data();\n> > > +\n> > > +\tcallbacks_->process_capture_result(callbacks_, &result);\n> > > +}\n> > > +\n> > > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > > {\n> > > \tif (!isValidRequest(camera3Request))\n> > > \t\treturn -EINVAL;\n> > > \n> > > \t{\n> > > +\t\t/*\n> > > +\t\t * Start the camera if that's the first request we handle after\n> > > +\t\t * a configuration or after a flush.\n> > > +\t\t *\n> > > +\t\t * If flush is in progress, return the pending request\n> > > +\t\t * immediately in error state.\n> > > +\t\t */\n> > > \t\tMutexLocker stateLock(stateMutex_);\n> > > +\t\tif (state_ == State::Flushing) {\n> > > +\t\t\tabortRequest(camera3Request);\n> > > +\t\t\treturn 0;\n> > > +\t\t}\n> > > \n> > > -\t\t/* Start the camera if that's the first request we handle. */\n> > > \t\tif (state_ == State::Stopped) {\n> > > \t\t\tworker_.start();\n> > > \n> > > @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > \tif (ret)\n> > > \t\treturn ret;\n> > > \n> > > +\t/*\n> > > +\t * Just before queuing the request, make sure flush() has not\n> > > +\t * been called while this function was running. If flush is in progress\n> > > +\t * abort the request. If flush has completed and has stopped the camera\n> > > +\t * we have to re-start it to be able to process the request.\n> > > +\t */\n> > > +\tMutexLocker stateLock(stateMutex_);\n> > > +\tif (state_ == State::Flushing) {\n> > > +\t\tabortRequest(camera3Request);\n> > > +\t\treturn 0;\n> > > +\t}\n> > \n> > It seems possibly overkill to do this check twice, but it shouldn't\n> > hurt. I suspect we'll rework this code later, possibly by adding a\n> > Camera::flush() in the libcamera native API, although I'm not entirely\n> > sure what benefit it would bring compared to a stop/start. For now this\n> > should be fine.\n> > \n> > > +\n> > > +\tif (state_ == State::Stopped) {\n> > > +\t\tworker_.start();\n> > > +\n> > > +\t\tret = camera_->start();\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > > +\t\t\treturn ret;\n> > > +\t\t}\n> > > +\n> > > +\t\tstate_ = State::Running;\n> > > +\t}\n> > \n> > This, however, bothers me a bit. Why do we need to start the camera in\n> > two different locations ? Could we drop the first start above ? And if\n> > we do so, given that preparing the request should be a short operation,\n> > I wonder if we shouldn't also drop the first Flushing check at the top\n> > of this function.\n> \n> This shouldn't be a blocker though, so I'll merge the series after\n> running tests. We can address the issue on top.\n\nI'm afraid this series causes CTS regressions :-(\n\nI'm running the full camera tests with\n\nrun cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n\nWith the current master branch, I get 22 or 23 failures (some are a bit\nrandom), and with this series, it increases to 25. Here's the diff:\n\n@@ -3,14 +3,16 @@\n android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n-android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations\n+android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n+android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination\n android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n android.hardware.cts.CameraTest#testFocusDistances\n android.hardware.cts.CameraTest#testImmediateZoom\n+android.hardware.cts.CameraTest#testJpegExif\n android.hardware.cts.CameraTest#testPreviewCallback\n android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n\nThere's some randomness in the RecordingTest, so that may not be\nsignificant. The other three tests seem to pass consistently in master,\nand fail consistently with the series applied. They also fail when run\nin isolation.\n\n> > The rest of the patch looks good to me.\n> > \n> > > +\n> > > \tworker_.queueRequest(descriptor.request_.get());\n> > > \n> > > \t{\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index c949fa509ca4..4aadb27c562c 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -43,6 +43,7 @@ public:\n> > > \n> > > \tint open(const hw_module_t *hardwareModule);\n> > > \tvoid close();\n> > > +\tvoid flush();\n> > > \n> > > \tunsigned int id() const { return id_; }\n> > > \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> > > @@ -92,6 +93,7 @@ private:\n> > > \n> > > \tenum class State {\n> > > \t\tStopped,\n> > > +\t\tFlushing,\n> > > \t\tRunning,\n> > > \t};\n> > > \n> > > @@ -106,6 +108,7 @@ private:\n> > > \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> > > \n> > > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > +\tvoid abortRequest(camera3_capture_request_t *request);\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> > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > index 696e80436821..8a3cfa175ff5 100644\n> > > --- a/src/android/camera_ops.cpp\n> > > +++ b/src/android/camera_ops.cpp\n> > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > > {\n> > > }\n> > > \n> > > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > {\n> > > +\tif (!dev)\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> > > +\tcamera->flush();\n> > > +\n> > > \treturn 0;\n> > > }\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 68B4DC3205\n\tfor ;\n\tWed, 2 Jun 2021 00:59:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C24FD68925;\n\tWed, 2 Jun 2021 02:59:00 +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 83AC3602A8\n\tfor ;\n\tWed, 2 Jun 2021 02:58:58 +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 E3E8B50E;\n\tWed, 2 Jun 2021 02:58:57 +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=\"L6uK6Mjx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622595538;\n\tbh=rkPMvSlQuACzF3YlrnQjqkW4kSmCgmI0OR89VqfkcXk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=L6uK6MjxZZksOjXdB1+kUecLw0dora0l02vK7j4wA4WMZMlWRhTWjVio+YpSfi8MW\n\tqmnzxvQNYQwVv7pK1rVHCMyIpfHduJ1oIw5ZbGofH9Pq2yzhUIHGQNxsILw/07cZr1\n\tkZzieJLff4+C9WpvUChuZwwMSiPm/4/OicS/uWaM=","Date":"Wed, 2 Jun 2021 03:58:47 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\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 v4 8/8] android: Implement flush()\n\tcamera operation","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":17375,"web_url":"https://patchwork.libcamera.org/comment/17375/","msgid":"<20210602043844.GF1929@pyrite.rasen.tech>","date":"2021-06-02T04:38:44","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n> \n> On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > > Implement the flush() camera operation in the CameraDevice class\n> > > > and make it available to the camera framework by implementing the\n> > > > operation wrapper in camera_ops.cpp.\n> > > > \n> > > > Introduce a new camera state State::Flushing to handle concurrent\n> > > > flush() and process_capture_request() calls.\n> > > > \n> > > > As flush() can race with processCaptureRequest() protect it\n> > > > by introducing a new State::Flushing state that\n> > > > processCaptureRequest() inspects before queuing the Request to the\n> > > > Camera. If flush() is in progress while processCaptureRequest() is\n> > > > called, return the current Request immediately in error state. If\n> > > > flush() has completed and a new call to processCaptureRequest() is\n> > > > made just after, start the camera again before queuing the request.\n> > > > \n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-\n> > > > src/android/camera_device.h | 3 ++\n> > > > src/android/camera_ops.cpp | 8 +++-\n> > > > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > > > \n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > > \tcamera_->release();\n> > > > }\n> > > > \n> > > > +void CameraDevice::flush()\n> > > > +{\n> > > > +\t{\n> > > > +\t\tMutexLocker stateLock(stateMutex_);\n> > > > +\t\tif (state_ != State::Running)\n> > > > +\t\t\treturn;\n> > > > +\n> > > > +\t\tstate_ = State::Flushing;\n> > > > +\t}\n> > > > +\n> > > > +\tworker_.stop();\n> > > > +\tcamera_->stop();\n> > > > +\n> > > > +\tMutexLocker stateLock(stateMutex_);\n> > > > +\tstate_ = State::Stopped;\n> > > > +}\n> > > > +\n> > > > void CameraDevice::stop()\n> > > > {\n> > > > \tMutexLocker stateLock(stateMutex_);\n> > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > \treturn 0;\n> > > > }\n> > > > \n> > > > +void CameraDevice::abortRequest(camera3_capture_request_t *request)\n> > > > +{\n> > > > +\tnotifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);\n> > > > +\n> > > > +\tcamera3_capture_result_t result = {};\n> > > > +\tresult.num_output_buffers = request->num_output_buffers;\n> > > > +\tresult.frame_number = request->frame_number;\n> > > > +\tresult.partial_result = 0;\n> > > > +\n> > > > +\tstd::vector resultBuffers(result.num_output_buffers);\n> > > > +\tfor (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > > +\t\tbuffer = request->output_buffers[i];\n> > > > +\t\tbuffer.release_fence = request->output_buffers[i].acquire_fence;\n> > > > +\t\tbuffer.acquire_fence = -1;\n> > > > +\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > +\t}\n> > > > +\tresult.output_buffers = resultBuffers.data();\n> > > > +\n> > > > +\tcallbacks_->process_capture_result(callbacks_, &result);\n> > > > +}\n> > > > +\n> > > > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > > > {\n> > > > \tif (!isValidRequest(camera3Request))\n> > > > \t\treturn -EINVAL;\n> > > > \n> > > > \t{\n> > > > +\t\t/*\n> > > > +\t\t * Start the camera if that's the first request we handle after\n> > > > +\t\t * a configuration or after a flush.\n> > > > +\t\t *\n> > > > +\t\t * If flush is in progress, return the pending request\n> > > > +\t\t * immediately in error state.\n> > > > +\t\t */\n> > > > \t\tMutexLocker stateLock(stateMutex_);\n> > > > +\t\tif (state_ == State::Flushing) {\n> > > > +\t\t\tabortRequest(camera3Request);\n> > > > +\t\t\treturn 0;\n> > > > +\t\t}\n> > > > \n> > > > -\t\t/* Start the camera if that's the first request we handle. */\n> > > > \t\tif (state_ == State::Stopped) {\n> > > > \t\t\tworker_.start();\n> > > > \n> > > > @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > \tif (ret)\n> > > > \t\treturn ret;\n> > > > \n> > > > +\t/*\n> > > > +\t * Just before queuing the request, make sure flush() has not\n> > > > +\t * been called while this function was running. If flush is in progress\n> > > > +\t * abort the request. If flush has completed and has stopped the camera\n> > > > +\t * we have to re-start it to be able to process the request.\n> > > > +\t */\n> > > > +\tMutexLocker stateLock(stateMutex_);\n> > > > +\tif (state_ == State::Flushing) {\n> > > > +\t\tabortRequest(camera3Request);\n> > > > +\t\treturn 0;\n> > > > +\t}\n> > > \n> > > It seems possibly overkill to do this check twice, but it shouldn't\n> > > hurt. I suspect we'll rework this code later, possibly by adding a\n> > > Camera::flush() in the libcamera native API, although I'm not entirely\n> > > sure what benefit it would bring compared to a stop/start. For now this\n> > > should be fine.\n> > > \n> > > > +\n> > > > +\tif (state_ == State::Stopped) {\n> > > > +\t\tworker_.start();\n> > > > +\n> > > > +\t\tret = camera_->start();\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(HAL, Error) << \"Failed to start camera\";\n> > > > +\t\t\treturn ret;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tstate_ = State::Running;\n> > > > +\t}\n> > > \n> > > This, however, bothers me a bit. Why do we need to start the camera in\n> > > two different locations ? Could we drop the first start above ? And if\n> > > we do so, given that preparing the request should be a short operation,\n> > > I wonder if we shouldn't also drop the first Flushing check at the top\n> > > of this function.\n> > \n> > This shouldn't be a blocker though, so I'll merge the series after\n> > running tests. We can address the issue on top.\n> \n> I'm afraid this series causes CTS regressions :-(\n> \n> I'm running the full camera tests with\n> \n> run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n> \n> With the current master branch, I get 22 or 23 failures (some are a bit\n> random), and with this series, it increases to 25. Here's the diff:\n> \n> @@ -3,14 +3,16 @@\n> android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n> android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n> android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n> -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n> android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations\n> +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> +android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination\n> android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n> android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n> android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n> android.hardware.cts.CameraTest#testFocusDistances\n> android.hardware.cts.CameraTest#testImmediateZoom\n> +android.hardware.cts.CameraTest#testJpegExif\n> android.hardware.cts.CameraTest#testPreviewCallback\n> android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n> android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n\nWith the subplan, all of the existing ones are excluded. The\nRecordingTests are failing at random (I'll file a bug report for this).\n\nI've managed to reproduce the three extra failures though, and they\nconsistently fail with a segfault.\n\n\nPaul\n\n> \n> There's some randomness in the RecordingTest, so that may not be\n> significant. The other three tests seem to pass consistently in master,\n> and fail consistently with the series applied. They also fail when run\n> in isolation.\n> \n> > > The rest of the patch looks good to me.\n> > > \n> > > > +\n> > > > \tworker_.queueRequest(descriptor.request_.get());\n> > > > \n> > > > \t{\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index c949fa509ca4..4aadb27c562c 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -43,6 +43,7 @@ public:\n> > > > \n> > > > \tint open(const hw_module_t *hardwareModule);\n> > > > \tvoid close();\n> > > > +\tvoid flush();\n> > > > \n> > > > \tunsigned int id() const { return id_; }\n> > > > \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> > > > @@ -92,6 +93,7 @@ private:\n> > > > \n> > > > \tenum class State {\n> > > > \t\tStopped,\n> > > > +\t\tFlushing,\n> > > > \t\tRunning,\n> > > > \t};\n> > > > \n> > > > @@ -106,6 +108,7 @@ private:\n> > > > \tgetRawResolutions(const libcamera::PixelFormat &pixelFormat);\n> > > > \n> > > > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > > +\tvoid abortRequest(camera3_capture_request_t *request);\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> > > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > --- a/src/android/camera_ops.cpp\n> > > > +++ b/src/android/camera_ops.cpp\n> > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > > > {\n> > > > }\n> > > > \n> > > > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > {\n> > > > +\tif (!dev)\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\tCameraDevice *camera = reinterpret_cast(dev->priv);\n> > > > +\tcamera->flush();\n> > > > +\n> > > > \treturn 0;\n> > > > }\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 D74AFC3205\n\tfor ;\n\tWed, 2 Jun 2021 04:38:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3358468927;\n\tWed, 2 Jun 2021 06:38:53 +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 9E8C7602A3\n\tfor ;\n\tWed, 2 Jun 2021 06:38:51 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6676411;\n\tWed, 2 Jun 2021 06:38:49 +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=\"TWsdjw6p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622608731;\n\tbh=wHfSON85N2fqfGoUoUGIcaF+vTSn0LTjXfFvLYdab+Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWsdjw6phdZgcZ5Vd2VokeBJJayKuQ7p5ftG6EA+0wzDGnBXS8MdXxqmJlnjSVE3S\n\tDfgW5GGHU26ztQsJoSnAl06lzS358EWSakJ2w/ZypFqpNCOdiO66P/jESCUDfkxCPH\n\tFCsLJzzKrbvOSV9Q4SWqIPivoaW1vp+Pp+hKYsZA=","Date":"Wed, 2 Jun 2021 13:38:44 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210602043844.GF1929@pyrite.rasen.tech>","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17399,"web_url":"https://patchwork.libcamera.org/comment/17399/","msgid":"","date":"2021-06-04T08:48:45","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Paul and Laurent,\n\nOn Wed, Jun 2, 2021 at 1:38 PM wrote:\n\n> Hi Jacopo,\n>\n> On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > > > Implement the flush() camera operation in the CameraDevice class\n> > > > > and make it available to the camera framework by implementing the\n> > > > > operation wrapper in camera_ops.cpp.\n> > > > >\n> > > > > Introduce a new camera state State::Flushing to handle concurrent\n> > > > > flush() and process_capture_request() calls.\n> > > > >\n> > > > > As flush() can race with processCaptureRequest() protect it\n> > > > > by introducing a new State::Flushing state that\n> > > > > processCaptureRequest() inspects before queuing the Request to the\n> > > > > Camera. If flush() is in progress while processCaptureRequest() is\n> > > > > called, return the current Request immediately in error state. If\n> > > > > flush() has completed and a new call to processCaptureRequest() is\n> > > > > made just after, start the camera again before queuing the request.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_device.cpp | 74\n> ++++++++++++++++++++++++++++++++++-\n> > > > > src/android/camera_device.h | 3 ++\n> > > > > src/android/camera_ops.cpp | 8 +++-\n> > > > > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp\n> b/src/android/camera_device.cpp\n> > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > > > camera_->release();\n> > > > > }\n> > > > >\n> > > > > +void CameraDevice::flush()\n> > > > > +{\n> > > > > + {\n> > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > + if (state_ != State::Running)\n> > > > > + return;\n> > > > > +\n> > > > > + state_ = State::Flushing;\n> > > > > + }\n> > > > > +\n> > > > > + worker_.stop();\n> > > > > + camera_->stop();\n> > > > > +\n> > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > + state_ = State::Stopped;\n> > > > > +}\n> > > > > +\n> > > > > void CameraDevice::stop()\n> > > > > {\n> > > > > MutexLocker stateLock(stateMutex_);\n> > > > > @@ -1894,15 +1911,46 @@ int\n> CameraDevice::processControls(Camera3RequestDescriptor *descriptor)\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > +void CameraDevice::abortRequest(camera3_capture_request_t\n> *request)\n> > > > > +{\n> > > > > + notifyError(request->frame_number, nullptr,\n> CAMERA3_MSG_ERROR_REQUEST);\n> > > > > +\n> > > > > + camera3_capture_result_t result = {};\n> > > > > + result.num_output_buffers = request->num_output_buffers;\n> > > > > + result.frame_number = request->frame_number;\n> > > > > + result.partial_result = 0;\n> > > > > +\n> > > > > + std::vector\n> resultBuffers(result.num_output_buffers);\n> > > > > + for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > > > + buffer = request->output_buffers[i];\n> > > > > + buffer.release_fence =\n> request->output_buffers[i].acquire_fence;\n> > > > > + buffer.acquire_fence = -1;\n> > > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > > + }\n> > > > > + result.output_buffers = resultBuffers.data();\n> > > > > +\n> > > > > + callbacks_->process_capture_result(callbacks_, &result);\n> > > > > +}\n> > > > > +\n> > > > > int CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Request)\n> > > > > {\n> > > > > if (!isValidRequest(camera3Request))\n> > > > > return -EINVAL;\n> > > > >\n> > > > > {\n> > > > > + /*\n> > > > > + * Start the camera if that's the first request we\n> handle after\n> > > > > + * a configuration or after a flush.\n> > > > > + *\n> > > > > + * If flush is in progress, return the pending\n> request\n> > > > > + * immediately in error state.\n> > > > > + */\n> > > > > MutexLocker stateLock(stateMutex_);\n> > > > > + if (state_ == State::Flushing) {\n> > > > > + abortRequest(camera3Request);\n> > > > > + return 0;\n> > > > > + }\n> > > > >\n> > > > > - /* Start the camera if that's the first request we\n> handle. */\n> > > > > if (state_ == State::Stopped) {\n> > > > > worker_.start();\n> > > > >\n> > > > > @@ -2004,6 +2052,30 @@ int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques\n> > > > > if (ret)\n> > > > > return ret;\n> > > > >\n> > > > > + /*\n> > > > > + * Just before queuing the request, make sure flush() has\n> not\n> > > > > + * been called while this function was running. If flush\n> is in progress\n> > > > > + * abort the request. If flush has completed and has\n> stopped the camera\n> > > > > + * we have to re-start it to be able to process the\n> request.\n> > > > > + */\n> > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > + if (state_ == State::Flushing) {\n> > > > > + abortRequest(camera3Request);\n> > > > > + return 0;\n> > > > > + }\n> > > >\n> > > > It seems possibly overkill to do this check twice, but it shouldn't\n> > > > hurt. I suspect we'll rework this code later, possibly by adding a\n> > > > Camera::flush() in the libcamera native API, although I'm not\n> entirely\n> > > > sure what benefit it would bring compared to a stop/start. For now\n> this\n> > > > should be fine.\n> > > >\n> > > > > +\n> > > > > + if (state_ == State::Stopped) {\n> > > > > + worker_.start();\n> > > > > +\n> > > > > + ret = camera_->start();\n> > > > > + if (ret) {\n> > > > > + LOG(HAL, Error) << \"Failed to start\n> camera\";\n> > > > > + return ret;\n> > > > > + }\n> > > > > +\n> > > > > + state_ = State::Running;\n> > > > > + }\n> > > >\n> > > > This, however, bothers me a bit. Why do we need to start the camera\n> in\n> > > > two different locations ? Could we drop the first start above ? And\n> if\n> > > > we do so, given that preparing the request should be a short\n> operation,\n> > > > I wonder if we shouldn't also drop the first Flushing check at the\n> top\n> > > > of this function.\n> > >\n> > > This shouldn't be a blocker though, so I'll merge the series after\n> > > running tests. We can address the issue on top.\n> >\n> > I'm afraid this series causes CTS regressions :-(\n> >\n> > I'm running the full camera tests with\n> >\n> > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n> >\n> > With the current master branch, I get 22 or 23 failures (some are a bit\n> > random), and with this series, it increases to 25. Here's the diff:\n> >\n> > @@ -3,14 +3,16 @@\n> > android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n> > android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n> > android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n> > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n> >\n> android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations\n> > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> >\n> android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination\n> > android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n> > android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n> > android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n> > android.hardware.cts.CameraTest#testFocusDistances\n> > android.hardware.cts.CameraTest#testImmediateZoom\n> > +android.hardware.cts.CameraTest#testJpegExif\n> > android.hardware.cts.CameraTest#testPreviewCallback\n> > android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n> > android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n>\n> With the subplan, all of the existing ones are excluded. The\n> RecordingTests are failing at random (I'll file a bug report for this).\n>\n> I've managed to reproduce the three extra failures though, and they\n> consistently fail with a segfault.\n>\n>\nI ran android.hardware.camera2.cts.StillCaptureTest and the failure tests\npass.\nCould you tell me on top of what commit you applied the patch series?\n\n-Hiro\n\n\n>\n> Paul\n>\n> >\n> > There's some randomness in the RecordingTest, so that may not be\n> > significant. The other three tests seem to pass consistently in master,\n> > and fail consistently with the series applied. They also fail when run\n> > in isolation.\n> >\n> > > > The rest of the patch looks good to me.\n> > > >\n> > > > > +\n> > > > > worker_.queueRequest(descriptor.request_.get());\n> > > > >\n> > > > > {\n> > > > > diff --git a/src/android/camera_device.h\n> b/src/android/camera_device.h\n> > > > > index c949fa509ca4..4aadb27c562c 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -43,6 +43,7 @@ public:\n> > > > >\n> > > > > int open(const hw_module_t *hardwareModule);\n> > > > > void close();\n> > > > > + void flush();\n> > > > >\n> > > > > unsigned int id() const { return id_; }\n> > > > > camera3_device_t *camera3Device() { return\n> &camera3Device_; }\n> > > > > @@ -92,6 +93,7 @@ private:\n> > > > >\n> > > > > enum class State {\n> > > > > Stopped,\n> > > > > + Flushing,\n> > > > > Running,\n> > > > > };\n> > > > >\n> > > > > @@ -106,6 +108,7 @@ private:\n> > > > > getRawResolutions(const libcamera::PixelFormat\n> &pixelFormat);\n> > > > >\n> > > > > libcamera::FrameBuffer *createFrameBuffer(const\n> buffer_handle_t camera3buffer);\n> > > > > + void abortRequest(camera3_capture_request_t *request);\n> > > > > void notifyShutter(uint32_t frameNumber, uint64_t\n> timestamp);\n> > > > > void notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream,\n> > > > > camera3_error_msg_code code);\n> > > > > diff --git a/src/android/camera_ops.cpp\n> b/src/android/camera_ops.cpp\n> > > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > > --- a/src/android/camera_ops.cpp\n> > > > > +++ b/src/android/camera_ops.cpp\n> > > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> struct camera3_device *dev,\n> > > > > {\n> > > > > }\n> > > > >\n> > > > > -static int hal_dev_flush([[maybe_unused]] const struct\n> camera3_device *dev)\n> > > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > > {\n> > > > > + if (!dev)\n> > > > > + return -EINVAL;\n> > > > > +\n> > > > > + CameraDevice *camera = reinterpret_cast *>(dev->priv);\n> > > > > + camera->flush();\n> > > > > +\n> > > > > return 0;\n> > > > > }\n> > > > >\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 1A1A3C3208\n\tfor ;\n\tFri, 4 Jun 2021 08:48:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6622F6892C;\n\tFri, 4 Jun 2021 10:48:57 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D02268922\n\tfor ;\n\tFri, 4 Jun 2021 10:48:56 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id g8so13371661ejx.1\n\tfor ;\n\tFri, 04 Jun 2021 01:48:56 -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=\"anq+oozd\"; 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=MKpZyqYCb9JR0oA9GkKDkt0mxmPMiLN/wtcSqDBwyDg=;\n\tb=anq+oozdmWNCdGeCynKqAsjHVrYk7njNJy5rLbRXQuMfBu9L0hoeuUijBgj4yhST3x\n\twL6sfKu/DlO/F1r6hjVY/GYVfTgWk79G4ZEnSw99abYVjTTkH5bO0j5mSj2crgi5JTm3\n\t+3bfMwh3CiLoN0vXNFCj1hI5k2SO7OGRpxScI=","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=MKpZyqYCb9JR0oA9GkKDkt0mxmPMiLN/wtcSqDBwyDg=;\n\tb=esYnCifdfGuqwEyWy0OUBADxGfa5v+nWGOoty57AXPMCHXCW5V8wE6cRriUH6dH1oH\n\t1PA71MqzZw9cMxKbX4tdXF/AByzd89YlrxbUqcf8dxcF/dAVKEU9v247yCLGim5F2zrN\n\t2XEtBZ9BFUZrOjs90bHrfiXdfj+Z8NbgGjrzbpHRam9Fd/Bz9xYajDF8qk8g+PhKfFXR\n\t1LsqwETnr7UcbBQVwDB6KIfk8i7N8qQ2x5XtAg8/OvnvyAexYNvXRTOAsy/JN83u9RNo\n\t8qpB7djvOqo0DLixp2rREwO2WSrWBH/V2NqEVztev6vz9WHtA7WRDPIFvzSe0rcQPCMq\n\tov1A==","X-Gm-Message-State":"AOAM533po1KGKGyC8u4kXcyOdBR9SjHOdaDvoHduJhvHYNiDZPFEsuYN\n\tdrPA3Sn5Y1i86TVZW1MvohRL07td71ggMq/Nqqapjw==","X-Google-Smtp-Source":"ABdhPJyBHWsxuCJwd6N2JYJLqF6sZ1mTo2wGYcBSWqb0O4jObGuq4PJ+Xfc5Bh3L0VBcILfIybhBovUx1/OgBQXsvag=","X-Received":"by 2002:a17:906:5d14:: with SMTP id\n\tg20mr3072421ejt.243.1622796535832; \n\tFri, 04 Jun 2021 01:48:55 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t\n\t\n\t\n\t<20210602043844.GF1929@pyrite.rasen.tech>","In-Reply-To":"<20210602043844.GF1929@pyrite.rasen.tech>","From":"Hirokazu Honda ","Date":"Fri, 4 Jun 2021 17:48:45 +0900","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"0000000000000fc48b05c3ecc35f\"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17400,"web_url":"https://patchwork.libcamera.org/comment/17400/","msgid":"<20210604092709.GE156622@pyrite.rasen.tech>","date":"2021-06-04T09:27:09","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:\n> Hi Paul and Laurent,\n> \n> On Wed, Jun 2, 2021 at 1:38 PM wrote:\n> \n> Hi Jacopo,\n> \n> On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > > > Implement the flush() camera operation in the CameraDevice class\n> > > > > and make it available to the camera framework by implementing the\n> > > > > operation wrapper in camera_ops.cpp.\n> > > > >\n> > > > > Introduce a new camera state State::Flushing to handle concurrent\n> > > > > flush() and process_capture_request() calls.\n> > > > >\n> > > > > As flush() can race with processCaptureRequest() protect it\n> > > > > by introducing a new State::Flushing state that\n> > > > > processCaptureRequest() inspects before queuing the Request to the\n> > > > > Camera. If flush() is in progress while processCaptureRequest() is\n> > > > > called, return the current Request immediately in error state. If\n> > > > > flush() has completed and a new call to processCaptureRequest() is\n> > > > > made just after, start the camera again before queuing the request.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > >  src/android/camera_device.cpp | 74\n> ++++++++++++++++++++++++++++++++++-\n> > > > >  src/android/camera_device.h   |  3 ++\n> > > > >  src/android/camera_ops.cpp    |  8 +++-\n> > > > >  3 files changed, 83 insertions(+), 2 deletions(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_device.cpp b/src/android/\n> camera_device.cpp\n> > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > > > --- a/src/android/camera_device.cpp\n> > > > > +++ b/src/android/camera_device.cpp\n> > > > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > > >         camera_->release();\n> > > > >  }\n> > > > > \n> > > > > +void CameraDevice::flush()\n> > > > > +{\n> > > > > +       {\n> > > > > +               MutexLocker stateLock(stateMutex_);\n> > > > > +               if (state_ != State::Running)\n> > > > > +                       return;\n> > > > > +\n> > > > > +               state_ = State::Flushing;\n> > > > > +       }\n> > > > > +\n> > > > > +       worker_.stop();\n> > > > > +       camera_->stop();\n> > > > > +\n> > > > > +       MutexLocker stateLock(stateMutex_);\n> > > > > +       state_ = State::Stopped;\n> > > > > +}\n> > > > > +\n> > > > >  void CameraDevice::stop()\n> > > > >  {\n> > > > >         MutexLocker stateLock(stateMutex_);\n> > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls\n> (Camera3RequestDescriptor *descriptor)\n> > > > >         return 0;\n> > > > >  }\n> > > > > \n> > > > > +void CameraDevice::abortRequest(camera3_capture_request_t\n> *request)\n> > > > > +{\n> > > > > +       notifyError(request->frame_number, nullptr,\n> CAMERA3_MSG_ERROR_REQUEST);\n> > > > > +\n> > > > > +       camera3_capture_result_t result = {};\n> > > > > +       result.num_output_buffers = request->num_output_buffers;\n> > > > > +       result.frame_number = request->frame_number;\n> > > > > +       result.partial_result = 0;\n> > > > > +\n> > > > > +       std::vector resultBuffers\n> (result.num_output_buffers);\n> > > > > +       for (auto [i, buffer] : utils::enumerate(resultBuffers)) {\n> > > > > +               buffer = request->output_buffers[i];\n> > > > > +               buffer.release_fence = request->output_buffers\n> [i].acquire_fence;\n> > > > > +               buffer.acquire_fence = -1;\n> > > > > +               buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > > +       }\n> > > > > +       result.output_buffers = resultBuffers.data();\n> > > > > +\n> > > > > +       callbacks_->process_capture_result(callbacks_, &result);\n> > > > > +}\n> > > > > +\n> > > > >  int CameraDevice::processCaptureRequest(camera3_capture_request_t\n> *camera3Request)\n> > > > >  {\n> > > > >         if (!isValidRequest(camera3Request))\n> > > > >                 return -EINVAL;\n> > > > > \n> > > > >         {\n> > > > > +               /*\n> > > > > +                * Start the camera if that's the first request we\n> handle after\n> > > > > +                * a configuration or after a flush.\n> > > > > +                *\n> > > > > +                * If flush is in progress, return the pending\n> request\n> > > > > +                * immediately in error state.\n> > > > > +                */\n> > > > >                 MutexLocker stateLock(stateMutex_);\n> > > > > +               if (state_ == State::Flushing) {\n> > > > > +                       abortRequest(camera3Request);\n> > > > > +                       return 0;\n> > > > > +               }\n> > > > > \n> > > > > -               /* Start the camera if that's the first request we\n> handle. */\n> > > > >                 if (state_ == State::Stopped) {\n> > > > >                         worker_.start();\n> > > > > \n> > > > > @@ -2004,6 +2052,30 @@ int CameraDevice::processCaptureRequest\n> (camera3_capture_request_t *camera3Reques\n> > > > >         if (ret)\n> > > > >                 return ret;\n> > > > > \n> > > > > +       /*\n> > > > > +        * Just before queuing the request, make sure flush() has\n> not\n> > > > > +        * been called while this function was running. If flush is\n> in progress\n> > > > > +        * abort the request. If flush has completed and has\n> stopped the camera\n> > > > > +        * we have to re-start it to be able to process the\n> request.\n> > > > > +        */\n> > > > > +       MutexLocker stateLock(stateMutex_);\n> > > > > +       if (state_ == State::Flushing) {\n> > > > > +               abortRequest(camera3Request);\n> > > > > +               return 0;\n> > > > > +       }\n> > > >\n> > > > It seems possibly overkill to do this check twice, but it shouldn't\n> > > > hurt. I suspect we'll rework this code later, possibly by adding a\n> > > > Camera::flush() in the libcamera native API, although I'm not\n> entirely\n> > > > sure what benefit it would bring compared to a stop/start. For now\n> this\n> > > > should be fine.\n> > > >\n> > > > > +\n> > > > > +       if (state_ == State::Stopped) {\n> > > > > +               worker_.start();\n> > > > > +\n> > > > > +               ret = camera_->start();\n> > > > > +               if (ret) {\n> > > > > +                       LOG(HAL, Error) << \"Failed to start\n> camera\";\n> > > > > +                       return ret;\n> > > > > +               }\n> > > > > +\n> > > > > +               state_ = State::Running;\n> > > > > +       }\n> > > >\n> > > > This, however, bothers me a bit. Why do we need to start the camera\n> in\n> > > > two different locations ? Could we drop the first start above ? And\n> if\n> > > > we do so, given that preparing the request should be a short\n> operation,\n> > > > I wonder if we shouldn't also drop the first Flushing check at the\n> top\n> > > > of this function.\n> > >\n> > > This shouldn't be a blocker though, so I'll merge the series after\n> > > running tests. We can address the issue on top.\n> >\n> > I'm afraid this series causes CTS regressions :-(\n> >\n> > I'm running the full camera tests with\n> >\n> > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n> >\n> > With the current master branch, I get 22 or 23 failures (some are a bit\n> > random), and with this series, it increases to 25. Here's the diff:\n> >\n> > @@ -3,14 +3,16 @@\n> >  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n> >  android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n> >  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n> > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n> >  android.hardware.camera2.cts.RobustnessTest#\n> testMandatoryOutputCombinations\n> > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> >  android.hardware.camera2.cts.StillCaptureTest#\n> testStillPreviewCombination\n> >  android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n> >  android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n> >  android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n> >  android.hardware.cts.CameraTest#testFocusDistances\n> >  android.hardware.cts.CameraTest#testImmediateZoom\n> > +android.hardware.cts.CameraTest#testJpegExif\n> >  android.hardware.cts.CameraTest#testPreviewCallback\n> >  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n> >  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n> \n> With the subplan, all of the existing ones are excluded. The\n> RecordingTests are failing at random (I'll file a bug report for this).\n> \n> I've managed to reproduce the three extra failures though, and they\n> consistently fail with a segfault.\n> \n> \n> \n> I ran android.hardware.camera2.cts.StillCaptureTest and the failure tests pass.\n> Could you tell me on top of what commit you applied the patch series?\n\nOn... master. I think 59de56f4 \"qcam: Add libatomic dependency\".\n\n\nPaul\n\n>  \n> \n> \n> >\n> > There's some randomness in the RecordingTest, so that may not be\n> > significant. The other three tests seem to pass consistently in master,\n> > and fail consistently with the series applied. They also fail when run\n> > in isolation.\n> >\n> > > > The rest of the patch looks good to me.\n> > > >\n> > > > > +\n> > > > >         worker_.queueRequest(descriptor.request_.get());\n> > > > > \n> > > > >         {\n> > > > > diff --git a/src/android/camera_device.h b/src/android/\n> camera_device.h\n> > > > > index c949fa509ca4..4aadb27c562c 100644\n> > > > > --- a/src/android/camera_device.h\n> > > > > +++ b/src/android/camera_device.h\n> > > > > @@ -43,6 +43,7 @@ public:\n> > > > > \n> > > > >         int open(const hw_module_t *hardwareModule);\n> > > > >         void close();\n> > > > > +       void flush();\n> > > > > \n> > > > >         unsigned int id() const { return id_; }\n> > > > >         camera3_device_t *camera3Device() { return &camera3Device_;\n> }\n> > > > > @@ -92,6 +93,7 @@ private:\n> > > > > \n> > > > >         enum class State {\n> > > > >                 Stopped,\n> > > > > +               Flushing,\n> > > > >                 Running,\n> > > > >         };\n> > > > > \n> > > > > @@ -106,6 +108,7 @@ private:\n> > > > >         getRawResolutions(const libcamera::PixelFormat &\n> pixelFormat);\n> > > > > \n> > > > >         libcamera::FrameBuffer *createFrameBuffer(const\n> buffer_handle_t camera3buffer);\n> > > > > +       void abortRequest(camera3_capture_request_t *request);\n> > > > >         void notifyShutter(uint32_t frameNumber, uint64_t\n> timestamp);\n> > > > >         void notifyError(uint32_t frameNumber, camera3_stream_t\n> *stream,\n> > > > >                          camera3_error_msg_code code);\n> > > > > diff --git a/src/android/camera_ops.cpp b/src/android/\n> camera_ops.cpp\n> > > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > > --- a/src/android/camera_ops.cpp\n> > > > > +++ b/src/android/camera_ops.cpp\n> > > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const\n> struct camera3_device *dev,\n> > > > >  {\n> > > > >  }\n> > > > > \n> > > > > -static int hal_dev_flush([[maybe_unused]] const struct\n> camera3_device *dev)\n> > > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > >  {\n> > > > > +       if (!dev)\n> > > > > +               return -EINVAL;\n> > > > > +\n> > > > > +       CameraDevice *camera = reinterpret_cast\n> (dev->priv);\n> > > > > +       camera->flush();\n> > > > > +\n> > > > >         return 0;\n> > > > >  }\n> > > > > \n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 85AE7C3206\n\tfor ;\n\tFri, 4 Jun 2021 09:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC53268926;\n\tFri, 4 Jun 2021 11:27:17 +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 89FBB68922\n\tfor ;\n\tFri, 4 Jun 2021 11:27:16 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ABEC2EF;\n\tFri, 4 Jun 2021 11:27:14 +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=\"kc+MWOCe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1622798836;\n\tbh=eflQrsuOXnMmLBQlJ+Uh33vEfei6rawq59pXSSMkvuk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kc+MWOCeSPXCRErU3J0glNxdcRHq02jClZ90oZ1nwKvuBxEhIjC8FPZezreK+qNuD\n\tCoLeb4jrEZ3SeMVyiSiCv+REC1wTP3wALkUHmXnGJ7aTfUEc3se2+yE5HwxEvpsVLb\n\tfLBwc2uZGrRl953KnkmXUvqVvrypE0NSfi//ekek=","Date":"Fri, 4 Jun 2021 18:27:09 +0900","From":"paul.elder@ideasonboard.com","To":"Hirokazu Honda ","Message-ID":"<20210604092709.GE156622@pyrite.rasen.tech>","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t\n\t\n\t\n\t<20210602043844.GF1929@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17404,"web_url":"https://patchwork.libcamera.org/comment/17404/","msgid":"","date":"2021-06-04T09:59:38","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Paul,\n\n\n\nOn Fri, Jun 4, 2021 at 6:27 PM wrote:\n\n> Hi Hiro,\n>\n> On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:\n> > Hi Paul and Laurent,\n> >\n> > On Wed, Jun 2, 2021 at 1:38 PM wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:\n> > > Hi Jacopo,\n> > >\n> > > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> > > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > > > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > > > > Implement the flush() camera operation in the CameraDevice\n> class\n> > > > > > and make it available to the camera framework by\n> implementing the\n> > > > > > operation wrapper in camera_ops.cpp.\n> > > > > >\n> > > > > > Introduce a new camera state State::Flushing to handle\n> concurrent\n> > > > > > flush() and process_capture_request() calls.\n> > > > > >\n> > > > > > As flush() can race with processCaptureRequest() protect it\n> > > > > > by introducing a new State::Flushing state that\n> > > > > > processCaptureRequest() inspects before queuing the Request\n> to the\n> > > > > > Camera. If flush() is in progress while\n> processCaptureRequest() is\n> > > > > > called, return the current Request immediately in error\n> state. If\n> > > > > > flush() has completed and a new call to\n> processCaptureRequest() is\n> > > > > > made just after, start the camera again before queuing the\n> request.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_device.cpp | 74\n> > ++++++++++++++++++++++++++++++++++-\n> > > > > > src/android/camera_device.h | 3 ++\n> > > > > > src/android/camera_ops.cpp | 8 +++-\n> > > > > > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_device.cpp b/src/android/\n> > camera_device.cpp\n> > > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > > > > --- a/src/android/camera_device.cpp\n> > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > > > > camera_->release();\n> > > > > > }\n> > > > > >\n> > > > > > +void CameraDevice::flush()\n> > > > > > +{\n> > > > > > + {\n> > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > + if (state_ != State::Running)\n> > > > > > + return;\n> > > > > > +\n> > > > > > + state_ = State::Flushing;\n> > > > > > + }\n> > > > > > +\n> > > > > > + worker_.stop();\n> > > > > > + camera_->stop();\n> > > > > > +\n> > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > + state_ = State::Stopped;\n> > > > > > +}\n> > > > > > +\n> > > > > > void CameraDevice::stop()\n> > > > > > {\n> > > > > > MutexLocker stateLock(stateMutex_);\n> > > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls\n> > (Camera3RequestDescriptor *descriptor)\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > +void CameraDevice::abortRequest(camera3_capture_request_t\n> > *request)\n> > > > > > +{\n> > > > > > + notifyError(request->frame_number, nullptr,\n> > CAMERA3_MSG_ERROR_REQUEST);\n> > > > > > +\n> > > > > > + camera3_capture_result_t result = {};\n> > > > > > + result.num_output_buffers =\n> request->num_output_buffers;\n> > > > > > + result.frame_number = request->frame_number;\n> > > > > > + result.partial_result = 0;\n> > > > > > +\n> > > > > > + std::vector resultBuffers\n> > (result.num_output_buffers);\n> > > > > > + for (auto [i, buffer] :\n> utils::enumerate(resultBuffers)) {\n> > > > > > + buffer = request->output_buffers[i];\n> > > > > > + buffer.release_fence =\n> request->output_buffers\n> > [i].acquire_fence;\n> > > > > > + buffer.acquire_fence = -1;\n> > > > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > > > + }\n> > > > > > + result.output_buffers = resultBuffers.data();\n> > > > > > +\n> > > > > > + callbacks_->process_capture_result(callbacks_,\n> &result);\n> > > > > > +}\n> > > > > > +\n> > > > > > int\n> CameraDevice::processCaptureRequest(camera3_capture_request_t\n> > *camera3Request)\n> > > > > > {\n> > > > > > if (!isValidRequest(camera3Request))\n> > > > > > return -EINVAL;\n> > > > > >\n> > > > > > {\n> > > > > > + /*\n> > > > > > + * Start the camera if that's the first\n> request we\n> > handle after\n> > > > > > + * a configuration or after a flush.\n> > > > > > + *\n> > > > > > + * If flush is in progress, return the\n> pending\n> > request\n> > > > > > + * immediately in error state.\n> > > > > > + */\n> > > > > > MutexLocker stateLock(stateMutex_);\n> > > > > > + if (state_ == State::Flushing) {\n> > > > > > + abortRequest(camera3Request);\n> > > > > > + return 0;\n> > > > > > + }\n> > > > > >\n> > > > > > - /* Start the camera if that's the first\n> request we\n> > handle. */\n> > > > > > if (state_ == State::Stopped) {\n> > > > > > worker_.start();\n> > > > > >\n> > > > > > @@ -2004,6 +2052,30 @@ int\n> CameraDevice::processCaptureRequest\n> > (camera3_capture_request_t *camera3Reques\n> > > > > > if (ret)\n> > > > > > return ret;\n> > > > > >\n> > > > > > + /*\n> > > > > > + * Just before queuing the request, make sure\n> flush() has\n> > not\n> > > > > > + * been called while this function was running. If\n> flush is\n> > in progress\n> > > > > > + * abort the request. If flush has completed and has\n> > stopped the camera\n> > > > > > + * we have to re-start it to be able to process the\n> > request.\n> > > > > > + */\n> > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > + if (state_ == State::Flushing) {\n> > > > > > + abortRequest(camera3Request);\n> > > > > > + return 0;\n> > > > > > + }\n> > > > >\n> > > > > It seems possibly overkill to do this check twice, but it\n> shouldn't\n> > > > > hurt. I suspect we'll rework this code later, possibly by\n> adding a\n> > > > > Camera::flush() in the libcamera native API, although I'm not\n> > entirely\n> > > > > sure what benefit it would bring compared to a stop/start. For\n> now\n> > this\n> > > > > should be fine.\n> > > > >\n> > > > > > +\n> > > > > > + if (state_ == State::Stopped) {\n> > > > > > + worker_.start();\n> > > > > > +\n> > > > > > + ret = camera_->start();\n> > > > > > + if (ret) {\n> > > > > > + LOG(HAL, Error) << \"Failed to start\n> > camera\";\n> > > > > > + return ret;\n> > > > > > + }\n> > > > > > +\n> > > > > > + state_ = State::Running;\n> > > > > > + }\n> > > > >\n> > > > > This, however, bothers me a bit. Why do we need to start the\n> camera\n> > in\n> > > > > two different locations ? Could we drop the first start above\n> ? And\n> > if\n> > > > > we do so, given that preparing the request should be a short\n> > operation,\n> > > > > I wonder if we shouldn't also drop the first Flushing check at\n> the\n> > top\n> > > > > of this function.\n> > > >\n> > > > This shouldn't be a blocker though, so I'll merge the series\n> after\n> > > > running tests. We can address the issue on top.\n> > >\n> > > I'm afraid this series causes CTS regressions :-(\n> > >\n> > > I'm running the full camera tests with\n> > >\n> > > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n> > >\n> > > With the current master branch, I get 22 or 23 failures (some are\n> a bit\n> > > random), and with this series, it increases to 25. Here's the diff:\n> > >\n> > > @@ -3,14 +3,16 @@\n> > > android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n> > >\n> android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n> > > android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n> > > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n> > > android.hardware.camera2.cts.RobustnessTest#\n> > testMandatoryOutputCombinations\n> > > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> > > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> > > android.hardware.camera2.cts.StillCaptureTest#\n> > testStillPreviewCombination\n> > >\n> android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n> > >\n> android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n> > >\n> android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n> > > android.hardware.cts.CameraTest#testFocusDistances\n> > > android.hardware.cts.CameraTest#testImmediateZoom\n> > > +android.hardware.cts.CameraTest#testJpegExif\n> > > android.hardware.cts.CameraTest#testPreviewCallback\n> > > android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n> > > android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n> >\n> > With the subplan, all of the existing ones are excluded. The\n> > RecordingTests are failing at random (I'll file a bug report for\n> this).\n> >\n> > I've managed to reproduce the three extra failures though, and they\n> > consistently fail with a segfault.\n> >\n> >\n> >\n> > I ran android.hardware.camera2.cts.StillCaptureTest and the failure\n> tests pass.\n> > Could you tell me on top of what commit you applied the patch series?\n>\n> On... master. I think 59de56f4 \"qcam: Add libatomic dependency\".\n>\n>\nI couldn't reproduce on the commit with the flush patch.\nWould you mind sharing the libcamera log with me while\nrunning android.hardware.cts.CameraTest#testJpegExif?\n\nThanks,\n-Hiro\n\n>\n> Paul\n>\n> >\n> >\n> >\n> > >\n> > > There's some randomness in the RecordingTest, so that may not be\n> > > significant. The other three tests seem to pass consistently in\n> master,\n> > > and fail consistently with the series applied. They also fail when\n> run\n> > > in isolation.\n> > >\n> > > > > The rest of the patch looks good to me.\n> > > > >\n> > > > > > +\n> > > > > > worker_.queueRequest(descriptor.request_.get());\n> > > > > >\n> > > > > > {\n> > > > > > diff --git a/src/android/camera_device.h b/src/android/\n> > camera_device.h\n> > > > > > index c949fa509ca4..4aadb27c562c 100644\n> > > > > > --- a/src/android/camera_device.h\n> > > > > > +++ b/src/android/camera_device.h\n> > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > >\n> > > > > > int open(const hw_module_t *hardwareModule);\n> > > > > > void close();\n> > > > > > + void flush();\n> > > > > >\n> > > > > > unsigned int id() const { return id_; }\n> > > > > > camera3_device_t *camera3Device() { return\n> &camera3Device_;\n> > }\n> > > > > > @@ -92,6 +93,7 @@ private:\n> > > > > >\n> > > > > > enum class State {\n> > > > > > Stopped,\n> > > > > > + Flushing,\n> > > > > > Running,\n> > > > > > };\n> > > > > >\n> > > > > > @@ -106,6 +108,7 @@ private:\n> > > > > > getRawResolutions(const libcamera::PixelFormat &\n> > pixelFormat);\n> > > > > >\n> > > > > > libcamera::FrameBuffer *createFrameBuffer(const\n> > buffer_handle_t camera3buffer);\n> > > > > > + void abortRequest(camera3_capture_request_t\n> *request);\n> > > > > > void notifyShutter(uint32_t frameNumber, uint64_t\n> > timestamp);\n> > > > > > void notifyError(uint32_t frameNumber,\n> camera3_stream_t\n> > *stream,\n> > > > > > camera3_error_msg_code code);\n> > > > > > diff --git a/src/android/camera_ops.cpp b/src/android/\n> > camera_ops.cpp\n> > > > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > > > --- a/src/android/camera_ops.cpp\n> > > > > > +++ b/src/android/camera_ops.cpp\n> > > > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]]\n> const\n> > struct camera3_device *dev,\n> > > > > > {\n> > > > > > }\n> > > > > >\n> > > > > > -static int hal_dev_flush([[maybe_unused]] const struct\n> > camera3_device *dev)\n> > > > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > > > {\n> > > > > > + if (!dev)\n> > > > > > + return -EINVAL;\n> > > > > > +\n> > > > > > + CameraDevice *camera = reinterpret_cast *>\n> > (dev->priv);\n> > > > > > + camera->flush();\n> > > > > > +\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > >\n> > > --\n> > > Regards,\n> > >\n> > > Laurent Pinchart\n> >\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 BBAEDC3208\n\tfor ;\n\tFri, 4 Jun 2021 09:59:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D3226892C;\n\tFri, 4 Jun 2021 11:59:51 +0200 (CEST)","from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com\n\t[IPv6:2a00:1450:4864:20::62d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4672868922\n\tfor ;\n\tFri, 4 Jun 2021 11:59:49 +0200 (CEST)","by mail-ej1-x62d.google.com with SMTP id g20so13677444ejt.0\n\tfor ;\n\tFri, 04 Jun 2021 02:59:49 -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=\"WebHClMd\"; 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=WeYJjRMLSxe+O6tpYuDEoxIFjU5xsjPMmvP+TqOr4HM=;\n\tb=WebHClMdbbtcdebyoDTkNCM/js2uBYFV+HChkX+LFDVrv6OXfOiM4l60L5vHbYCEEo\n\tN77TUXLdWhXyfK5VfbMyQIuLhsyoTYGFGevgL5zOXcFNzlGG5ZjDNwwdKFRjQbEe7fWm\n\tAYvsUvv2B3rGwrfkFZ5YHkenpCc0pEmPx5Gpk=","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=WeYJjRMLSxe+O6tpYuDEoxIFjU5xsjPMmvP+TqOr4HM=;\n\tb=J1im7arV7ccXiDgdGMHfR1BnlUkU3Px7IaHL3aN84IwCdIe6SE0EY3YYr1CUoJg/9W\n\tbFG+O6LaMEKw3eJ9pAK4usfNp6m9rBY16rOcgkmoK6iTcZh5wWECV+jk56oVpvdzzXUl\n\tlyNYPZcOXzYwv2ariCB1xaKYB6qq9RQrvB1Se8dR+yYYtrKgQjOwHV+Kwc4KrlFPKS1c\n\t5P0KJJ9uM2vFHS3o6mzgQ9c6bL4q4fZUehG9nyHpRH9d4SbVyX3oijdYV9XI2FyjzUd4\n\t7NjCzQ7T0qVJJ8ajOaDLTwHjrbO5jbpjNiee/mXVBxXw2lZNQnRM/g68s4bST1toQ4f+\n\tjWCQ==","X-Gm-Message-State":"AOAM532AYVDl7GBddW/rDOb/yXxFWpvKVlG8XJjDuujUg1gJJvXdatIo\n\tOkFDen2FpbcgW2ksQT1SMSNPVkdBy1TuyOEH6dpkmw==","X-Google-Smtp-Source":"ABdhPJwYKSrketvB/f+/cSBhWvMGYdjMMLf9BZVWzA4ZlnEN4x+MFqVn0IixWpouWdVdzAOCxIFBl3+IJCaMLPFbgkk=","X-Received":"by 2002:a17:906:3a04:: with SMTP id\n\tz4mr3382577eje.221.1622800788796; \n\tFri, 04 Jun 2021 02:59:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t\n\t\n\t\n\t<20210602043844.GF1929@pyrite.rasen.tech>\n\t\n\t<20210604092709.GE156622@pyrite.rasen.tech>","In-Reply-To":"<20210604092709.GE156622@pyrite.rasen.tech>","From":"Hirokazu Honda ","Date":"Fri, 4 Jun 2021 18:59:38 +0900","Message-ID":"","To":"Paul Elder ","Content-Type":"multipart/alternative; boundary=\"0000000000008ec8d005c3edc0e1\"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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":17480,"web_url":"https://patchwork.libcamera.org/comment/17480/","msgid":"<20210608130239.zclulz4ou36erqn3@uno.localdomain>","date":"2021-06-08T13:02:39","subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello,\n\nOn Fri, Jun 04, 2021 at 06:59:38PM +0900, Hirokazu Honda wrote:\n> Hi Paul,\n>\n>\n>\n> On Fri, Jun 4, 2021 at 6:27 PM wrote:\n>\n> > Hi Hiro,\n> >\n> > On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:\n> > > Hi Paul and Laurent,\n> > >\n> > > On Wed, Jun 2, 2021 at 1:38 PM wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:\n> > > > Hi Jacopo,\n> > > >\n> > > > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:\n> > > > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:\n> > > > > > On Fri, May 28, 2021 at 12:03:59AM +0200, Jacopo Mondi wrote:\n> > > > > > > Implement the flush() camera operation in the CameraDevice\n> > class\n> > > > > > > and make it available to the camera framework by\n> > implementing the\n> > > > > > > operation wrapper in camera_ops.cpp.\n> > > > > > >\n> > > > > > > Introduce a new camera state State::Flushing to handle\n> > concurrent\n> > > > > > > flush() and process_capture_request() calls.\n> > > > > > >\n> > > > > > > As flush() can race with processCaptureRequest() protect it\n> > > > > > > by introducing a new State::Flushing state that\n> > > > > > > processCaptureRequest() inspects before queuing the Request\n> > to the\n> > > > > > > Camera. If flush() is in progress while\n> > processCaptureRequest() is\n> > > > > > > called, return the current Request immediately in error\n> > state. If\n> > > > > > > flush() has completed and a new call to\n> > processCaptureRequest() is\n> > > > > > > made just after, start the camera again before queuing the\n> > request.\n> > > > > > >\n> > > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > > ---\n> > > > > > > src/android/camera_device.cpp | 74\n> > > ++++++++++++++++++++++++++++++++++-\n> > > > > > > src/android/camera_device.h | 3 ++\n> > > > > > > src/android/camera_ops.cpp | 8 +++-\n> > > > > > > 3 files changed, 83 insertions(+), 2 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/\n> > > camera_device.cpp\n> > > > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644\n> > > > > > > --- a/src/android/camera_device.cpp\n> > > > > > > +++ b/src/android/camera_device.cpp\n> > > > > > > @@ -797,6 +797,23 @@ void CameraDevice::close()\n> > > > > > > camera_->release();\n> > > > > > > }\n> > > > > > >\n> > > > > > > +void CameraDevice::flush()\n> > > > > > > +{\n> > > > > > > + {\n> > > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > > + if (state_ != State::Running)\n> > > > > > > + return;\n> > > > > > > +\n> > > > > > > + state_ = State::Flushing;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + worker_.stop();\n> > > > > > > + camera_->stop();\n> > > > > > > +\n> > > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > > + state_ = State::Stopped;\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > void CameraDevice::stop()\n> > > > > > > {\n> > > > > > > MutexLocker stateLock(stateMutex_);\n> > > > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls\n> > > (Camera3RequestDescriptor *descriptor)\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > > > > > +void CameraDevice::abortRequest(camera3_capture_request_t\n> > > *request)\n> > > > > > > +{\n> > > > > > > + notifyError(request->frame_number, nullptr,\n> > > CAMERA3_MSG_ERROR_REQUEST);\n> > > > > > > +\n> > > > > > > + camera3_capture_result_t result = {};\n> > > > > > > + result.num_output_buffers =\n> > request->num_output_buffers;\n> > > > > > > + result.frame_number = request->frame_number;\n> > > > > > > + result.partial_result = 0;\n> > > > > > > +\n> > > > > > > + std::vector resultBuffers\n> > > (result.num_output_buffers);\n> > > > > > > + for (auto [i, buffer] :\n> > utils::enumerate(resultBuffers)) {\n> > > > > > > + buffer = request->output_buffers[i];\n> > > > > > > + buffer.release_fence =\n> > request->output_buffers\n> > > [i].acquire_fence;\n> > > > > > > + buffer.acquire_fence = -1;\n> > > > > > > + buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > > > > > + }\n> > > > > > > + result.output_buffers = resultBuffers.data();\n> > > > > > > +\n> > > > > > > + callbacks_->process_capture_result(callbacks_,\n> > &result);\n> > > > > > > +}\n> > > > > > > +\n> > > > > > > int\n> > CameraDevice::processCaptureRequest(camera3_capture_request_t\n> > > *camera3Request)\n> > > > > > > {\n> > > > > > > if (!isValidRequest(camera3Request))\n> > > > > > > return -EINVAL;\n> > > > > > >\n> > > > > > > {\n> > > > > > > + /*\n> > > > > > > + * Start the camera if that's the first\n> > request we\n> > > handle after\n> > > > > > > + * a configuration or after a flush.\n> > > > > > > + *\n> > > > > > > + * If flush is in progress, return the\n> > pending\n> > > request\n> > > > > > > + * immediately in error state.\n> > > > > > > + */\n> > > > > > > MutexLocker stateLock(stateMutex_);\n> > > > > > > + if (state_ == State::Flushing) {\n> > > > > > > + abortRequest(camera3Request);\n> > > > > > > + return 0;\n> > > > > > > + }\n> > > > > > >\n> > > > > > > - /* Start the camera if that's the first\n> > request we\n> > > handle. */\n> > > > > > > if (state_ == State::Stopped) {\n> > > > > > > worker_.start();\n> > > > > > >\n> > > > > > > @@ -2004,6 +2052,30 @@ int\n> > CameraDevice::processCaptureRequest\n> > > (camera3_capture_request_t *camera3Reques\n> > > > > > > if (ret)\n> > > > > > > return ret;\n> > > > > > >\n> > > > > > > + /*\n> > > > > > > + * Just before queuing the request, make sure\n> > flush() has\n> > > not\n> > > > > > > + * been called while this function was running. If\n> > flush is\n> > > in progress\n> > > > > > > + * abort the request. If flush has completed and has\n> > > stopped the camera\n> > > > > > > + * we have to re-start it to be able to process the\n> > > request.\n> > > > > > > + */\n> > > > > > > + MutexLocker stateLock(stateMutex_);\n> > > > > > > + if (state_ == State::Flushing) {\n> > > > > > > + abortRequest(camera3Request);\n> > > > > > > + return 0;\n> > > > > > > + }\n> > > > > >\n> > > > > > It seems possibly overkill to do this check twice, but it\n> > shouldn't\n> > > > > > hurt. I suspect we'll rework this code later, possibly by\n> > adding a\n> > > > > > Camera::flush() in the libcamera native API, although I'm not\n> > > entirely\n> > > > > > sure what benefit it would bring compared to a stop/start. For\n> > now\n> > > this\n> > > > > > should be fine.\n> > > > > >\n> > > > > > > +\n> > > > > > > + if (state_ == State::Stopped) {\n> > > > > > > + worker_.start();\n> > > > > > > +\n> > > > > > > + ret = camera_->start();\n> > > > > > > + if (ret) {\n> > > > > > > + LOG(HAL, Error) << \"Failed to start\n> > > camera\";\n> > > > > > > + return ret;\n> > > > > > > + }\n> > > > > > > +\n> > > > > > > + state_ = State::Running;\n> > > > > > > + }\n> > > > > >\n> > > > > > This, however, bothers me a bit. Why do we need to start the\n> > camera\n> > > in\n> > > > > > two different locations ? Could we drop the first start above\n> > ? And\n> > > if\n> > > > > > we do so, given that preparing the request should be a short\n> > > operation,\n> > > > > > I wonder if we shouldn't also drop the first Flushing check at\n> > the\n> > > top\n> > > > > > of this function.\n> > > > >\n> > > > > This shouldn't be a blocker though, so I'll merge the series\n> > after\n> > > > > running tests. We can address the issue on top.\n> > > >\n> > > > I'm afraid this series causes CTS regressions :-(\n> > > >\n> > > > I'm running the full camera tests with\n> > > >\n> > > > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases\n> > > >\n> > > > With the current master branch, I get 22 or 23 failures (some are\n> > a bit\n> > > > random), and with this series, it increases to 25. Here's the diff:\n> > > >\n> > > > @@ -3,14 +3,16 @@\n> > > > android.hardware.camera2.cts.ImageReaderTest#testRawPrivate\n> > > >\n> > android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate\n> > > > android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes\n> > > > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot\n> > > > android.hardware.camera2.cts.RobustnessTest#\n> > > testMandatoryOutputCombinations\n> > > > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths\n> > > > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif\n> > > > android.hardware.camera2.cts.StillCaptureTest#\n> > > testStillPreviewCombination\n> > > >\n> > android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces\n> > > >\n> > android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks\n> > > >\n> > android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback\n> > > > android.hardware.cts.CameraTest#testFocusDistances\n> > > > android.hardware.cts.CameraTest#testImmediateZoom\n> > > > +android.hardware.cts.CameraTest#testJpegExif\n> > > > android.hardware.cts.CameraTest#testPreviewCallback\n> > > > android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer\n> > > > android.hardware.cts.CameraTest#testPreviewCallbackWithPicture\n> > >\n> > > With the subplan, all of the existing ones are excluded. The\n> > > RecordingTests are failing at random (I'll file a bug report for\n> > this).\n> > >\n> > > I've managed to reproduce the three extra failures though, and they\n> > > consistently fail with a segfault.\n> > >\n> > >\n> > >\n> > > I ran android.hardware.camera2.cts.StillCaptureTest and the failure\n> > tests pass.\n> > > Could you tell me on top of what commit you applied the patch series?\n> >\n> > On... master. I think 59de56f4 \"qcam: Add libatomic dependency\".\n> >\n> >\n> I couldn't reproduce on the commit with the flush patch.\n> Would you mind sharing the libcamera log with me while\n> running android.hardware.cts.CameraTest#testJpegExif?\n>\n\nI've been able to reproduce the segfault.\nI have tested and sent the flush() series before Paul's metadata\nauto-resize series went in. The rebase of flush on top of the most\nrecent master requires a little change in [1/6]\n\ndiff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\nindex 075a8332f3b3..510fe0eae9ca 100644\n--- a/src/android/camera_device.cpp\n+++ b/src/android/camera_device.cpp\n@@ -2174,7 +2174,6 @@ void CameraDevice::requestComplete(Request *request)\n /* The camera framework expects an empy metadata pack on error. */\n resultMetadata = std::make_unique(0, 0);\n }\n- captureResult.result = resultMetadata->get();\n\n /* Handle any JPEG compression. */\n for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n@@ -2210,6 +2209,7 @@ void CameraDevice::requestComplete(Request *request)\n }\n }\n\n+ captureResult.result = resultMetadata->get();\n callbacks_->process_capture_result(callbacks_, &captureResult);\n }\n\nreason being that the new metadata auto-resize relocate the metadata\npack, hence when encoding is requested and new metadata are added to the\nresult pack, if it gets relocated the pointer assigned to\ncaptureResult.result is invalid.\n\nThe fix is trivial, it is enough to assign captureResult.result just\nafter the encoding phase.\n\nI'll re-send with this latest change.\n\nThanks\n j\n\n> -Hiro\n>\n> >\n> > Paul\n> >\n> > >\n> > >\n> > >\n> > > >\n> > > > There's some randomness in the RecordingTest, so that may not be\n> > > > significant. The other three tests seem to pass consistently in\n> > master,\n> > > > and fail consistently with the series applied. They also fail when\n> > run\n> > > > in isolation.\n> > > >\n> > > > > > The rest of the patch looks good to me.\n> > > > > >\n> > > > > > > +\n> > > > > > > worker_.queueRequest(descriptor.request_.get());\n> > > > > > >\n> > > > > > > {\n> > > > > > > diff --git a/src/android/camera_device.h b/src/android/\n> > > camera_device.h\n> > > > > > > index c949fa509ca4..4aadb27c562c 100644\n> > > > > > > --- a/src/android/camera_device.h\n> > > > > > > +++ b/src/android/camera_device.h\n> > > > > > > @@ -43,6 +43,7 @@ public:\n> > > > > > >\n> > > > > > > int open(const hw_module_t *hardwareModule);\n> > > > > > > void close();\n> > > > > > > + void flush();\n> > > > > > >\n> > > > > > > unsigned int id() const { return id_; }\n> > > > > > > camera3_device_t *camera3Device() { return\n> > &camera3Device_;\n> > > }\n> > > > > > > @@ -92,6 +93,7 @@ private:\n> > > > > > >\n> > > > > > > enum class State {\n> > > > > > > Stopped,\n> > > > > > > + Flushing,\n> > > > > > > Running,\n> > > > > > > };\n> > > > > > >\n> > > > > > > @@ -106,6 +108,7 @@ private:\n> > > > > > > getRawResolutions(const libcamera::PixelFormat &\n> > > pixelFormat);\n> > > > > > >\n> > > > > > > libcamera::FrameBuffer *createFrameBuffer(const\n> > > buffer_handle_t camera3buffer);\n> > > > > > > + void abortRequest(camera3_capture_request_t\n> > *request);\n> > > > > > > void notifyShutter(uint32_t frameNumber, uint64_t\n> > > timestamp);\n> > > > > > > void notifyError(uint32_t frameNumber,\n> > camera3_stream_t\n> > > *stream,\n> > > > > > > camera3_error_msg_code code);\n> > > > > > > diff --git a/src/android/camera_ops.cpp b/src/android/\n> > > camera_ops.cpp\n> > > > > > > index 696e80436821..8a3cfa175ff5 100644\n> > > > > > > --- a/src/android/camera_ops.cpp\n> > > > > > > +++ b/src/android/camera_ops.cpp\n> > > > > > > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]]\n> > const\n> > > struct camera3_device *dev,\n> > > > > > > {\n> > > > > > > }\n> > > > > > >\n> > > > > > > -static int hal_dev_flush([[maybe_unused]] const struct\n> > > camera3_device *dev)\n> > > > > > > +static int hal_dev_flush(const struct camera3_device *dev)\n> > > > > > > {\n> > > > > > > + if (!dev)\n> > > > > > > + return -EINVAL;\n> > > > > > > +\n> > > > > > > + CameraDevice *camera = reinterpret_cast > *>\n> > > (dev->priv);\n> > > > > > > + camera->flush();\n> > > > > > > +\n> > > > > > > return 0;\n> > > > > > > }\n> > > > > > >\n> > > >\n> > > > --\n> > > > Regards,\n> > > >\n> > > > Laurent Pinchart\n> > >\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 ACF89C3206\n\tfor ;\n\tTue, 8 Jun 2021 13:01:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDACA687EF;\n\tTue, 8 Jun 2021 15:01:51 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4271602A0\n\tfor ;\n\tTue, 8 Jun 2021 15:01:50 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 2A4E1C0006;\n\tTue, 8 Jun 2021 13:01:49 +0000 (UTC)"],"Date":"Tue, 8 Jun 2021 15:02:39 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210608130239.zclulz4ou36erqn3@uno.localdomain>","References":"<20210527220359.30127-1-jacopo@jmondi.org>\n\t<20210527220359.30127-9-jacopo@jmondi.org>\n\t\n\t\n\t\n\t<20210602043844.GF1929@pyrite.rasen.tech>\n\t\n\t<20210604092709.GE156622@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 8/8] android: Implement flush()\n\tcamera operation","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\" "}}]