[{"id":16542,"web_url":"https://patchwork.libcamera.org/comment/16542/","msgid":"<YIYLQ+D8RwooqIUu@pendragon.ideasonboard.com>","date":"2021-04-26T00:37:23","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote:\n> This implements flush(). It is mostly the same as close()\n> though the canceled events are reported with error messages.\n> \n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> ---\n>  src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------\n>  src/android/camera_device.h   |  1 +\n>  src/android/camera_ops.cpp    |  6 ++++-\n>  3 files changed, 40 insertions(+), 8 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index c74dc0e7..22a2e13c 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -753,6 +753,15 @@ void CameraDevice::close()\n>  \tstop(/*releaseCamera=*/true);\n>  }\n>  \n> +int CameraDevice::flush()\n> +{\n> +\tstd::scoped_lock<std::mutex> lock(mutex_);\n> +\n> +\tstop();\n> +\n> +\treturn 0;\n> +}\n> +\n>  void CameraDevice::stop(bool releaseCamera)\n>  {\n>  \tstreams_.clear();\n> @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera)\n>  \tworker_.stop();\n>  \tcamera_->stop();\n>  \n> +\tfor (auto &[cookie, descriptor] : descriptors_) {\n> +\t\tLOG(HAL, Debug) << \"request is canceled: \" << cookie;\n\n\t\tLOG(HAL, Debug) << \"request \" << cookie << \" is canceled\";\n\n> +\n> +\t\tcamera3_capture_result_t captureResult = {};\n> +\t\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\t\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> +\t\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\t\tbuffer.acquire_fence = -1;\n> +\t\t\tbuffer.release_fence = -1;\n> +\t\t\tbuffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> +\t\t}\n> +\t\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\n> +\t\tnotifyError(descriptor.frameNumber_,\n> +\t\t\t    descriptor.buffers_[0].stream);\n> +\t\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n\nThis looks very similar to the code in CameraDevice::requestComplete(),\nwould it make sense to move it to a separate function (including the\ncall to notifyError) ?\n\n> +\t}\n>  \tdescriptors_.clear();\n>  \n>  \trunning_ = false;\n> @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request)\n>  \t}\n>  \n>  \tif (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> +\t\t/*\n> +\t\t * \\todo Report and identify the stream number or configuration\n> +\t\t * to clarify the stream that failed.\n> +\t\t */\n> +\t\tLOG(HAL, Error)\n> +\t\t\t<< \"Error occurred on frame \" << descriptor.frameNumber_ << \" (\"\n> +\t\t\t<< toPixelFormat(descriptor.buffers_[0].stream->format).toString() << \")\";\n> +\n>  \t\t/* \\todo Improve error handling. In case we notify an error\n>  \t\t * because the metadata generation fails, a shutter event has\n>  \t\t * already been notified for this frame number before the error\n> @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n>  {\n>  \tcamera3_notify_msg_t notify = {};\n>  \n> -\t/*\n> -\t * \\todo Report and identify the stream number or configuration to\n> -\t * clarify the stream that failed.\n> -\t */\n> -\tLOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> -\t\t\t<< toPixelFormat(stream->format).toString() << \")\";\n> -\n>  \tnotify.type = CAMERA3_MSG_ERROR;\n>  \tnotify.message.error.error_stream = stream;\n>  \tnotify.message.error.frame_number = frameNumber;\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 08553584..7004c89a 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -41,6 +41,7 @@ public:\n>  \n>  \tint open(const hw_module_t *hardwareModule);\n>  \tvoid close();\n> +\tint flush();\n>  \n>  \tunsigned int id() const { return id_; }\n>  \tcamera3_device_t *camera3Device() { return &camera3Device_; }\n> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> index 696e8043..981a3d30 100644\n> --- a/src/android/camera_ops.cpp\n> +++ b/src/android/camera_ops.cpp\n> @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n>  \n>  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n\nYou can drop [[maybe_unused]].\n\nIt would be nice to have an [[unused]] that causes a warning when the\nvariable is used.\n\n>  {\n> -\treturn 0;\n> +\tif (!dev)\n> +\t\treturn -EINVAL;\n> +\n> +\tCameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);\n> +\treturn camera->flush();\n>  }\n>  \n>  int hal_dev_close(hw_device_t *hw_device)","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 259B8BDC92\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 26 Apr 2021 00:37:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6480668882;\n\tMon, 26 Apr 2021 02:37:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD0ED6887C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 02:37:28 +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 230114FB;\n\tMon, 26 Apr 2021 02:37:28 +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=\"DOOKdZcY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619397448;\n\tbh=c9Eo80HsxFYAXIU34lTTnN7B7HNjVif48F14iurjBHU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DOOKdZcYPa5edVuQAR2vTq0YaFsS6BxWvIAksez3mAVEFbBjG1KL3lAtt75Yhu0y1\n\tYdcsf4wi5ZljIsc3RY0RKnS6jszMeaAGMudUsoHr2Hmmy184mpWVLHli9wGM1gZj00\n\tbNIDtKZ/OZ5yU4KIFK82v4E0TdAlzXQQzGx3DplI=","Date":"Mon, 26 Apr 2021 03:37:23 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YIYLQ+D8RwooqIUu@pendragon.ideasonboard.com>","References":"<20210423040738.1227220-1-hiroh@chromium.org>\n\t<20210423040738.1227220-3-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210423040738.1227220-3-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16549,"web_url":"https://patchwork.libcamera.org/comment/16549/","msgid":"<CAO5uPHNRfWdsHHnL-OCSThrYC4aDwF9MgyTUCmkZXDbkicGNeQ@mail.gmail.com>","date":"2021-04-26T02:51:40","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote:\n> > This implements flush(). It is mostly the same as close()\n> > though the canceled events are reported with error messages.\n> >\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > ---\n> >  src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------\n> >  src/android/camera_device.h   |  1 +\n> >  src/android/camera_ops.cpp    |  6 ++++-\n> >  3 files changed, 40 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index c74dc0e7..22a2e13c 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -753,6 +753,15 @@ void CameraDevice::close()\n> >       stop(/*releaseCamera=*/true);\n> >  }\n> >\n> > +int CameraDevice::flush()\n> > +{\n> > +     std::scoped_lock<std::mutex> lock(mutex_);\n> > +\n> > +     stop();\n> > +\n> > +     return 0;\n> > +}\n> > +\n> >  void CameraDevice::stop(bool releaseCamera)\n> >  {\n> >       streams_.clear();\n> > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera)\n> >       worker_.stop();\n> >       camera_->stop();\n> >\n> > +     for (auto &[cookie, descriptor] : descriptors_) {\n> > +             LOG(HAL, Debug) << \"request is canceled: \" << cookie;\n>\n>                 LOG(HAL, Debug) << \"request \" << cookie << \" is canceled\";\n>\n> > +\n> > +             camera3_capture_result_t captureResult = {};\n> > +             captureResult.frame_number = descriptor.frameNumber_;\n> > +             captureResult.num_output_buffers = descriptor.buffers_.size();\n> > +             for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > +                     buffer.acquire_fence = -1;\n> > +                     buffer.release_fence = -1;\n> > +                     buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > +             }\n> > +             captureResult.output_buffers = descriptor.buffers_.data();\n> > +\n> > +             notifyError(descriptor.frameNumber_,\n> > +                         descriptor.buffers_[0].stream);\n> > +             callbacks_->process_capture_result(callbacks_, &captureResult);\n>\n> This looks very similar to the code in CameraDevice::requestComplete(),\n> would it make sense to move it to a separate function (including the\n> call to notifyError) ?\n>\n\nSure, I will factorize in the next patch.\nMy first thought is to reduce the number of patches in the series and\nfactorize them after the patches are merged to not complicate this\npatch series any more.\n\n> > +     }\n> >       descriptors_.clear();\n> >\n> >       running_ = false;\n> > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request)\n> >       }\n> >\n> >       if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> > +             /*\n> > +              * \\todo Report and identify the stream number or configuration\n> > +              * to clarify the stream that failed.\n> > +              */\n> > +             LOG(HAL, Error)\n> > +                     << \"Error occurred on frame \" << descriptor.frameNumber_ << \" (\"\n> > +                     << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << \")\";\n> > +\n> >               /* \\todo Improve error handling. In case we notify an error\n> >                * because the metadata generation fails, a shutter event has\n> >                * already been notified for this frame number before the error\n> > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> >  {\n> >       camera3_notify_msg_t notify = {};\n> >\n> > -     /*\n> > -      * \\todo Report and identify the stream number or configuration to\n> > -      * clarify the stream that failed.\n> > -      */\n> > -     LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> > -                     << toPixelFormat(stream->format).toString() << \")\";\n> > -\n> >       notify.type = CAMERA3_MSG_ERROR;\n> >       notify.message.error.error_stream = stream;\n> >       notify.message.error.frame_number = frameNumber;\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 08553584..7004c89a 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -41,6 +41,7 @@ public:\n> >\n> >       int open(const hw_module_t *hardwareModule);\n> >       void close();\n> > +     int flush();\n> >\n> >       unsigned int id() const { return id_; }\n> >       camera3_device_t *camera3Device() { return &camera3Device_; }\n> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > index 696e8043..981a3d30 100644\n> > --- a/src/android/camera_ops.cpp\n> > +++ b/src/android/camera_ops.cpp\n> > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> >\n> >  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n>\n> You can drop [[maybe_unused]].\n>\n> It would be nice to have an [[unused]] that causes a warning when the\n> variable is used.\n>\n> >  {\n> > -     return 0;\n> > +     if (!dev)\n> > +             return -EINVAL;\n> > +\n> > +     CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);\n> > +     return camera->flush();\n> >  }\n> >\n> >  int hal_dev_close(hw_device_t *hw_device)\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 00319BDC91\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 26 Apr 2021 02:51:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5291B68881;\n\tMon, 26 Apr 2021 04:51:51 +0200 (CEST)","from mail-ej1-x62a.google.com (mail-ej1-x62a.google.com\n\t[IPv6:2a00:1450:4864:20::62a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8795B605BD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 04:51:50 +0200 (CEST)","by mail-ej1-x62a.google.com with SMTP id r12so82154626ejr.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 25 Apr 2021 19:51:50 -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=\"YDpr1/Eh\"; 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=Y8nb3rQq7v1VQqSWlmMdK30HBJoc7rzxZ2sTR0L0JDo=;\n\tb=YDpr1/Eh9OGyu0bzqkM/ukx07hoE8F/hK5thrdeAydomWTTovE/7b6RBTMURyXsOWH\n\t9w2zamlz5Kvgzd/PIWhdg1fjeCwZgrLxH337vBMyyER3+Bdo0Wgl0guDCc+OKznZB8hw\n\tEiAU84n/Jj9LBjb1l8q/NiMga9tCglb6p4J0Q=","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=Y8nb3rQq7v1VQqSWlmMdK30HBJoc7rzxZ2sTR0L0JDo=;\n\tb=h1YkoEB16gV3DBBPaLDxXCufYKv9GHqUmsjqbMyFuRHL+OUiylDoHcxfe9k4JwQVeG\n\t4phR7kmzskiQOW9BpfRiun65M4qO48LiS+Y0aJC3UHuSm4+isGUKfW/06xQGpiCaq0az\n\tJnXO0TYutZXOR3pCLZZB5zr17DEu/Y4G13lEoVBaJywWE0OoNtOPzEnn5XH6Sv5FWN24\n\tXsRN00Oj5lwmPLWLe8lTdh+s+Uni2FiGmzgMdpyo+iCXG1Bfp5OsD5+qqev9d6wOGC7H\n\tr0jzJ9BZHAZ96ihWUclFFCmGKt/ShnKZnoU8DJsMAMTOVyVIePGHnDJkj5n4w5n1e7X9\n\tgbNQ==","X-Gm-Message-State":"AOAM5304MoN2mLN9OVKDAhEYUvG7rqJ9dmyg8yAVFqDtpnn1bM18U+Fs\n\tTmGZVaRig/VHmRSyjRuo4i0jEcEfHky4GqmVTIk7cA==","X-Google-Smtp-Source":"ABdhPJwJAdaBUZ66pruevEoQtUwVu4lpeRqvyuGM7cFkNohc3TL9eU+IhPpA2/uxhxZStPXhqXXNRvQ9rphVJLifGy8=","X-Received":"by 2002:a17:906:13d6:: with SMTP id\n\tg22mr16125853ejc.475.1619405510224; \n\tSun, 25 Apr 2021 19:51:50 -0700 (PDT)","MIME-Version":"1.0","References":"<20210423040738.1227220-1-hiroh@chromium.org>\n\t<20210423040738.1227220-3-hiroh@chromium.org>\n\t<YIYLQ+D8RwooqIUu@pendragon.ideasonboard.com>","In-Reply-To":"<YIYLQ+D8RwooqIUu@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Mon, 26 Apr 2021 11:51:40 +0900","Message-ID":"<CAO5uPHNRfWdsHHnL-OCSThrYC4aDwF9MgyTUCmkZXDbkicGNeQ@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16552,"web_url":"https://patchwork.libcamera.org/comment/16552/","msgid":"<YIYszDuAbmjv/OJD@pendragon.ideasonboard.com>","date":"2021-04-26T03:00:28","subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Apr 26, 2021 at 11:51:40AM +0900, Hirokazu Honda wrote:\n> On Mon, Apr 26, 2021 at 9:37 AM Laurent Pinchart wrote:\n> > On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote:\n> > > This implements flush(). It is mostly the same as close()\n> > > though the canceled events are reported with error messages.\n> > >\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > ---\n> > >  src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------\n> > >  src/android/camera_device.h   |  1 +\n> > >  src/android/camera_ops.cpp    |  6 ++++-\n> > >  3 files changed, 40 insertions(+), 8 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index c74dc0e7..22a2e13c 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -753,6 +753,15 @@ void CameraDevice::close()\n> > >       stop(/*releaseCamera=*/true);\n> > >  }\n> > >\n> > > +int CameraDevice::flush()\n> > > +{\n> > > +     std::scoped_lock<std::mutex> lock(mutex_);\n> > > +\n> > > +     stop();\n> > > +\n> > > +     return 0;\n> > > +}\n> > > +\n> > >  void CameraDevice::stop(bool releaseCamera)\n> > >  {\n> > >       streams_.clear();\n> > > @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera)\n> > >       worker_.stop();\n> > >       camera_->stop();\n> > >\n> > > +     for (auto &[cookie, descriptor] : descriptors_) {\n> > > +             LOG(HAL, Debug) << \"request is canceled: \" << cookie;\n> >\n> >                 LOG(HAL, Debug) << \"request \" << cookie << \" is canceled\";\n> >\n> > > +\n> > > +             camera3_capture_result_t captureResult = {};\n> > > +             captureResult.frame_number = descriptor.frameNumber_;\n> > > +             captureResult.num_output_buffers = descriptor.buffers_.size();\n> > > +             for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> > > +                     buffer.acquire_fence = -1;\n> > > +                     buffer.release_fence = -1;\n> > > +                     buffer.status = CAMERA3_BUFFER_STATUS_ERROR;\n> > > +             }\n> > > +             captureResult.output_buffers = descriptor.buffers_.data();\n> > > +\n> > > +             notifyError(descriptor.frameNumber_,\n> > > +                         descriptor.buffers_[0].stream);\n> > > +             callbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n> > This looks very similar to the code in CameraDevice::requestComplete(),\n> > would it make sense to move it to a separate function (including the\n> > call to notifyError) ?\n> \n> Sure, I will factorize in the next patch.\n> My first thought is to reduce the number of patches in the series and\n> factorize them after the patches are merged to not complicate this\n> patch series any more.\n\nI actually makes review easier if you factor out the code in a separate\nfunction in a patch of its own, before this patch.\n\n> > > +     }\n> > >       descriptors_.clear();\n> > >\n> > >       running_ = false;\n> > > @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request)\n> > >       }\n> > >\n> > >       if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\n> > > +             /*\n> > > +              * \\todo Report and identify the stream number or configuration\n> > > +              * to clarify the stream that failed.\n> > > +              */\n> > > +             LOG(HAL, Error)\n> > > +                     << \"Error occurred on frame \" << descriptor.frameNumber_ << \" (\"\n> > > +                     << toPixelFormat(descriptor.buffers_[0].stream->format).toString() << \")\";\n> > > +\n> > >               /* \\todo Improve error handling. In case we notify an error\n> > >                * because the metadata generation fails, a shutter event has\n> > >                * already been notified for this frame number before the error\n> > > @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)\n> > >  {\n> > >       camera3_notify_msg_t notify = {};\n> > >\n> > > -     /*\n> > > -      * \\todo Report and identify the stream number or configuration to\n> > > -      * clarify the stream that failed.\n> > > -      */\n> > > -     LOG(HAL, Error) << \"Error occurred on frame \" << frameNumber << \" (\"\n> > > -                     << toPixelFormat(stream->format).toString() << \")\";\n> > > -\n> > >       notify.type = CAMERA3_MSG_ERROR;\n> > >       notify.message.error.error_stream = stream;\n> > >       notify.message.error.frame_number = frameNumber;\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index 08553584..7004c89a 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -41,6 +41,7 @@ public:\n> > >\n> > >       int open(const hw_module_t *hardwareModule);\n> > >       void close();\n> > > +     int flush();\n> > >\n> > >       unsigned int id() const { return id_; }\n> > >       camera3_device_t *camera3Device() { return &camera3Device_; }\n> > > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp\n> > > index 696e8043..981a3d30 100644\n> > > --- a/src/android/camera_ops.cpp\n> > > +++ b/src/android/camera_ops.cpp\n> > > @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,\n> > >\n> > >  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)\n> >\n> > You can drop [[maybe_unused]].\n> >\n> > It would be nice to have an [[unused]] that causes a warning when the\n> > variable is used.\n> >\n> > >  {\n> > > -     return 0;\n> > > +     if (!dev)\n> > > +             return -EINVAL;\n> > > +\n> > > +     CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);\n> > > +     return camera->flush();\n> > >  }\n> > >\n> > >  int hal_dev_close(hw_device_t *hw_device)","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 97358BDC91\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 26 Apr 2021 03:00:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 587B368881;\n\tMon, 26 Apr 2021 05:00:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 846A2605BD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 26 Apr 2021 05:00:33 +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 246554FB;\n\tMon, 26 Apr 2021 05:00:33 +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=\"kl6bBvlJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619406033;\n\tbh=jsyGcM9VA+5xnjLqAdjVrGJEeCyB5qvbtCmcqrm2sAw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kl6bBvlJUG77JqNrlgcsmD81GOS8wrqw2kNHkVcPJ94rUBe+RGkNivlmvXL7uR5r9\n\txr0IS+gukkolS/CjgQittv4YOoTMgrFLz0ldSErw9CW9171u81Ys6mgnQYqW9RP/bT\n\tGg/JmOyoSXlq5CfQ5azEZlgRUepnLgxCIcs1Q0o4=","Date":"Mon, 26 Apr 2021 06:00:28 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YIYszDuAbmjv/OJD@pendragon.ideasonboard.com>","References":"<20210423040738.1227220-1-hiroh@chromium.org>\n\t<20210423040738.1227220-3-hiroh@chromium.org>\n\t<YIYLQ+D8RwooqIUu@pendragon.ideasonboard.com>\n\t<CAO5uPHNRfWdsHHnL-OCSThrYC4aDwF9MgyTUCmkZXDbkicGNeQ@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHNRfWdsHHnL-OCSThrYC4aDwF9MgyTUCmkZXDbkicGNeQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] android: CameraDevice:\n\tImplement HAL3 API flush","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]