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

[Jacopo Mondi]

Hi Harvey,
   sorry for the late feedback

On Thu, Sep 12, 2024 at 04:52:48AM +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 &> metadataCompleted;

Bikeshedding on the name a bit, as this is allowed to be called
multiple times, with partial metadata, I would call it
'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/request.h                   |  5 +++
>  src/libcamera/camera.cpp                      |  6 +++
>  src/libcamera/pipeline_handler.cpp            | 37 +++++++++++++++++++
>  src/libcamera/request.cpp                     | 20 ++++++++++
>  6 files changed, 70 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 94cee7bd..08c5c58c 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 &> metadataCompleted;
>  	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 0d380803..1af63a23 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);
>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
>  	void completeRequest(Request *request);
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index e214a9d1..0200f4a1 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>
>
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/signal.h>
> @@ -64,6 +65,8 @@ public:
>
>  	std::string toString() const;
>
> +	ControlList addCompletedMetadata(const ControlList &metadata);
> +
>  private:
>  	LIBCAMERA_DISABLE_COPY(Request)
>
> @@ -73,6 +76,8 @@ private:
>
>  	const uint64_t cookie_;
>  	Status status_;
> +
> +	std::unordered_set<unsigned int> completedMetadata_;
>  };
>
>  std::ostream &operator<<(std::ostream &out, const Request &r);
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index a86f552a..5ffae23e 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -892,6 +892,12 @@ const std::string &Camera::id() const
>   * completed
>   */
>
> +/**
> + * \var Camera::metadataCompleted
> + * \brief Signal emitted when some metadata for a request is completed as a
> + * partial result

Currently, we return metadata as part of a Request signalled with
Camera::requestCompleted. Existing applications do not handle the new
signal and I don't think we should make it mandatory to do so.
According to the implementation of this patch, the same application
running on a pipeline handler that calls
PipelineHandler::completeMetadata and on a pipeline handler that
doesn't will receive different sets of metadata.

- If a pipeline handler calls PipelineHandler::completeMetadata() it
  will send metadata in chunks and only the ones not previously
  signalled with be part of Request::metadata().

- If a pipeline handler doesn't call PipelineHandler::completeMetadata()
  the full metadata buffer will be in Request::metadata().

How does an application know what to expect ?

Imo we should advertise if the camera supports partial metadata or not
(maybe through a property). If it does applications should opt-in to
receive partial metadata. This could even be done per-configuration by
adding a field to CameraConfiguration ?

If applications enable early metdata completion they agree to handle
the new signal, and the behaviour will be the one implemented in this
patch. If they do not enable early metadata completion then
PipelineHandler::completeMetadata() should simply merge the partial
metadata in Request::metadata() and not emit the new signal but return
the full metadata buffer as part of the Request in requestCompleted,
as they currently do so that pipelines that use early completion and
pipelines that do not will behave identically from an application
perspective.

What do you think ?

> + */
> +
>  /**
>   * \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 e5940469..5d2999cb 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -535,6 +535,28 @@ 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 buffer belongs to

    * \param[in] request The Request the metadata buffer belongs to


> + * \param[in] metadata The partial metadata that has completed
> + *
> + * This function could be called by pipeline handlers to signal completion of
> + * the \a metadata part of the \a request. It notifies applications of metadata
> + * completion.

I think a little more details are needed.

    * 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.

1) According to this implementation metadata can be completed
out-of-order (partial metadata for Request X+1 can be signalled before
Request X completes). Is this desirable ?

> + *
> + * \context This function shall be called from the CameraManager thread.
> + */
> +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> +{
> +	const ControlList &validMetadata = request->addCompletedMetadata(metadata);

Request::addCompleteMetadata constructs and returns a ControlList,
I'm surprised the compiler doesn't complain you're taking a reference
to a temporary object (it's apparently legal for const references only).

However, as a temporary will be constructed anyway this doesn't
buy you any gain, possibily the contrary, as this will disallow C++
copy elision from happening (afaiu). I think you should drop &


> +	if (!validMetadata.empty()) {
> +		request->metadata().merge(validMetadata);
> +
> +		Camera *camera = request->_d()->camera();
> +		camera->metadataCompleted.emit(request, validMetadata);
> +	}
> +}
> +
>  /**
>   * \brief Signal request completion
>   * \param[in] request The request that has completed
> @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)
>
>  	Camera::Private *data = camera->_d();
>
> +	/*
> +	 * 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.
> +	 */
> +	const ControlList &validMetadata = request->addCompletedMetadata(
> +		request->metadata());
> +	if (!validMetadata.empty())
> +		camera->metadataCompleted.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 8c56ed30..3ce23a8e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -598,6 +598,26 @@ std::string Request::toString() const
>  	return ss.str();
>  }
>
> +/**
> + * \brief Add completed metadata, as a partial result
> + * \param[in] metadata The metadata completed
> + *
> + * Request will record the entries that has been sent to the application, to
> + * prevent duplicated controls.
> + *
> + * \return ControlList that hasn't been completed before
> + */
> +ControlList Request::addCompletedMetadata(const ControlList &metadata)
> +{
> +	ControlList resultMetadata;
> +	for (auto &[id, value] : metadata) {
> +		if (!completedMetadata_.count(id))

completedMetadata_ should be cleared in Request::reset().

I wonder if we should actually prevent the same metadata to be
returned multiple times by the pipeline handler or not.

> +			resultMetadata.set(id, value);
> +	}
> +
> +	return resultMetadata;
> +}
> +
>  /**
>   * \brief Insert a text representation of a Request into an output stream
>   * \param[in] out The output stream
> --
> 2.46.0.598.g6f2099f65c-goog
>