[{"id":30896,"web_url":"https://patchwork.libcamera.org/comment/30896/","msgid":"<20240825011524.GA21395@pendragon.ideasonboard.com>","date":"2024-08-25T01:15:24","subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Harvey and Han-Lin,\n\nThank you for the patch.\n\nOn Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:\n> From: Han-Lin Chen \n> \n> Add enum RequestCompletionMode to Camera with two values:\n> InSubmissionOrder andImmediately. The purpose is to allow the\n> application to configure the order of signaling requestCompleted.\n> The InSubmissionOrder mode is the default mode, which signals according\n> to the request submission order.\n> The Immediately mode allows the pipeline handler to signal as soon as a\n> request is completed. Applications need to reconstruct the order by\n> themselves.\n> \n> In the real use cases, it allows a camera app to continue the preview\n> stream flowing, while certain ISPs/algorithms are still processing a\n> complex flow like a still capture request, instead of being blocked\n> by the still capture request.\n\nGiven how this changes the request mechanism in a fundamental way, you\nwill need to be way more convincing than this.\n\nBased on the cover letter, this is meant to implement partial metadata\nsupport in the Android camera HAL. If that's the only use case, I think\na better solution is to add partial metadata support to the libcamera\nnative API. This being said, I don't see how this change can provide\npartial metadata support, as metadata is still reported in one go. I\nassume you want to report metadata to the camera service ahead of\nrequest completion time, using the partial metadata API of the Android\ncamera HAL, but still in one go. If that's right, that should be quite\nsimple to implement in the PipelineHandler class by adding a signal to\nreport metadata once a request is marked as complete.\n\n> Signed-off-by: Han-Lin Chen \n> Co-developed-by: Harvey Yang \n> ---\n> Documentation/guides/pipeline-handler.rst | 3 +-\n> include/libcamera/camera.h | 8 ++++\n> include/libcamera/internal/camera.h | 1 +\n> src/libcamera/camera.cpp | 50 +++++++++++++++++++++--\n> src/libcamera/pipeline_handler.cpp | 7 ++++\n> 5 files changed, 65 insertions(+), 4 deletions(-)\n> \n> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst\n> index 26aea4334..f8e64e2f0 100644\n> --- a/Documentation/guides/pipeline-handler.rst\n> +++ b/Documentation/guides/pipeline-handler.rst\n> @@ -1453,7 +1453,8 @@ completion of that buffer to the Camera by using the PipelineHandler base class\n> have been completed, the pipeline handler must again notify the ``Camera`` using\n> the PipelineHandler base class ``completeRequest`` function. The PipelineHandler\n> class implementation makes sure the request completion notifications are\n> -delivered to applications in the same order as they have been submitted.\n> +delivered to applications in the same order as they have been submitted, unless\n> +the Camera's ``RequestCompletionMode`` is set to ``Immediately``.\n> \n> .. _connecting: https://libcamera.org/api-html/classlibcamera_1_1Signal.html#aa04db72d5b3091ffbb4920565aeed382\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 94cee7bd8..5226d94ef 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -116,6 +116,11 @@ class Camera final : public Object, public std::enable_shared_from_this,\n> \tLIBCAMERA_DECLARE_PRIVATE()\n> \n> public:\n> +\tenum RequestCompletionMode {\n> +\t\tInSubmissionOrder,\n> +\t\tImmediately,\n> +\t};\n> +\n> \tstatic std::shared_ptr create(std::unique_ptr d,\n> \t\t\t\t\t const std::string &id,\n> \t\t\t\t\t const std::set &streams);\n> @@ -129,6 +134,9 @@ public:\n> \tint acquire();\n> \tint release();\n> \n> +\tint setRequestCompletionMode(RequestCompletionMode mode);\n> +\tRequestCompletionMode requestCompletionMode() const;\n> +\n> \tconst ControlInfoMap &controls() const;\n> \tconst ControlList &properties() const;\n> \n> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h\n> index 0add0428b..cb3bbf4f6 100644\n> --- a/include/libcamera/internal/camera.h\n> +++ b/include/libcamera/internal/camera.h\n> @@ -68,6 +68,7 @@ private:\n> \n> \tbool disconnected_;\n> \tstd::atomic state_;\n> +\tRequestCompletionMode requestCompletionMode_;\n> \n> \tstd::unique_ptr validator_;\n> };\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 382a68f7b..02decc87e 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -5,7 +5,7 @@\n> * Camera device\n> */\n> \n> -#include \n> +#include \"libcamera/internal/camera.h\"\n> \n> #include \n> #include \n> @@ -14,12 +14,12 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> #include \n> \n> -#include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> #include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> @@ -584,7 +584,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF\n> */\n> Camera::Private::Private(PipelineHandler *pipe)\n> \t: requestSequence_(0), pipe_(pipe->shared_from_this()),\n> -\t disconnected_(false), state_(CameraAvailable)\n> +\t disconnected_(false), state_(CameraAvailable),\n> +\t requestCompletionMode_(Camera::InSubmissionOrder)\n> {\n> }\n> \n> @@ -858,6 +859,15 @@ std::shared_ptr Camera::create(std::unique_ptr d,\n> \treturn std::shared_ptr(camera, Deleter());\n> }\n> \n> +/**\n> + * \\enum Camera::RequestCompletionMode\n> + * \\brief The mode of request completion behavior\n> + * \\var libcamera::Camera::InSubmissionOrder\n> + * \\brief requestCompleted will be emited according to the request submission order\n> + * \\var libcamera::Camera::Immediately\n> + * \\brief requestCompleted will be emited immediately when a request is completed\n> + */\n> +\n> /**\n> * \\brief Retrieve the ID of the camera\n> *\n> @@ -1405,6 +1415,40 @@ int Camera::stop()\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Set the request completion mode\n> + * \\param[in] mode The RequestCompletionMode\n> + *\n> + * This function sets the request completion mode.\n> + * InSubmissionOrder is the default mode.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + * \\retval -EACCES The camera is running so can't change the behavior\n> + */\n> +int Camera::setRequestCompletionMode(RequestCompletionMode mode)\n> +{\n> +\tPrivate *const d = _d();\n> +\n> +\tint ret = d->isAccessAllowed(Private::CameraAvailable,\n> +\t\t\t\t Private::CameraConfigured);\n> +\tif (ret < 0)\n> +\t\treturn -EACCES;\n> +\n> +\td->requestCompletionMode_ = mode;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the request completion mode\n> + *\n> + * \\return The current RequestCompletionMode\n> + */\n> +Camera::RequestCompletionMode Camera::requestCompletionMode() const\n> +{\n> +\treturn _d()->requestCompletionMode_;\n> +}\n> +\n> /**\n> * \\brief Handle request completion and notify application\n> * \\param[in] request The request that has completed\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 5a6de685b..767e224e9 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -525,6 +525,13 @@ void PipelineHandler::completeRequest(Request *request)\n> \n> \tCamera::Private *data = camera->_d();\n> \n> +\tif (camera->requestCompletionMode() == Camera::Immediately) {\n> +\t\tcamera->requestComplete(request);\n> +\t\tdata->queuedRequests_.remove(request);\n> +\t\treturn;\n> +\t}\n> +\n> +\t/* camera->requestCompletionMode() == Camera::InSubmissionOrder */\n> \twhile (!data->queuedRequests_.empty()) {\n> \t\tRequest *req = data->queuedRequests_.front();\n> \t\tif (req->status() == Request::RequestPending)","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 94CC8C323E\n\tfor ;\n\tSun, 25 Aug 2024 01:15:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32D8B633CF;\n\tSun, 25 Aug 2024 03:15:30 +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 5355661902\n\tfor ;\n\tSun, 25 Aug 2024 03:15:28 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6D5145B;\n\tSun, 25 Aug 2024 03:14:22 +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=\"lxKvhyX7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724548463;\n\tbh=OQ7AU/2gQnO3o3YwiWTlwoE0zjMHlcmk3eRUIqvOdtQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lxKvhyX7i3KVMKRxwSRHIMSO3W8WUWhcTjF5vGENP/OydujHCkXBusJWYHxJOjaWX\n\t1JJM+Lnvyer4+X/ORQxxBFC90+JW1xDHg1BUPAuliYAT2JfION0idXs7aJnDfMYlkP\n\tL9wnSxAnOS7zoNwOB8H6vnXA4Es/C0vw7DhvxxNA=","Date":"Sun, 25 Aug 2024 04:15:24 +0300","From":"Laurent Pinchart ","To":"Harvey Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","Message-ID":"<20240825011524.GA21395@pendragon.ideasonboard.com>","References":"<20240823163251.2979324-1-chenghaoyang@google.com>\n\t<20240823163251.2979324-2-chenghaoyang@google.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240823163251.2979324-2-chenghaoyang@google.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30904,"web_url":"https://patchwork.libcamera.org/comment/30904/","msgid":"","date":"2024-08-26T12:13:22","subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"Hi Laurent,\n\nOn Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Harvey and Han-Lin,\n>\n> Thank you for the patch.\n>\n> On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:\n> > From: Han-Lin Chen \n> >\n> > Add enum RequestCompletionMode to Camera with two values:\n> > InSubmissionOrder andImmediately. The purpose is to allow the\n> > application to configure the order of signaling requestCompleted.\n> > The InSubmissionOrder mode is the default mode, which signals according\n> > to the request submission order.\n> > The Immediately mode allows the pipeline handler to signal as soon as a\n> > request is completed. Applications need to reconstruct the order by\n> > themselves.\n> >\n> > In the real use cases, it allows a camera app to continue the preview\n> > stream flowing, while certain ISPs/algorithms are still processing a\n> > complex flow like a still capture request, instead of being blocked\n> > by the still capture request.\n>\n> Given how this changes the request mechanism in a fundamental way, you\n> will need to be way more convincing than this.\n>\n> Based on the cover letter, this is meant to implement partial metadata\n> support in the Android camera HAL. If that's the only use case, I think\n> a better solution is to add partial metadata support to the libcamera\n> native API. This being said, I don't see how this change can provide\n> partial metadata support, as metadata is still reported in one go. I\n> assume you want to report metadata to the camera service ahead of\n> request completion time, using the partial metadata API of the Android\n> camera HAL, but still in one go. If that's right, that should be quite\n> simple to implement in the PipelineHandler class by adding a signal to\n> report metadata once a request is marked as complete.\n>\n>\nAlthough the purpose of this patch was not to implement partial metadata\nsupport in the Android adapter, it's true that we don't really need it:\nBuffers and metadata can be returned earlier to the application with\nthe partial result support, and Android adapter actually needs to ensure\nthat the requests are returned in the submission order as well [1].\n\nAnd yes, we'll implement the mechanism to report the last metadata\nas a partial result ahead of the requestCompleted signal.\n\nLet's drop this patch.\n\n[1]:\nhttps://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808\n\nOne more question before I submit partial result support patches:\nIn mtkisp7 branch, we added `partialResultCompleted` [2] signal\nin libcamera::Camera, while it actually duplicates the signal with\n`bufferCompleted`. In the use cases though, we only call\n`PipelineHandler::completeMetadata` &\n`PipelineHandler::completeBuffer`, without calling\n`PipelineHandler::completePartialResult` directly in the pipeline\nhandler mtkisp7's implementation. That being said, we don't need\nto support having multiple buffers and/or metadata within the same\npartial result in the libcamera core libraries.\n\nDo you think we should keep `bufferCompleted` signal and add\na `metadataCompleted` signal in libcamera::Camera, or remove\n`bufferCompleted` signal and add a `partialResultCompleted`\nsignal?\n\nThanks!\nHarvey\n\n[2]:\nhttps://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h\n[3]:\nhttps://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h\n\n\n> Signed-off-by: Han-Lin Chen \n> > Co-developed-by: Harvey Yang \n> > ---\n> > Documentation/guides/pipeline-handler.rst | 3 +-\n> > include/libcamera/camera.h | 8 ++++\n> > include/libcamera/internal/camera.h | 1 +\n> > src/libcamera/camera.cpp | 50 +++++++++++++++++++++--\n> > src/libcamera/pipeline_handler.cpp | 7 ++++\n> > 5 files changed, 65 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/Documentation/guides/pipeline-handler.rst\n> b/Documentation/guides/pipeline-handler.rst\n> > index 26aea4334..f8e64e2f0 100644\n> > --- a/Documentation/guides/pipeline-handler.rst\n> > +++ b/Documentation/guides/pipeline-handler.rst\n> > @@ -1453,7 +1453,8 @@ completion of that buffer to the Camera by using\n> the PipelineHandler base class\n> > have been completed, the pipeline handler must again notify the\n> ``Camera`` using\n> > the PipelineHandler base class ``completeRequest`` function. The\n> PipelineHandler\n> > class implementation makes sure the request completion notifications are\n> > -delivered to applications in the same order as they have been submitted.\n> > +delivered to applications in the same order as they have been\n> submitted, unless\n> > +the Camera's ``RequestCompletionMode`` is set to ``Immediately``.\n> >\n> > .. _connecting:\n> https://libcamera.org/api-html/classlibcamera_1_1Signal.html#aa04db72d5b3091ffbb4920565aeed382\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 94cee7bd8..5226d94ef 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -116,6 +116,11 @@ class Camera final : public Object, public\n> std::enable_shared_from_this,\n> > LIBCAMERA_DECLARE_PRIVATE()\n> >\n> > public:\n> > + enum RequestCompletionMode {\n> > + InSubmissionOrder,\n> > + Immediately,\n> > + };\n> > +\n> > static std::shared_ptr create(std::unique_ptr d,\n> > const std::string &id,\n> > const std::set\n> &streams);\n> > @@ -129,6 +134,9 @@ public:\n> > int acquire();\n> > int release();\n> >\n> > + int setRequestCompletionMode(RequestCompletionMode mode);\n> > + RequestCompletionMode requestCompletionMode() const;\n> > +\n> > const ControlInfoMap &controls() const;\n> > const ControlList &properties() const;\n> >\n> > diff --git a/include/libcamera/internal/camera.h\n> b/include/libcamera/internal/camera.h\n> > index 0add0428b..cb3bbf4f6 100644\n> > --- a/include/libcamera/internal/camera.h\n> > +++ b/include/libcamera/internal/camera.h\n> > @@ -68,6 +68,7 @@ private:\n> >\n> > bool disconnected_;\n> > std::atomic state_;\n> > + RequestCompletionMode requestCompletionMode_;\n> >\n> > std::unique_ptr validator_;\n> > };\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 382a68f7b..02decc87e 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -5,7 +5,7 @@\n> > * Camera device\n> > */\n> >\n> > -#include \n> > +#include \"libcamera/internal/camera.h\"\n> >\n> > #include \n> > #include \n> > @@ -14,12 +14,12 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > #include \n> >\n> > -#include \"libcamera/internal/camera.h\"\n> > #include \"libcamera/internal/camera_controls.h\"\n> > #include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/pipeline_handler.h\"\n> > @@ -584,7 +584,8 @@ CameraConfiguration::Status\n> CameraConfiguration::validateColorSpaces(ColorSpaceF\n> > */\n> > Camera::Private::Private(PipelineHandler *pipe)\n> > : requestSequence_(0), pipe_(pipe->shared_from_this()),\n> > - disconnected_(false), state_(CameraAvailable)\n> > + disconnected_(false), state_(CameraAvailable),\n> > + requestCompletionMode_(Camera::InSubmissionOrder)\n> > {\n> > }\n> >\n> > @@ -858,6 +859,15 @@ std::shared_ptr\n> Camera::create(std::unique_ptr d,\n> > return std::shared_ptr(camera, Deleter());\n> > }\n> >\n> > +/**\n> > + * \\enum Camera::RequestCompletionMode\n> > + * \\brief The mode of request completion behavior\n> > + * \\var libcamera::Camera::InSubmissionOrder\n> > + * \\brief requestCompleted will be emited according to the request\n> submission order\n> > + * \\var libcamera::Camera::Immediately\n> > + * \\brief requestCompleted will be emited immediately when a request is\n> completed\n> > + */\n> > +\n> > /**\n> > * \\brief Retrieve the ID of the camera\n> > *\n> > @@ -1405,6 +1415,40 @@ int Camera::stop()\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Set the request completion mode\n> > + * \\param[in] mode The RequestCompletionMode\n> > + *\n> > + * This function sets the request completion mode.\n> > + * InSubmissionOrder is the default mode.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + * \\retval -EACCES The camera is running so can't change the behavior\n> > + */\n> > +int Camera::setRequestCompletionMode(RequestCompletionMode mode)\n> > +{\n> > + Private *const d = _d();\n> > +\n> > + int ret = d->isAccessAllowed(Private::CameraAvailable,\n> > + Private::CameraConfigured);\n> > + if (ret < 0)\n> > + return -EACCES;\n> > +\n> > + d->requestCompletionMode_ = mode;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get the request completion mode\n> > + *\n> > + * \\return The current RequestCompletionMode\n> > + */\n> > +Camera::RequestCompletionMode Camera::requestCompletionMode() const\n> > +{\n> > + return _d()->requestCompletionMode_;\n> > +}\n> > +\n> > /**\n> > * \\brief Handle request completion and notify application\n> > * \\param[in] request The request that has completed\n> > diff --git a/src/libcamera/pipeline_handler.cpp\n> b/src/libcamera/pipeline_handler.cpp\n> > index 5a6de685b..767e224e9 100644\n> > --- a/src/libcamera/pipeline_handler.cpp\n> > +++ b/src/libcamera/pipeline_handler.cpp\n> > @@ -525,6 +525,13 @@ void PipelineHandler::completeRequest(Request\n> *request)\n> >\n> > Camera::Private *data = camera->_d();\n> >\n> > + if (camera->requestCompletionMode() == Camera::Immediately) {\n> > + camera->requestComplete(request);\n> > + data->queuedRequests_.remove(request);\n> > + return;\n> > + }\n> > +\n> > + /* camera->requestCompletionMode() == Camera::InSubmissionOrder */\n> > while (!data->queuedRequests_.empty()) {\n> > Request *req = data->queuedRequests_.front();\n> > if (req->status() == Request::RequestPending)\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 10263C323E\n\tfor ;\n\tMon, 26 Aug 2024 12:13:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E0EC1633CF;\n\tMon, 26 Aug 2024 14:13:36 +0200 (CEST)","from mail-lj1-x234.google.com (mail-lj1-x234.google.com\n\t[IPv6:2a00:1450:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1E47C6190C\n\tfor ;\n\tMon, 26 Aug 2024 14:13:35 +0200 (CEST)","by mail-lj1-x234.google.com with SMTP id\n\t38308e7fff4ca-2f3f90295a9so45491581fa.0\n\tfor ;\n\tMon, 26 Aug 2024 05:13:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OoFtA0bR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1724674414; x=1725279214;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=CEMAtfzUi82jiNZK35ICyYxyB2uTuFa5QzjFx9inlho=;\n\tb=OoFtA0bRgTwjQYoXiGTgznet7Df8TGhz/i3ulOuaCjUSGzWhjOfoW+D6XH/XMRVavp\n\tmJrFgAM+bhuReWC6wWmpX9FfNKe6RP42Fn3aRBXlCqdAc4l/zx6tk9OkZ1ULVbspiiID\n\tRgfIsUWeR/eQKU/l4hi/eEKSpyWM/kBNQepBI=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724674414; x=1725279214;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=CEMAtfzUi82jiNZK35ICyYxyB2uTuFa5QzjFx9inlho=;\n\tb=V2RjU0BVGxi2ZN5BAI9fyuRSqTl/auIecnzjAU5UKC8TOLWjRO9I65+senfHe1WLnU\n\tXq/WvGSa1hZ1yC6MlE9MVJKA6h4UNuR5LzhMFQ1+P9C2iaCtWFl7qcM2kZWO5XF8ZSuw\n\tXovJxqLtazyfLQqDjrJUYBPwsSpmOppMAavtTz0s23OeVM73P22zlB4rg+EwseNILQii\n\tYIZqCOnMYIvffa9nsoJI1SyTwoNSwML1BLv7qwhsK076BGk4bx389RxWHeIGWyfJVbzt\n\t9ov1//FewShGhExEN40CtavKC0+4LL5GbW+9hC16A4Zq8XGCQ7PJBwVDA/NkieeIpZBH\n\twu5Q==","X-Gm-Message-State":"AOJu0YzLKrCQeeZbnR/OxwsLZ/ZEISA9MvsvuBE64sBamqrnjTKBMASF\n\tPMcZEizuInTqeBoLuyorPcnK3wUgDw9tJV+qDfyt9Dk3JVX1yx1dnNQA/mMz+pwSgcNbs2J4w3M\n\tx3ywajv61F9QDhMRrsxVmdySo2CV03eq7VfLZ","X-Google-Smtp-Source":"AGHT+IFYnw/1ZTuyZ3jsKnRtCwEjViWr9q0rmm/6A4T81qys9RL1Pk4/8cz3wpVGEv5J1bfPylXpy35+NEenBuiWfiY=","X-Received":"by 2002:a2e:90c5:0:b0:2f4:1e6:5f57 with SMTP id\n\t38308e7fff4ca-2f4f4942464mr58888621fa.45.1724674413779;\n\tMon, 26 Aug 2024 05:13:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20240823163251.2979324-1-chenghaoyang@google.com>\n\t<20240823163251.2979324-2-chenghaoyang@google.com>\n\t<20240825011524.GA21395@pendragon.ideasonboard.com>","In-Reply-To":"<20240825011524.GA21395@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 26 Aug 2024 14:13:22 +0200","Message-ID":"","Subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"multipart/alternative; boundary=\"000000000000c970e90620950dd8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31024,"web_url":"https://patchwork.libcamera.org/comment/31024/","msgid":"<20240901004646.GD3811@pendragon.ideasonboard.com>","date":"2024-09-01T00:46:46","subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Aug 26, 2024 at 02:13:22PM +0200, Cheng-Hao Yang wrote:\n> On Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart wrote:\n> > On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:\n> > > From: Han-Lin Chen \n> > >\n> > > Add enum RequestCompletionMode to Camera with two values:\n> > > InSubmissionOrder andImmediately. The purpose is to allow the\n> > > application to configure the order of signaling requestCompleted.\n> > > The InSubmissionOrder mode is the default mode, which signals according\n> > > to the request submission order.\n> > > The Immediately mode allows the pipeline handler to signal as soon as a\n> > > request is completed. Applications need to reconstruct the order by\n> > > themselves.\n> > >\n> > > In the real use cases, it allows a camera app to continue the preview\n> > > stream flowing, while certain ISPs/algorithms are still processing a\n> > > complex flow like a still capture request, instead of being blocked\n> > > by the still capture request.\n> >\n> > Given how this changes the request mechanism in a fundamental way, you\n> > will need to be way more convincing than this.\n> >\n> > Based on the cover letter, this is meant to implement partial metadata\n> > support in the Android camera HAL. If that's the only use case, I think\n> > a better solution is to add partial metadata support to the libcamera\n> > native API. This being said, I don't see how this change can provide\n> > partial metadata support, as metadata is still reported in one go. I\n> > assume you want to report metadata to the camera service ahead of\n> > request completion time, using the partial metadata API of the Android\n> > camera HAL, but still in one go. If that's right, that should be quite\n> > simple to implement in the PipelineHandler class by adding a signal to\n> > report metadata once a request is marked as complete.\n>\n> Although the purpose of this patch was not to implement partial metadata\n> support in the Android adapter, it's true that we don't really need it:\n> Buffers and metadata can be returned earlier to the application with\n> the partial result support, and Android adapter actually needs to ensure\n> that the requests are returned in the submission order as well [1].\n> \n> And yes, we'll implement the mechanism to report the last metadata\n> as a partial result ahead of the requestCompleted signal.\n> \n> Let's drop this patch.\n> \n> [1]: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808\n> \n> One more question before I submit partial result support patches:\n> In mtkisp7 branch, we added `partialResultCompleted` [2] signal\n> in libcamera::Camera, while it actually duplicates the signal with\n> `bufferCompleted`. In the use cases though, we only call\n> `PipelineHandler::completeMetadata` &\n> `PipelineHandler::completeBuffer`, without calling\n> `PipelineHandler::completePartialResult` directly in the pipeline\n> handler mtkisp7's implementation. That being said, we don't need\n> to support having multiple buffers and/or metadata within the same\n> partial result in the libcamera core libraries.\n> \n> Do you think we should keep `bufferCompleted` signal and add\n> a `metadataCompleted` signal in libcamera::Camera, or remove\n> `bufferCompleted` signal and add a `partialResultCompleted`\n> signal?\n\nI think added a metadataCompleted signal is the simplest and least\nintrusive change for now (please voice your opinion if you disagree, I\nmay be missing something), so it would be my preference.\n\n> [2]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h\n> [3]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h\n> \n> > > Signed-off-by: Han-Lin Chen \n> > > Co-developed-by: Harvey Yang \n> > > ---\n> > > Documentation/guides/pipeline-handler.rst | 3 +-\n> > > include/libcamera/camera.h | 8 ++++\n> > > include/libcamera/internal/camera.h | 1 +\n> > > src/libcamera/camera.cpp | 50 +++++++++++++++++++++--\n> > > src/libcamera/pipeline_handler.cpp | 7 ++++\n> > > 5 files changed, 65 insertions(+), 4 deletions(-)\n\n[snip]","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 73AB8C323E\n\tfor ;\n\tSun, 1 Sep 2024 00:47:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31F5463471;\n\tSun, 1 Sep 2024 02:47:20 +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 A616B618FE\n\tfor ;\n\tSun, 1 Sep 2024 02:47:18 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 37A6C4AD;\n\tSun, 1 Sep 2024 02:46:08 +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=\"wSe95UgO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725151568;\n\tbh=YPTXukJgDFc0s7JSs+QoVV5fKslUUG2fJLlKBqcI+aM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=wSe95UgOH09L8RkAByLc7lJ6ZxmlJRFmiTeus3bBiASpZRDBpBVbQO3Ih9Nu8Bh1p\n\tdkILYXbUh/omjtXgsMI9UXbzr9JLdohCthila3SCKuyDHiA5tPy4eYDCVRo/R5ftQj\n\tajbV0TqlEZEVTkWsL16dZRYlfv4YgcEzpuqeK6xI=","Date":"Sun, 1 Sep 2024 03:46:46 +0300","From":"Laurent Pinchart ","To":"Cheng-Hao Yang ","Cc":"libcamera-devel@lists.libcamera.org,\n\tHan-Lin Chen ","Subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","Message-ID":"<20240901004646.GD3811@pendragon.ideasonboard.com>","References":"<20240823163251.2979324-1-chenghaoyang@google.com>\n\t<20240823163251.2979324-2-chenghaoyang@google.com>\n\t<20240825011524.GA21395@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31037,"web_url":"https://patchwork.libcamera.org/comment/31037/","msgid":"","date":"2024-09-02T07:48:12","subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","submitter":{"id":117,"url":"https://patchwork.libcamera.org/api/people/117/","name":"Cheng-Hao Yang","email":"chenghaoyang@chromium.org"},"content":"On Sun, Sep 1, 2024 at 8:47 AM Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> On Mon, Aug 26, 2024 at 02:13:22PM +0200, Cheng-Hao Yang wrote:\n> > On Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart wrote:\n> > > On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:\n> > > > From: Han-Lin Chen \n> > > >\n> > > > Add enum RequestCompletionMode to Camera with two values:\n> > > > InSubmissionOrder andImmediately. The purpose is to allow the\n> > > > application to configure the order of signaling requestCompleted.\n> > > > The InSubmissionOrder mode is the default mode, which signals\n> according\n> > > > to the request submission order.\n> > > > The Immediately mode allows the pipeline handler to signal as soon\n> as a\n> > > > request is completed. Applications need to reconstruct the order by\n> > > > themselves.\n> > > >\n> > > > In the real use cases, it allows a camera app to continue the preview\n> > > > stream flowing, while certain ISPs/algorithms are still processing a\n> > > > complex flow like a still capture request, instead of being blocked\n> > > > by the still capture request.\n> > >\n> > > Given how this changes the request mechanism in a fundamental way, you\n> > > will need to be way more convincing than this.\n> > >\n> > > Based on the cover letter, this is meant to implement partial metadata\n> > > support in the Android camera HAL. If that's the only use case, I think\n> > > a better solution is to add partial metadata support to the libcamera\n> > > native API. This being said, I don't see how this change can provide\n> > > partial metadata support, as metadata is still reported in one go. I\n> > > assume you want to report metadata to the camera service ahead of\n> > > request completion time, using the partial metadata API of the Android\n> > > camera HAL, but still in one go. If that's right, that should be quite\n> > > simple to implement in the PipelineHandler class by adding a signal to\n> > > report metadata once a request is marked as complete.\n> >\n> > Although the purpose of this patch was not to implement partial metadata\n> > support in the Android adapter, it's true that we don't really need it:\n> > Buffers and metadata can be returned earlier to the application with\n> > the partial result support, and Android adapter actually needs to ensure\n> > that the requests are returned in the submission order as well [1].\n> >\n> > And yes, we'll implement the mechanism to report the last metadata\n> > as a partial result ahead of the requestCompleted signal.\n> >\n> > Let's drop this patch.\n> >\n> > [1]:\n> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808\n> >\n> > One more question before I submit partial result support patches:\n> > In mtkisp7 branch, we added `partialResultCompleted` [2] signal\n> > in libcamera::Camera, while it actually duplicates the signal with\n> > `bufferCompleted`. In the use cases though, we only call\n> > `PipelineHandler::completeMetadata` &\n> > `PipelineHandler::completeBuffer`, without calling\n> > `PipelineHandler::completePartialResult` directly in the pipeline\n> > handler mtkisp7's implementation. That being said, we don't need\n> > to support having multiple buffers and/or metadata within the same\n> > partial result in the libcamera core libraries.\n> >\n> > Do you think we should keep `bufferCompleted` signal and add\n> > a `metadataCompleted` signal in libcamera::Camera, or remove\n> > `bufferCompleted` signal and add a `partialResultCompleted`\n> > signal?\n>\n> I think added a metadataCompleted signal is the simplest and least\n> intrusive change for now (please voice your opinion if you disagree, I\n> may be missing something), so it would be my preference.\n>\n\nThanks for the reply. I think it makes sense, as both mtkisp7 and\nintelipu7 doesn't seem to need a partial result that contains both\nbuffers and metadata.\n\nI'll modify the patches and upload them accordingly.\n\nBR,\nHarvey\n\n\n>\n> > [2]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h\n> > [3]:\n> https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h\n> >\n> > > > Signed-off-by: Han-Lin Chen \n> > > > Co-developed-by: Harvey Yang \n> > > > ---\n> > > > Documentation/guides/pipeline-handler.rst | 3 +-\n> > > > include/libcamera/camera.h | 8 ++++\n> > > > include/libcamera/internal/camera.h | 1 +\n> > > > src/libcamera/camera.cpp | 50\n> +++++++++++++++++++++--\n> > > > src/libcamera/pipeline_handler.cpp | 7 ++++\n> > > > 5 files changed, 65 insertions(+), 4 deletions(-)\n>\n> [snip]\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 CB39AC324C\n\tfor ;\n\tMon, 2 Sep 2024 07:48:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A9D6634CB;\n\tMon, 2 Sep 2024 09:48:26 +0200 (CEST)","from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D08A618FE\n\tfor ;\n\tMon, 2 Sep 2024 09:48:25 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id\n\t38308e7fff4ca-2f3e9fb6ee9so48000151fa.3\n\tfor ;\n\tMon, 02 Sep 2024 00:48:25 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fefk+hna\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=chromium.org; s=google; t=1725263304; x=1725868104;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=7EOg8NM8+Kdq+kd/u+CllGGlyA4ld9spvV9sT9gI788=;\n\tb=fefk+hnadsESRJUOPYPmjPZHH0ySd4oYlpaVyofKluMsFTfudtn977fZInz0mRBSkL\n\tlDPJ0me5q/TRdflnMFpdCce+MpvfWBXKvouoqCaA9R3WuOlDyaqYwZdjdi62EJfzTczn\n\tt1lrH3rhvADcnqziPWEGbIAac6pm0ayS4QtIo=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725263304; x=1725868104;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=7EOg8NM8+Kdq+kd/u+CllGGlyA4ld9spvV9sT9gI788=;\n\tb=AjrNvgg0pkMq1yd607peSSysHUTFSEueRGyzqapNgf7dHB9/HM7RIhvCg0BkzQj5RG\n\tsEIK9LJHoCK3Xi/Yth1tO/mD3ozfdcdQFM+eTA1/6Uwcw4iHxJS2yBWWHP6tkjPHPz/F\n\tFcrN9rk4fMWPl+gaJtGcjQU2T2ZYdAwy0jWrEhjdOd+dhfhfNdw2xDJsq9R3jNqXdeB4\n\tcd2WNIZJcBR5ClNzh7Rz8ulSN58AxxlpvQxtL8CyDtTA5F6ItCT/vVe8ycInpOMiacFb\n\tx2jTVL1nBXrogXzvBNYNAYIqStP+bDQn6Uc83/rpgdsA9wAcGiIj9C448bW/s37Tg36O\n\tmjiA==","X-Gm-Message-State":"AOJu0YzsODD7VpHIy4O4puiVK2seAqy+GQIvgKO/mfLx6ZYkynez2B3+\n\tT9M3jciTiNESjg9T5K0BWrsngUT8JAKUdhIUw3R5r8a+yW3+yhwHNpLASAqa8ehE9o1aYW4DLI5\n\tEohbe/qTjvK/E750kQKF9JGWR3eFMdXPYrpRS","X-Google-Smtp-Source":"AGHT+IGnInMLct8pfaaXrUmyFKYj9RNWYm/NpJq8Tu1p1BMNFA/CjMSqkbCpUOMU4FoIy6YPpkz6JuJEjap+P2HeaIs=","X-Received":"by 2002:a2e:a781:0:b0:2f1:a19b:d5a4 with SMTP id\n\t38308e7fff4ca-2f636a2b8f9mr12858811fa.22.1725263303929;\n\tMon, 02 Sep 2024 00:48:23 -0700 (PDT)","MIME-Version":"1.0","References":"<20240823163251.2979324-1-chenghaoyang@google.com>\n\t<20240823163251.2979324-2-chenghaoyang@google.com>\n\t<20240825011524.GA21395@pendragon.ideasonboard.com>\n\t\n\t<20240901004646.GD3811@pendragon.ideasonboard.com>","In-Reply-To":"<20240901004646.GD3811@pendragon.ideasonboard.com>","From":"Cheng-Hao Yang ","Date":"Mon, 2 Sep 2024 15:48:12 +0800","Message-ID":"","Subject":"Re: [PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to\n\tconfigure the completion order","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tHan-Lin Chen ","Content-Type":"multipart/alternative; boundary=\"000000000000600f2c06211e2a34\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]