[PATCH v1 1/1] libcamera: Camera: Add RequestCompletionMode to configure the completion order

[Laurent Pinchart]

Hi Harvey and Han-Lin,

Thank you for the patch.

On Fri, Aug 23, 2024 at 04:22:45PM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen at chromium.org>
> 
> Add enum RequestCompletionMode to Camera with two values:
> InSubmissionOrder andImmediately. The purpose is to allow the
> application to configure the order of signaling requestCompleted.
> The InSubmissionOrder mode is the default mode, which signals according
> to the request submission order.
> The Immediately mode allows the pipeline handler to signal as soon as a
> request is completed. Applications need to reconstruct the order by
> themselves.
> 
> In the real use cases, it allows a camera app to continue the preview
> stream flowing, while certain ISPs/algorithms are still processing a
> complex flow like a still capture request, instead of being blocked
> by the still capture request.

Given how this changes the request mechanism in a fundamental way, you
will need to be way more convincing than this.

Based on the cover letter, this is meant to implement partial metadata
support in the Android camera HAL. If that's the only use case, I think
a better solution is to add partial metadata support to the libcamera
native API. This being said, I don't see how this change can provide
partial metadata support, as metadata is still reported in one go. I
assume you want to report metadata to the camera service ahead of
request completion time, using the partial metadata API of the Android
camera HAL, but still in one go. If that's right, that should be quite
simple to implement in the PipelineHandler class by adding a signal to
report metadata once a request is marked as complete.

> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  Documentation/guides/pipeline-handler.rst |  3 +-
>  include/libcamera/camera.h                |  8 ++++
>  include/libcamera/internal/camera.h       |  1 +
>  src/libcamera/camera.cpp                  | 50 +++++++++++++++++++++--
>  src/libcamera/pipeline_handler.cpp        |  7 ++++
>  5 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> index 26aea4334..f8e64e2f0 100644
> --- a/Documentation/guides/pipeline-handler.rst
> +++ b/Documentation/guides/pipeline-handler.rst
> @@ -1453,7 +1453,8 @@ completion of that buffer to the Camera by using the PipelineHandler base class
>  have been completed, the pipeline handler must again notify the ``Camera`` using
>  the PipelineHandler base class ``completeRequest`` function. The PipelineHandler
>  class implementation makes sure the request completion notifications are
> -delivered to applications in the same order as they have been submitted.
> +delivered to applications in the same order as they have been submitted, unless
> +the Camera's ``RequestCompletionMode`` is set to ``Immediately``.
>  
>  .. _connecting: https://libcamera.org/api-html/classlibcamera_1_1Signal.html#aa04db72d5b3091ffbb4920565aeed382
>  
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 94cee7bd8..5226d94ef 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -116,6 +116,11 @@ class Camera final : public Object, public std::enable_shared_from_this<Camera>,
>  	LIBCAMERA_DECLARE_PRIVATE()
>  
>  public:
> +	enum RequestCompletionMode {
> +		InSubmissionOrder,
> +		Immediately,
> +	};
> +
>  	static std::shared_ptr<Camera> create(std::unique_ptr<Private> d,
>  					      const std::string &id,
>  					      const std::set<Stream *> &streams);
> @@ -129,6 +134,9 @@ public:
>  	int acquire();
>  	int release();
>  
> +	int setRequestCompletionMode(RequestCompletionMode mode);
> +	RequestCompletionMode requestCompletionMode() const;
> +
>  	const ControlInfoMap &controls() const;
>  	const ControlList &properties() const;
>  
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 0add0428b..cb3bbf4f6 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -68,6 +68,7 @@ private:
>  
>  	bool disconnected_;
>  	std::atomic<State> state_;
> +	RequestCompletionMode requestCompletionMode_;
>  
>  	std::unique_ptr<CameraControlValidator> validator_;
>  };
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 382a68f7b..02decc87e 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -5,7 +5,7 @@
>   * Camera device
>   */
>  
> -#include <libcamera/camera.h>
> +#include "libcamera/internal/camera.h"
>  
>  #include <array>
>  #include <atomic>
> @@ -14,12 +14,12 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/thread.h>
>  
> +#include <libcamera/camera.h>
>  #include <libcamera/color_space.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> -#include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
>  #include "libcamera/internal/formats.h"
>  #include "libcamera/internal/pipeline_handler.h"
> @@ -584,7 +584,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   */
>  Camera::Private::Private(PipelineHandler *pipe)
>  	: requestSequence_(0), pipe_(pipe->shared_from_this()),
> -	  disconnected_(false), state_(CameraAvailable)
> +	  disconnected_(false), state_(CameraAvailable),
> +	  requestCompletionMode_(Camera::InSubmissionOrder)
>  {
>  }
>  
> @@ -858,6 +859,15 @@ std::shared_ptr<Camera> Camera::create(std::unique_ptr<Private> d,
>  	return std::shared_ptr<Camera>(camera, Deleter());
>  }
>  
> +/**
> + * \enum Camera::RequestCompletionMode
> + * \brief The mode of request completion behavior
> + * \var libcamera::Camera::InSubmissionOrder
> + * \brief requestCompleted will be emited according to the request submission order
> + * \var libcamera::Camera::Immediately
> + * \brief requestCompleted will be emited immediately when a request is completed
> + */
> +
>  /**
>   * \brief Retrieve the ID of the camera
>   *
> @@ -1405,6 +1415,40 @@ int Camera::stop()
>  	return 0;
>  }
>  
> +/**
> + * \brief Set the request completion mode
> + * \param[in] mode The RequestCompletionMode
> + *
> + * This function sets the request completion mode.
> + * InSubmissionOrder is the default mode.
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EACCES The camera is running so can't change the behavior
> + */
> +int Camera::setRequestCompletionMode(RequestCompletionMode mode)
> +{
> +	Private *const d = _d();
> +
> +	int ret = d->isAccessAllowed(Private::CameraAvailable,
> +				     Private::CameraConfigured);
> +	if (ret < 0)
> +		return -EACCES;
> +
> +	d->requestCompletionMode_ = mode;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Get the request completion mode
> + *
> + * \return The current RequestCompletionMode
> + */
> +Camera::RequestCompletionMode Camera::requestCompletionMode() const
> +{
> +	return _d()->requestCompletionMode_;
> +}
> +
>  /**
>   * \brief Handle request completion and notify application
>   * \param[in] request The request that has completed
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5a6de685b..767e224e9 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -525,6 +525,13 @@ void PipelineHandler::completeRequest(Request *request)
>  
>  	Camera::Private *data = camera->_d();
>  
> +	if (camera->requestCompletionMode() == Camera::Immediately) {
> +		camera->requestComplete(request);
> +		data->queuedRequests_.remove(request);
> +		return;
> +	}
> +
> +	/* camera->requestCompletionMode() == Camera::InSubmissionOrder */
>  	while (!data->queuedRequests_.empty()) {
>  		Request *req = data->queuedRequests_.front();
>  		if (req->status() == Request::RequestPending)

-- 
Regards,

Laurent Pinchart