<div dir="ltr"><div dir="ltr">Hi Laurent,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Aug 25, 2024 at 3:15 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey and Han-Lin,<br>
<br>
Thank you for the patch.<br>
<br>
On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:<br>
> From: Han-Lin Chen <<a href="mailto:hanlinchen@chromium.org" target="_blank">hanlinchen@chromium.org</a>><br>
> <br>
> Add enum RequestCompletionMode to Camera with two values:<br>
> InSubmissionOrder andImmediately. The purpose is to allow the<br>
> application to configure the order of signaling requestCompleted.<br>
> The InSubmissionOrder mode is the default mode, which signals according<br>
> to the request submission order.<br>
> The Immediately mode allows the pipeline handler to signal as soon as a<br>
> request is completed. Applications need to reconstruct the order by<br>
> themselves.<br>
> <br>
> In the real use cases, it allows a camera app to continue the preview<br>
> stream flowing, while certain ISPs/algorithms are still processing a<br>
> complex flow like a still capture request, instead of being blocked<br>
> by the still capture request.<br>
<br>
Given how this changes the request mechanism in a fundamental way, you<br>
will need to be way more convincing than this.<br>
<br>
Based on the cover letter, this is meant to implement partial metadata<br>
support in the Android camera HAL. If that's the only use case, I think<br>
a better solution is to add partial metadata support to the libcamera<br>
native API. This being said, I don't see how this change can provide<br>
partial metadata support, as metadata is still reported in one go. I<br>
assume you want to report metadata to the camera service ahead of<br>
request completion time, using the partial metadata API of the Android<br>
camera HAL, but still in one go. If that's right, that should be quite<br>
simple to implement in the PipelineHandler class by adding a signal to<br>
report metadata once a request is marked as complete.<br>
<br></blockquote><div><br></div><div>Although the purpose of this patch was not to implement partial metadata</div><div>support in the Android adapter, it's true that we don't really need it:</div><div>Buffers and metadata can be returned earlier to the application with </div><div>the partial result support, and Android adapter actually needs to ensure</div><div>that the requests are returned in the submission order as well [1].</div><div><br></div><div>And yes, we'll implement the mechanism to report the last metadata </div><div>as a partial result ahead of the requestCompleted signal.</div><div><br></div><div>Let's drop this patch.</div><div><br></div><div>[1]: <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/android/camera_device.cpp;l=1808</a></div><div><br></div><div>One more question before I submit partial result support patches:</div><div>In mtkisp7 branch, we added `partialResultCompleted` [2] signal</div><div>in libcamera::Camera, while it actually duplicates the signal with</div><div>`bufferCompleted`. In the use cases though, we only call</div><div>`PipelineHandler::completeMetadata` &</div><div>`PipelineHandler::completeBuffer`, without calling </div><div>`PipelineHandler::completePartialResult` directly in the pipeline</div><div>handler mtkisp7's implementation. That being said, we don't need</div><div>to support having multiple buffers and/or metadata within the same</div><div>partial result in the libcamera core libraries.</div><div><br></div><div>Do you think we should keep `bufferCompleted` signal and add</div><div>a `metadataCompleted` signal in libcamera::Camera, or remove</div><div>`bufferCompleted` signal and add a `partialResultCompleted`</div><div>signal?</div><div><br></div><div>Thanks!<br>Harvey</div><div><br></div><div>[2]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/camera.h</a></div><div>[3]: <a href="https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h">https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5674660/1/include/libcamera/internal/pipeline_handler.h</a> </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> Signed-off-by: Han-Lin Chen <<a href="mailto:hanlinchen@chromium.org" target="_blank">hanlinchen@chromium.org</a>><br>
> Co-developed-by: Harvey Yang <<a href="mailto:chenghaoyang@chromium.org" target="_blank">chenghaoyang@chromium.org</a>><br>
> ---<br>
> Documentation/guides/pipeline-handler.rst | 3 +-<br>
> include/libcamera/camera.h | 8 ++++<br>
> include/libcamera/internal/camera.h | 1 +<br>
> src/libcamera/camera.cpp | 50 +++++++++++++++++++++--<br>
> src/libcamera/pipeline_handler.cpp | 7 ++++<br>
> 5 files changed, 65 insertions(+), 4 deletions(-)<br>
> <br>
> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst<br>
> index 26aea4334..f8e64e2f0 100644<br>
> --- a/Documentation/guides/pipeline-handler.rst<br>
> +++ b/Documentation/guides/pipeline-handler.rst<br>
> @@ -1453,7 +1453,8 @@ completion of that buffer to the Camera by using the PipelineHandler base class<br>
> have been completed, the pipeline handler must again notify the ``Camera`` using<br>
> the PipelineHandler base class ``completeRequest`` function. The PipelineHandler<br>
> class implementation makes sure the request completion notifications are<br>
> -delivered to applications in the same order as they have been submitted.<br>
> +delivered to applications in the same order as they have been submitted, unless<br>
> +the Camera's ``RequestCompletionMode`` is set to ``Immediately``.<br>
> <br>
> .. _connecting: <a href="https://libcamera.org/api-html/classlibcamera_1_1Signal.html#aa04db72d5b3091ffbb4920565aeed382" rel="noreferrer" target="_blank">https://libcamera.org/api-html/classlibcamera_1_1Signal.html#aa04db72d5b3091ffbb4920565aeed382</a><br>
> <br>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h<br>
> index 94cee7bd8..5226d94ef 100644<br>
> --- a/include/libcamera/camera.h<br>
> +++ b/include/libcamera/camera.h<br>
> @@ -116,6 +116,11 @@ class Camera final : public Object, public std::enable_shared_from_this<Camera>,<br>
> LIBCAMERA_DECLARE_PRIVATE()<br>
> <br>
> public:<br>
> + enum RequestCompletionMode {<br>
> + InSubmissionOrder,<br>
> + Immediately,<br>
> + };<br>
> +<br>
> static std::shared_ptr<Camera> create(std::unique_ptr<Private> d,<br>
> const std::string &id,<br>
> const std::set<Stream *> &streams);<br>
> @@ -129,6 +134,9 @@ public:<br>
> int acquire();<br>
> int release();<br>
> <br>
> + int setRequestCompletionMode(RequestCompletionMode mode);<br>
> + RequestCompletionMode requestCompletionMode() const;<br>
> +<br>
> const ControlInfoMap &controls() const;<br>
> const ControlList &properties() const;<br>
> <br>
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h<br>
> index 0add0428b..cb3bbf4f6 100644<br>
> --- a/include/libcamera/internal/camera.h<br>
> +++ b/include/libcamera/internal/camera.h<br>
> @@ -68,6 +68,7 @@ private:<br>
> <br>
> bool disconnected_;<br>
> std::atomic<State> state_;<br>
> + RequestCompletionMode requestCompletionMode_;<br>
> <br>
> std::unique_ptr<CameraControlValidator> validator_;<br>
> };<br>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp<br>
> index 382a68f7b..02decc87e 100644<br>
> --- a/src/libcamera/camera.cpp<br>
> +++ b/src/libcamera/camera.cpp<br>
> @@ -5,7 +5,7 @@<br>
> * Camera device<br>
> */<br>
> <br>
> -#include <libcamera/camera.h><br>
> +#include "libcamera/internal/camera.h"<br>
> <br>
> #include <array><br>
> #include <atomic><br>
> @@ -14,12 +14,12 @@<br>
> #include <libcamera/base/log.h><br>
> #include <libcamera/base/thread.h><br>
> <br>
> +#include <libcamera/camera.h><br>
> #include <libcamera/color_space.h><br>
> #include <libcamera/framebuffer_allocator.h><br>
> #include <libcamera/request.h><br>
> #include <libcamera/stream.h><br>
> <br>
> -#include "libcamera/internal/camera.h"<br>
> #include "libcamera/internal/camera_controls.h"<br>
> #include "libcamera/internal/formats.h"<br>
> #include "libcamera/internal/pipeline_handler.h"<br>
> @@ -584,7 +584,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF<br>
> */<br>
> Camera::Private::Private(PipelineHandler *pipe)<br>
> : requestSequence_(0), pipe_(pipe->shared_from_this()),<br>
> - disconnected_(false), state_(CameraAvailable)<br>
> + disconnected_(false), state_(CameraAvailable),<br>
> + requestCompletionMode_(Camera::InSubmissionOrder)<br>
> {<br>
> }<br>
> <br>
> @@ -858,6 +859,15 @@ std::shared_ptr<Camera> Camera::create(std::unique_ptr<Private> d,<br>
> return std::shared_ptr<Camera>(camera, Deleter());<br>
> }<br>
> <br>
> +/**<br>
> + * \enum Camera::RequestCompletionMode<br>
> + * \brief The mode of request completion behavior<br>
> + * \var libcamera::Camera::InSubmissionOrder<br>
> + * \brief requestCompleted will be emited according to the request submission order<br>
> + * \var libcamera::Camera::Immediately<br>
> + * \brief requestCompleted will be emited immediately when a request is completed<br>
> + */<br>
> +<br>
> /**<br>
> * \brief Retrieve the ID of the camera<br>
> *<br>
> @@ -1405,6 +1415,40 @@ int Camera::stop()<br>
> return 0;<br>
> }<br>
> <br>
> +/**<br>
> + * \brief Set the request completion mode<br>
> + * \param[in] mode The RequestCompletionMode<br>
> + *<br>
> + * This function sets the request completion mode.<br>
> + * InSubmissionOrder is the default mode.<br>
> + *<br>
> + * \return 0 on success or a negative error code otherwise<br>
> + * \retval -EACCES The camera is running so can't change the behavior<br>
> + */<br>
> +int Camera::setRequestCompletionMode(RequestCompletionMode mode)<br>
> +{<br>
> + Private *const d = _d();<br>
> +<br>
> + int ret = d->isAccessAllowed(Private::CameraAvailable,<br>
> + Private::CameraConfigured);<br>
> + if (ret < 0)<br>
> + return -EACCES;<br>
> +<br>
> + d->requestCompletionMode_ = mode;<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/**<br>
> + * \brief Get the request completion mode<br>
> + *<br>
> + * \return The current RequestCompletionMode<br>
> + */<br>
> +Camera::RequestCompletionMode Camera::requestCompletionMode() const<br>
> +{<br>
> + return _d()->requestCompletionMode_;<br>
> +}<br>
> +<br>
> /**<br>
> * \brief Handle request completion and notify application<br>
> * \param[in] request The request that has completed<br>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp<br>
> index 5a6de685b..767e224e9 100644<br>
> --- a/src/libcamera/pipeline_handler.cpp<br>
> +++ b/src/libcamera/pipeline_handler.cpp<br>
> @@ -525,6 +525,13 @@ void PipelineHandler::completeRequest(Request *request)<br>
> <br>
> Camera::Private *data = camera->_d();<br>
> <br>
> + if (camera->requestCompletionMode() == Camera::Immediately) {<br>
> + camera->requestComplete(request);<br>
> + data->queuedRequests_.remove(request);<br>
> + return;<br>
> + }<br>
> +<br>
> + /* camera->requestCompletionMode() == Camera::InSubmissionOrder */<br>
> while (!data->queuedRequests_.empty()) {<br>
> Request *req = data->queuedRequests_.front();<br>
> if (req->status() == Request::RequestPending)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>