[{"id":16565,"web_url":"https://patchwork.libcamera.org/comment/16565/","msgid":"","date":"2021-04-26T05:28:44","subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","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 Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:\n> This factorizes a code of generating camera3_capture_result. It\n> will be useful to report capture result in other places than\n> CameraDevice::reqeustComplete().\n> \n> Signed-off-by: Hirokazu Honda \n> \n> ---\n> I was going to implement\n> reportShutter() - createCaptureResult() + notifyShutter(), and\n> reportError() - createCaptureResult() + notifyError().\n> \n> But reportError() can be called after reportShutter(). It is a\n> bit less efficient to create capture result twice in the case.\n> So I only factorize a code of generating a capture result.\n\nCan't we have a single function to do all that ? Right now we have\n\n\t/* Prepare to call back the Android camera stack. */\n\tcamera3_capture_result_t captureResult = {};\n\tcaptureResult.frame_number = descriptor.frameNumber_;\n\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n\t\tbuffer.acquire_fence = -1;\n\t\tbuffer.release_fence = -1;\n\t\tbuffer.status = status;\n\t}\n\tcaptureResult.output_buffers = descriptor.buffers_.data();\n\n\tif (status == CAMERA3_BUFFER_STATUS_OK) {\n\t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n\n\t\tcaptureResult.partial_result = 1;\n\t\tcaptureResult.result = resultMetadata->get();\n\t}\n\n\tif (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\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\t\t * is here signalled. Make sure the error path plays well with\n\t\t * the camera stack state machine.\n\t\t */\n\t\tnotifyError(descriptor.frameNumber_,\n\t\t\t descriptor.buffers_[0].stream);\n\t}\n\n\tcallbacks_->process_capture_result(callbacks_, &captureResult);\n\nCould we move all that to a function that takes descriptor, status,\ntimestamp and resultMedadata as arguments ? In stop(), to implement\nflush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,\nso notifyShutter() would be skipped.\n\nIf you think that would cause issues, this patch is already an\nimprovement, so we could proceed with it.\n\n> ---\n> src/android/camera_device.cpp | 29 ++++++++++++++++++++---------\n> src/android/camera_device.h | 3 +++\n> 2 files changed, 23 insertions(+), 9 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index a71aee2f..ced8efcc 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)\n> \t}\n> \n> \t/* Prepare to call back the Android camera stack. */\n> -\tcamera3_capture_result_t captureResult = {};\n> -\tcaptureResult.frame_number = descriptor.frameNumber_;\n> -\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> -\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> -\t\tbuffer.acquire_fence = -1;\n> -\t\tbuffer.release_fence = -1;\n> -\t\tbuffer.status = status;\n> -\t}\n> -\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\tcamera3_capture_result_t captureResult =\n> +\t\tcreateCaptureResult(descriptor, status);\n> \n> \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> \t\tnotifyShutter(descriptor.frameNumber_, timestamp);\n> @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const\n> \treturn \"'\" + camera_->id() + \"'\";\n> }\n> \n> +\n> +camera3_capture_result_t CameraDevice::createCaptureResult(\n> +\tCamera3RequestDescriptor &descriptor,\n> +\tcamera3_buffer_status status) const\n> +{\n> +\tcamera3_capture_result_t captureResult = {};\n> +\tcaptureResult.frame_number = descriptor.frameNumber_;\n> +\tcaptureResult.num_output_buffers = descriptor.buffers_.size();\n> +\tfor (camera3_stream_buffer_t &buffer : descriptor.buffers_) {\n> +\t\tbuffer.acquire_fence = -1;\n> +\t\tbuffer.release_fence = -1;\n> +\t\tbuffer.status = status;\n> +\t}\n> +\tcaptureResult.output_buffers = descriptor.buffers_.data();\n> +\n> +\treturn captureResult;\n> +}\n> +\n> void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> {\n> \tcamera3_notify_msg_t notify = {};\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index c63e8e21..a1abcead 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -100,6 +100,9 @@ private:\n> \n> \tstd::tuple calculateStaticMetadataSize();\n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> +\tcamera3_capture_result_t createCaptureResult(\n> +\t\tCamera3RequestDescriptor &descriptor,\n> +\t\tcamera3_buffer_status status) const;\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> \tstd::unique_ptr requestTemplatePreview();","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 D4601BDC92\n\tfor ;\n\tMon, 26 Apr 2021 05:28:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 459E8688AC;\n\tMon, 26 Apr 2021 07:28:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3E95B688A0\n\tfor ;\n\tMon, 26 Apr 2021 07:28:50 +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 C8A884FB;\n\tMon, 26 Apr 2021 07:28: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=\"ntPn8rEm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619414930;\n\tbh=0OigCna+KGAwRuoMiquL/IimzBrIep6Xql6zmMlGyn8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ntPn8rEmINF9ANP+90S8s5ekmdNXBNj4Z0Q1T5TxApG5/WDkjTccSmIX8G4bvy4gU\n\t55DGvFqQ2p0zp/mz8org9WbEFlKUElsg+J1lMqXnkymQKDtcwdeEzqkGis4pDppcvj\n\tkT4Ggl1tsfWtkop1XdrsgEqCFGosR2mAYPJNiOI0=","Date":"Mon, 26 Apr 2021 08:28:44 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210426034502.2270770-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210426034502.2270770-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16577,"web_url":"https://patchwork.libcamera.org/comment/16577/","msgid":"","date":"2021-04-26T09:08:10","subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","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 2:28 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:\n> > This factorizes a code of generating camera3_capture_result. It\n> > will be useful to report capture result in other places than\n> > CameraDevice::reqeustComplete().\n> >\n> > Signed-off-by: Hirokazu Honda \n> >\n> > ---\n> > I was going to implement\n> > reportShutter() - createCaptureResult() + notifyShutter(), and\n> > reportError() - createCaptureResult() + notifyError().\n> >\n> > But reportError() can be called after reportShutter(). It is a\n> > bit less efficient to create capture result twice in the case.\n> > So I only factorize a code of generating a capture result.\n>\n> Can't we have a single function to do all that ? Right now we have\n>\n> /* Prepare to call back the Android camera stack. */\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 = status;\n> }\n> captureResult.output_buffers = descriptor.buffers_.data();\n>\n> if (status == CAMERA3_BUFFER_STATUS_OK) {\n> notifyShutter(descriptor.frameNumber_, timestamp);\n>\n> captureResult.partial_result = 1;\n> captureResult.result = resultMetadata->get();\n> }\n>\n> if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\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> * is here signalled. Make sure the error path plays well with\n> * the camera stack state machine.\n> */\n> notifyError(descriptor.frameNumber_,\n> descriptor.buffers_[0].stream);\n> }\n>\n> callbacks_->process_capture_result(callbacks_, &captureResult);\n>\n> Could we move all that to a function that takes descriptor, status,\n> timestamp and resultMedadata as arguments ? In stop(), to implement\n> flush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,\n> so notifyShutter() would be skipped.\n>\n> If you think that would cause issues, this patch is already an\n> improvement, so we could proceed with it.\n>\n\nI did so some times ago when I worked with Jacopo for Flush().\nJacopo suggested to not not contain a code for shutter and error in\nthe same function, as either of them are not executed.\nJacopo, how do you think?\n\n-Hiro\n\n> > ---\n> > src/android/camera_device.cpp | 29 ++++++++++++++++++++---------\n> > src/android/camera_device.h | 3 +++\n> > 2 files changed, 23 insertions(+), 9 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index a71aee2f..ced8efcc 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)\n> > }\n> >\n> > /* Prepare to call back the Android camera stack. */\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 = status;\n> > - }\n> > - captureResult.output_buffers = descriptor.buffers_.data();\n> > + camera3_capture_result_t captureResult =\n> > + createCaptureResult(descriptor, status);\n> >\n> > if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > notifyShutter(descriptor.frameNumber_, timestamp);\n> > @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const\n> > return \"'\" + camera_->id() + \"'\";\n> > }\n> >\n> > +\n> > +camera3_capture_result_t CameraDevice::createCaptureResult(\n> > + Camera3RequestDescriptor &descriptor,\n> > + camera3_buffer_status status) const\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 = status;\n> > + }\n> > + captureResult.output_buffers = descriptor.buffers_.data();\n> > +\n> > + return captureResult;\n> > +}\n> > +\n> > void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > {\n> > camera3_notify_msg_t notify = {};\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index c63e8e21..a1abcead 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -100,6 +100,9 @@ private:\n> >\n> > std::tuple calculateStaticMetadataSize();\n> > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > + camera3_capture_result_t createCaptureResult(\n> > + Camera3RequestDescriptor &descriptor,\n> > + camera3_buffer_status status) const;\n> > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > std::unique_ptr requestTemplatePreview();\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 97ABBBDC9A\n\tfor ;\n\tMon, 26 Apr 2021 09:08:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0D82968882;\n\tMon, 26 Apr 2021 11:08:24 +0200 (CEST)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C173E60516\n\tfor ;\n\tMon, 26 Apr 2021 11:08:21 +0200 (CEST)","by mail-ed1-x52b.google.com with SMTP id y3so28526071eds.5\n\tfor ;\n\tMon, 26 Apr 2021 02:08:21 -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=\"acC8btv+\"; 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=3paBkhuEtVmzKEe5DizwHXbmAaeiNdriqKD6TjxJ/fk=;\n\tb=acC8btv+p4qZlSx0HPPinX/+UF49sgFlboiR61YnTDUGdr5O6yuKCj67GZ+ORVqUPE\n\tNgqx5TML5LaRWs35HT17xivKCZADAhDzKL+/FhConFvKOAo+hPYc/QbRWNW//Ao1nN4j\n\t/jzCUqP0Xwobf2AEgey6IQpsUatiiH6eFYzjc=","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=3paBkhuEtVmzKEe5DizwHXbmAaeiNdriqKD6TjxJ/fk=;\n\tb=ekZCnlHn0eQqAxQswCP+uMhizo2IvcPXIYnnOw1Eno/3i6Bn1yK7OjvVvpIv265XUE\n\t1rivjK1sNhEOclX4EKTS6AIz/Pz4eh75An/ceeWsRQc/tXGOG5ISfix0IWviH8qvEz/q\n\t9x/FdWz43pl1JJToCtCGY+ById2nss9xeqpxvgH3GPpoV7eEtgO3zbfW7c8WTF7khwZT\n\t3M6zIYwp2mD1u9XiaZMS5XynUtrieaowsmI1QAm6a4YIG0dzFsXtwDFaMBhnQA+w0dgp\n\tsm3DIH+JZc1FArypptfjTn2t0cGtsPwousG5iCT8O9FyTQkHXkbEJtUDDe58IIs+mBaN\n\tkuJw==","X-Gm-Message-State":"AOAM531M7iZlRZrz8cFLUHltOa28ToZ+Nr2OQies9m0N2vSRcrjVfnjS\n\tey9qe8lUuJgJumUtLEOvSfnxqk9uxo+D03t5eqlt0Zl9duA=","X-Google-Smtp-Source":"ABdhPJyeD3AR6iEcZzSYgc6hvX9dwv3P+TIHgAowkxSegAOmygVbf8EbxIhFqm5IIuOhK+JB59BN7BSoddigdEr8TYg=","X-Received":"by 2002:aa7:c7da:: with SMTP id\n\to26mr19904109eds.244.1619428101369; \n\tMon, 26 Apr 2021 02:08:21 -0700 (PDT)","MIME-Version":"1.0","References":"<20210426034502.2270770-1-hiroh@chromium.org>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 26 Apr 2021 18:08:10 +0900","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16584,"web_url":"https://patchwork.libcamera.org/comment/16584/","msgid":"<20210426103932.lj7lgqpipkl3ynsd@uno.localdomain>","date":"2021-04-26T10:39:32","subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Mon, Apr 26, 2021 at 06:08:10PM +0900, Hirokazu Honda wrote:\n> Hi Laurent,\n>\n> On Mon, Apr 26, 2021 at 2:28 PM Laurent Pinchart\n> wrote:\n> >\n> > Hi Hiro,\n> >\n> > Thank you for the patch.\n> >\n> > On Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:\n> > > This factorizes a code of generating camera3_capture_result. It\n> > > will be useful to report capture result in other places than\n> > > CameraDevice::reqeustComplete().\n> > >\n> > > Signed-off-by: Hirokazu Honda \n> > >\n> > > ---\n> > > I was going to implement\n> > > reportShutter() - createCaptureResult() + notifyShutter(), and\n> > > reportError() - createCaptureResult() + notifyError().\n> > >\n> > > But reportError() can be called after reportShutter(). It is a\n> > > bit less efficient to create capture result twice in the case.\n> > > So I only factorize a code of generating a capture result.\n> >\n> > Can't we have a single function to do all that ? Right now we have\n> >\n> > /* Prepare to call back the Android camera stack. */\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 = status;\n> > }\n> > captureResult.output_buffers = descriptor.buffers_.data();\n> >\n> > if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > notifyShutter(descriptor.frameNumber_, timestamp);\n> >\n> > captureResult.partial_result = 1;\n> > captureResult.result = resultMetadata->get();\n> > }\n> >\n> > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\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> > * is here signalled. Make sure the error path plays well with\n> > * the camera stack state machine.\n> > */\n> > notifyError(descriptor.frameNumber_,\n> > descriptor.buffers_[0].stream);\n> > }\n> >\n> > callbacks_->process_capture_result(callbacks_, &captureResult);\n> >\n> > Could we move all that to a function that takes descriptor, status,\n> > timestamp and resultMedadata as arguments ? In stop(), to implement\n> > flush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,\n> > so notifyShutter() would be skipped.\n> >\n> > If you think that would cause issues, this patch is already an\n> > improvement, so we could proceed with it.\n> >\n>\n> I did so some times ago when I worked with Jacopo for Flush().\n> Jacopo suggested to not not contain a code for shutter and error in\n> the same function, as either of them are not executed.\n> Jacopo, how do you think?\n>\n\nIt's terribily hard to have a sense of how a patch is used without\ncontext with all the pending work we have in flux.\n\nIf I recall correctly, in the context of the flush() implementation we\ndiscussed, was that having all in one function was not that beneficial\nas notifications will have to be sent to the framework asynchronously\nin the long term, and part of the function was dead code.\n\nAgain, memory can fail me, there's really a lot of things moving and\nkeep the full picture in mind is hard.\n\nThanks\n j\n\n> -Hiro\n>\n> > > ---\n> > > src/android/camera_device.cpp | 29 ++++++++++++++++++++---------\n> > > src/android/camera_device.h | 3 +++\n> > > 2 files changed, 23 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index a71aee2f..ced8efcc 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)\n> > > }\n> > >\n> > > /* Prepare to call back the Android camera stack. */\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 = status;\n> > > - }\n> > > - captureResult.output_buffers = descriptor.buffers_.data();\n> > > + camera3_capture_result_t captureResult =\n> > > + createCaptureResult(descriptor, status);\n> > >\n> > > if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > notifyShutter(descriptor.frameNumber_, timestamp);\n> > > @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const\n> > > return \"'\" + camera_->id() + \"'\";\n> > > }\n> > >\n> > > +\n> > > +camera3_capture_result_t CameraDevice::createCaptureResult(\n> > > + Camera3RequestDescriptor &descriptor,\n> > > + camera3_buffer_status status) const\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 = status;\n> > > + }\n> > > + captureResult.output_buffers = descriptor.buffers_.data();\n> > > +\n> > > + return captureResult;\n> > > +}\n> > > +\n> > > void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > > {\n> > > camera3_notify_msg_t notify = {};\n> > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > index c63e8e21..a1abcead 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -100,6 +100,9 @@ private:\n> > >\n> > > std::tuple calculateStaticMetadataSize();\n> > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > + camera3_capture_result_t createCaptureResult(\n> > > + Camera3RequestDescriptor &descriptor,\n> > > + camera3_buffer_status status) const;\n> > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > std::unique_ptr requestTemplatePreview();\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 0E841BDC9C\n\tfor ;\n\tMon, 26 Apr 2021 10:38:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA3EB688AF;\n\tMon, 26 Apr 2021 12:38:53 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 45B4E605BE\n\tfor ;\n\tMon, 26 Apr 2021 12:38:52 +0200 (CEST)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 8C7D3100006;\n\tMon, 26 Apr 2021 10:38:51 +0000 (UTC)"],"Date":"Mon, 26 Apr 2021 12:39:32 +0200","From":"Jacopo Mondi ","To":"Hirokazu Honda ","Message-ID":"<20210426103932.lj7lgqpipkl3ynsd@uno.localdomain>","References":"<20210426034502.2270770-1-hiroh@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":16587,"web_url":"https://patchwork.libcamera.org/comment/16587/","msgid":"","date":"2021-04-26T11:01:11","subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Jacopo,\n\nOn Mon, Apr 26, 2021 at 7:38 PM Jacopo Mondi wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Apr 26, 2021 at 06:08:10PM +0900, Hirokazu Honda wrote:\n> > Hi Laurent,\n> >\n> > On Mon, Apr 26, 2021 at 2:28 PM Laurent Pinchart\n> > wrote:\n> > >\n> > > Hi Hiro,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Mon, Apr 26, 2021 at 12:45:02PM +0900, Hirokazu Honda wrote:\n> > > > This factorizes a code of generating camera3_capture_result. It\n> > > > will be useful to report capture result in other places than\n> > > > CameraDevice::reqeustComplete().\n> > > >\n> > > > Signed-off-by: Hirokazu Honda \n> > > >\n> > > > ---\n> > > > I was going to implement\n> > > > reportShutter() - createCaptureResult() + notifyShutter(), and\n> > > > reportError() - createCaptureResult() + notifyError().\n> > > >\n> > > > But reportError() can be called after reportShutter(). It is a\n> > > > bit less efficient to create capture result twice in the case.\n> > > > So I only factorize a code of generating a capture result.\n> > >\n> > > Can't we have a single function to do all that ? Right now we have\n> > >\n> > > /* Prepare to call back the Android camera stack. */\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 = status;\n> > > }\n> > > captureResult.output_buffers = descriptor.buffers_.data();\n> > >\n> > > if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > notifyShutter(descriptor.frameNumber_, timestamp);\n> > >\n> > > captureResult.partial_result = 1;\n> > > captureResult.result = resultMetadata->get();\n> > > }\n> > >\n> > > if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {\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> > > * is here signalled. Make sure the error path plays well with\n> > > * the camera stack state machine.\n> > > */\n> > > notifyError(descriptor.frameNumber_,\n> > > descriptor.buffers_[0].stream);\n> > > }\n> > >\n> > > callbacks_->process_capture_result(callbacks_, &captureResult);\n> > >\n> > > Could we move all that to a function that takes descriptor, status,\n> > > timestamp and resultMedadata as arguments ? In stop(), to implement\n> > > flush(), it would be called with status == CAMERA3_BUFFER_STATUS_ERROR,\n> > > so notifyShutter() would be skipped.\n> > >\n> > > If you think that would cause issues, this patch is already an\n> > > improvement, so we could proceed with it.\n> > >\n> >\n> > I did so some times ago when I worked with Jacopo for Flush().\n> > Jacopo suggested to not not contain a code for shutter and error in\n> > the same function, as either of them are not executed.\n> > Jacopo, how do you think?\n> >\n>\n> It's terribily hard to have a sense of how a patch is used without\n> context with all the pending work we have in flux.\n>\n> If I recall correctly, in the context of the flush() implementation we\n> discussed, was that having all in one function was not that beneficial\n> as notifications will have to be sent to the framework asynchronously\n> in the long term, and part of the function was dead code.\n>\n> Again, memory can fail me, there's really a lot of things moving and\n> keep the full picture in mind is hard.\n>\n\nI uploaded the next patch series. Let's discuss there.\n\n> Thanks\n> j\n>\n> > -Hiro\n> >\n> > > > ---\n> > > > src/android/camera_device.cpp | 29 ++++++++++++++++++++---------\n> > > > src/android/camera_device.h | 3 +++\n> > > > 2 files changed, 23 insertions(+), 9 deletions(-)\n> > > >\n> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > > index a71aee2f..ced8efcc 100644\n> > > > --- a/src/android/camera_device.cpp\n> > > > +++ b/src/android/camera_device.cpp\n> > > > @@ -2094,15 +2094,8 @@ void CameraDevice::requestComplete(Request *request)\n> > > > }\n> > > >\n> > > > /* Prepare to call back the Android camera stack. */\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 = status;\n> > > > - }\n> > > > - captureResult.output_buffers = descriptor.buffers_.data();\n> > > > + camera3_capture_result_t captureResult =\n> > > > + createCaptureResult(descriptor, status);\n> > > >\n> > > > if (status == CAMERA3_BUFFER_STATUS_OK) {\n> > > > notifyShutter(descriptor.frameNumber_, timestamp);\n> > > > @@ -2130,6 +2123,24 @@ std::string CameraDevice::logPrefix() const\n> > > > return \"'\" + camera_->id() + \"'\";\n> > > > }\n> > > >\n> > > > +\n> > > > +camera3_capture_result_t CameraDevice::createCaptureResult(\n> > > > + Camera3RequestDescriptor &descriptor,\n> > > > + camera3_buffer_status status) const\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 = status;\n> > > > + }\n> > > > + captureResult.output_buffers = descriptor.buffers_.data();\n> > > > +\n> > > > + return captureResult;\n> > > > +}\n> > > > +\n> > > > void CameraDevice::notifyShutter(uint32_t frameNumber, uint64_t timestamp)\n> > > > {\n> > > > camera3_notify_msg_t notify = {};\n> > > > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > > > index c63e8e21..a1abcead 100644\n> > > > --- a/src/android/camera_device.h\n> > > > +++ b/src/android/camera_device.h\n> > > > @@ -100,6 +100,9 @@ private:\n> > > >\n> > > > std::tuple calculateStaticMetadataSize();\n> > > > libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > > + camera3_capture_result_t createCaptureResult(\n> > > > + Camera3RequestDescriptor &descriptor,\n> > > > + camera3_buffer_status status) const;\n> > > > void notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > > void notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > > > std::unique_ptr requestTemplatePreview();\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 B8689BDC9C\n\tfor ;\n\tMon, 26 Apr 2021 11:01:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2F5AB688BE;\n\tMon, 26 Apr 2021 13:01:25 +0200 (CEST)","from mail-ej1-x636.google.com (mail-ej1-x636.google.com\n\t[IPv6:2a00:1450:4864:20::636])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1AB6605BE\n\tfor ;\n\tMon, 26 Apr 2021 13:01:23 +0200 (CEST)","by mail-ej1-x636.google.com with SMTP id zl7so4093898ejb.6\n\tfor ;\n\tMon, 26 Apr 2021 04:01:23 -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=\"giAU+hU/\"; 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=rBzCmJLahYz28i1or9aZr4Bcgdo6vTvDLuK0KiLNEiI=;\n\tb=giAU+hU/oIBBMmbaC0mUfqXsqcKi/0myNX7amSyUVWbeFiRnJh0ehGQWyM+4EwPhGN\n\toer28cWyEN9URzshkgTplvxeI2qpdtDtggY60W7sQEUbz2aR+LA/zTxUi4bAodmnAlxH\n\tbTm9PUNMmOyr3It2yztvq5s5tXqV6x2FsGtA8=","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=rBzCmJLahYz28i1or9aZr4Bcgdo6vTvDLuK0KiLNEiI=;\n\tb=lF9poensQcWyip0Xf3azVxv7FAJ9wOvhwAvzZe9vnlEwOk4LU9eJJrmQgWkvrO34+0\n\tZ3qjNx+PdKZY/FuFK34hP8b1+U1GEwZKYBNWKOtEk+AeCY1jW0GcopEekPEo6wCnLroW\n\tc0L39gvSaR6f4CPhEQN+oEO1DPrNE352lP9+mAQnjOX6imXfFD4h154rpKVqAxxp9GLZ\n\t5zV3ffBb5JDc2XZll7rF9rl0xUW6DtlvtyGuKpSUqr7NYB1oHrvht7uTtUdYYqK+uL8X\n\ttf/IcVJ6X6qjDMl8JnZYPuThRpETOgsFT80fPNRg8FPjL9vyvC9ZZJg1BGfE6jiOymF5\n\tW30w==","X-Gm-Message-State":"AOAM530tgzA3TAO5d/sPHxlnmfjT51yvB6pta9dTG1HBt7QWHZDHPaxA\n\tiUSxaIB04qvfLfGf05Pwh8X1x6JHWOaNSIyhIWDKWQ==","X-Google-Smtp-Source":"ABdhPJxqDEPH9LaGcmh9fAutJfBsQzJNGiJh/4nvtmVut99/yUmlaYB4YuGAXB5fP/ZgWjFAeAXEw1H1e8WniETJnG4=","X-Received":"by 2002:a17:907:1b06:: with SMTP id\n\tmp6mr18277032ejc.292.1619434883254; \n\tMon, 26 Apr 2021 04:01:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20210426034502.2270770-1-hiroh@chromium.org>\n\t\n\t\n\t<20210426103932.lj7lgqpipkl3ynsd@uno.localdomain>","In-Reply-To":"<20210426103932.lj7lgqpipkl3ynsd@uno.localdomain>","From":"Hirokazu Honda ","Date":"Mon, 26 Apr 2021 20:01:11 +0900","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH] android: CameraDevice: factorize\n\tgenerating a capture result","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]