[PATCH v2 1/9] libcamera: Camera: Add signals for completion of metadata as a partial result

[Jacopo Mondi]

Hi Harvey

On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen at chromium.org>
>
> Allows pipeline handler to signal metadata completion by adding the
> following signals to Camera:
>
> Signal<Request *, const ControlList &> metadataAvailable;
>
> Together with the bufferCompleted signal, the pipeline handler is allowed to
> return buffers and partial metadata at any stage of processing.
>
> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> ---
>  include/libcamera/camera.h                    |  1 +
>  include/libcamera/internal/pipeline_handler.h |  1 +
>  include/libcamera/internal/request.h          |  4 ++
>  include/libcamera/request.h                   |  1 +
>  src/libcamera/camera.cpp                      |  6 +++
>  src/libcamera/pipeline_handler.cpp            | 41 +++++++++++++++++++
>  src/libcamera/request.cpp                     | 21 ++++++++++
>  7 files changed, 75 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 94cee7bd8..eb7cdf81b 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -122,6 +122,7 @@ public:
>
>  	const std::string &id() const;
>
> +	Signal<Request *, const ControlList &> metadataAvailable;
>  	Signal<Request *, FrameBuffer *> bufferCompleted;
>  	Signal<Request *> requestCompleted;
>  	Signal<> disconnected;
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index fb28a18d0..6c6cad66f 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -58,6 +58,7 @@ public:
>  	void registerRequest(Request *request);
>  	void queueRequest(Request *request);
>
> +	void completeMetadata(Request *request, const ControlList &metadata);

nit: "complete" seems to suggest we're done with it, while instead
the function should probably follow the signal name
"metadataAvailable()"

>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>  	void completeRequest(Request *request);
>  	void cancelRequest(Request *request);
> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
> index 4e7d05b1e..286cd9d76 100644
> --- a/include/libcamera/internal/request.h
> +++ b/include/libcamera/internal/request.h
> @@ -43,6 +43,8 @@ public:
>  	void prepare(std::chrono::milliseconds timeout = 0ms);
>  	Signal<> prepared;
>
> +	ControlList addCompletedMetadata(const ControlList &metadata);
> +
>  private:
>  	friend class PipelineHandler;
>  	friend std::ostream &operator<<(std::ostream &out, const Request &r);
> @@ -60,6 +62,8 @@ private:
>  	std::unordered_set<FrameBuffer *> pending_;
>  	std::map<FrameBuffer *, std::unique_ptr<EventNotifier>> notifiers_;
>  	std::unique_ptr<Timer> timer_;
> +
> +	std::unordered_set<unsigned int> completedMetadata_;
>  };
>
>  } /* namespace libcamera */
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index e214a9d13..2c78d9bb4 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -12,6 +12,7 @@
>  #include <ostream>
>  #include <stdint.h>
>  #include <string>
> +#include <unordered_set>

Not necessary anymore

>
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/signal.h>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 7507e9dda..22484721a 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -892,6 +892,12 @@ const std::string &Camera::id() const
>   * completed
>   */
>
> +/**
> + * \var Camera::metadataAvailable
> + * \brief Signal emitted when some metadata for a request is available as a
> + * partial result
> + */
> +
>  /**
>   * \var Camera::requestCompleted
>   * \brief Signal emitted when a request queued to the camera has completed
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 991b06f26..4ba96cfa2 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -531,6 +531,32 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  	return request->_d()->completeBuffer(buffer);
>  }
>
> +/**
> + * \brief Complete part of metadata for a request
> + * \param[in] request The request the metadata belongs to
> + * \param[in] metadata The partial metadata that has completed
> + *
> + * This function could be called by pipeline handlers to signal availability of
> + * \a metadata before \a request completes. Early metadata completion allows to
> + * notify applications about the availability of a partial metadata buffer
> + * before the associated Request has completed.
> + *
> + * A metadata key is expected to be completed at most once. If it's completed
> + * more than once, the key will be dropped since the second time.

* A metadata is expected to be completed at most once. If completed
* more than once it will be ignored.

> + *
> + * \context This function shall be called from the CameraManager thread.
> + */
> +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> +{
> +	const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
> +	if (!validMetadata.empty()) {
> +		request->metadata().merge(validMetadata);
> +
> +		Camera *camera = request->_d()->camera();
> +		camera->metadataAvailable.emit(request, validMetadata);
> +	}
> +}
> +
>  /**
>   * \brief Signal request completion
>   * \param[in] request The request that has completed
> @@ -553,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)
>
>  	Camera::Private *data = camera->_d();

Do not break the

	Camera::Private *data = camera->_d();

	while (!data->queuedRequests_.empty()) {

sequence

Move the below hunk before the declaration of *data
>
> +	/*
> +	 * Collect metadata which is not yet completed by the Camera, and
> +	 * create one partial result to cover the missing metadata before
> +	 * completing the whole request. This guarantees the aggregation of
> +	 * metadata in completed partial results equals to the global metadata
> +	 * in the request.
> +	 *
> +	 * \todo: Forbid merging metadata into request.metadata() directly and
> +	 * force calling completeMetadata() to report metadata.

This seems to suggest all pipelines will go through partial metadata
completion. I'm not 100% this will be the case.

> +	 */
> +	const ControlList validMetadata = request->_d()->addCompletedMetadata(
> +		request->metadata());
> +	if (!validMetadata.empty())
> +		camera->metadataAvailable.emit(request, validMetadata);
> +
>  	while (!data->queuedRequests_.empty()) {
>  		Request *req = data->queuedRequests_.front();
>  		if (req->status() == Request::RequestPending)
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 8c56ed30d..ae5cdeb19 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -178,6 +178,7 @@ void Request::Private::reset()
>  	pending_.clear();
>  	notifiers_.clear();
>  	timer_.reset();
> +	completedMetadata_.clear();
>  }
>
>  /*
> @@ -270,6 +271,26 @@ void Request::Private::prepare(std::chrono::milliseconds timeout)
>   * if they have failed preparing.
>   */
>
> +/**
> + * \brief Add completed metadata, as a partial result

What about:

 * \brief Accumulate metadata keys and compute a list of not yet notified metadata

> + * \param[in] metadata The metadata completed

The completed metadata list

> + *
> + * Request will record the entries that has been sent to the application, to
> + * prevent duplicated controls.

What about:

 * Accumulate the metadata keys from \a metadata in an internal list of
 * completed metadata and compute a list of metadata that has not yet been
 * notified to the application.
 *
 * A metadata can only be notified once and gets ignored if completed multiple
 * times.

> + *
> + * \return ControlList that hasn't been completed before

\return A ControlList of metadata that hasn't been notified to the
application yet

> + */
> +ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> +{
> +	ControlList resultMetadata;
> +	for (auto &[id, value] : metadata) {
> +		if (!completedMetadata_.count(id))

I might have missed where the id is added to completedMetadata_ ?

Thanks
   j

> +			resultMetadata.set(id, value);
> +	}
> +
> +	return resultMetadata;
> +}
> +
>  void Request::Private::notifierActivated(FrameBuffer *buffer)
>  {
>  	/* Close the fence if successfully signalled. */
> --
> 2.47.0.338.g60cca15819-goog
>