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

22112 diff mbox series

From: Han-Lin Chen <hanlinchen@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@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@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(+)

Hi Harvey

On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> From: Han-Lin Chen <hanlinchen@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@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@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
>

Hi Jacopo,

Will upload a new version in another series.

On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@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()"

Done

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

Removed

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

Updated.

>
> > + *
> > + * \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

Done

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

Let me remove the todo then?

>
> > +      */
> > +     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

Thanks! Done.

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

Updated.

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

Thanks! Done.

>
> > + *
> > + * \return ControlList that hasn't been completed before
>
> \return A ControlList of metadata that hasn't been notified to the
> application yet

Done

>
> > + */
> > +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_ ?

Nah, I should've added it here.
Updated.

Actually, I'm considering if we should keep the values in
`completedMetadata_`, and refresh `Request::metadata()` when calling
signal requestCompleted, to prevent the pipeline handler updating
metadata tags that have been sent to applications. This would ensure
the alignment of metadata between signal metadataAvailable and the
completed metadata in Request.
WDYT?

BR,
Harvey


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

Hi Harvey

On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> Hi Jacopo,
>
> Will upload a new version in another series.
>
> On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Harvey
> >
> > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
>
> Done
>
> >
> > >       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
>
> Removed
>
> >
> > >
> > >  #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.
>
> Updated.
>
> >
> > > + *
> > > + * \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
>
> Done
>
> > >
> > > +     /*
> > > +      * 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.
>
> Let me remove the todo then?
>
> >
> > > +      */
> > > +     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
>
> Thanks! Done.
>
> >
> > > + * \param[in] metadata The metadata completed
> >
> > The completed metadata list
>
> Updated.
>
> >
> > > + *
> > > + * 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.
>
> Thanks! Done.
>
> >
> > > + *
> > > + * \return ControlList that hasn't been completed before
> >
> > \return A ControlList of metadata that hasn't been notified to the
> > application yet
>
> Done
>
> >
> > > + */
> > > +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_ ?
>
> Nah, I should've added it here.
> Updated.
>
> Actually, I'm considering if we should keep the values in
> `completedMetadata_`, and refresh `Request::metadata()` when calling
> signal requestCompleted, to prevent the pipeline handler updating

Is this an "OR keep ids in completedMetadata_ OR accumulate them in
Request::metadata()" ?

> metadata tags that have been sent to applications. This would ensure
> the alignment of metadata between signal metadataAvailable and the
> completed metadata in Request.
> WDYT?

Unless there are reasons I am missing why this isn't possible, I think
having the accumulated metadata in Request::metadata() is a good idea.

Even more so as you're already doing it in
PipelineHandler::completeMetadata()

        void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
        {
             const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
             if (!validMetadata.empty()) {
                     request->metadata().merge(validMetadata);

Hence I think you can use Request::metadata_ to check what metadata
have already been completed with something like

ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
{
	ControlList resultMetadata;
	for (auto &[id, value] : metadata) {
		if (!metadata_.contains(id) && !resultMetadata.count(id))
			resultMetadata.set(id, value);
	}

        metadata_.merge(resultMetadata);

	return resultMetadata;
}

Do you think this could work ?

Thanks
  j

>
> BR,
> Harvey
>
>
> >
> > 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
> > >

Hi Jacopo,

On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> > Hi Jacopo,
> >
> > Will upload a new version in another series.
> >
> > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Harvey
> > >
> > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
> >
> > Done
> >
> > >
> > > >       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
> >
> > Removed
> >
> > >
> > > >
> > > >  #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.
> >
> > Updated.
> >
> > >
> > > > + *
> > > > + * \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
> >
> > Done
> >
> > > >
> > > > +     /*
> > > > +      * 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.
> >
> > Let me remove the todo then?
> >
> > >
> > > > +      */
> > > > +     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
> >
> > Thanks! Done.
> >
> > >
> > > > + * \param[in] metadata The metadata completed
> > >
> > > The completed metadata list
> >
> > Updated.
> >
> > >
> > > > + *
> > > > + * 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.
> >
> > Thanks! Done.
> >
> > >
> > > > + *
> > > > + * \return ControlList that hasn't been completed before
> > >
> > > \return A ControlList of metadata that hasn't been notified to the
> > > application yet
> >
> > Done
> >
> > >
> > > > + */
> > > > +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_ ?
> >
> > Nah, I should've added it here.
> > Updated.
> >
> > Actually, I'm considering if we should keep the values in
> > `completedMetadata_`, and refresh `Request::metadata()` when calling
> > signal requestCompleted, to prevent the pipeline handler updating
>
> Is this an "OR keep ids in completedMetadata_ OR accumulate them in
> Request::metadata()" ?

I meant keeping ids & values in `completedMetadata_`.

>
> > metadata tags that have been sent to applications. This would ensure
> > the alignment of metadata between signal metadataAvailable and the
> > completed metadata in Request.
> > WDYT?
>
> Unless there are reasons I am missing why this isn't possible, I think
> having the accumulated metadata in Request::metadata() is a good idea.
>
> Even more so as you're already doing it in
> PipelineHandler::completeMetadata()
>
>         void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
>         {
>              const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
>              if (!validMetadata.empty()) {
>                      request->metadata().merge(validMetadata);
>
> Hence I think you can use Request::metadata_ to check what metadata
> have already been completed with something like
>
> ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> {
>         ControlList resultMetadata;
>         for (auto &[id, value] : metadata) {
>                 if (!metadata_.contains(id) && !resultMetadata.count(id))
>                         resultMetadata.set(id, value);
>         }
>
>         metadata_.merge(resultMetadata);
>
>         return resultMetadata;
> }
>
> Do you think this could work ?

Exactly, that's what I was thinking. Thanks!
Only a nit that we need:
`metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`

Will include this in the next version.

BR,
Harvey

>
> Thanks
>   j
>
> >
> > BR,
> > Harvey
> >
> >
> > >
> > > 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
> > > >

Hi Jacopo,

On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote:
>
> Hi Jacopo,
>
> On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Harvey
> >
> > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> > > Hi Jacopo,
> > >
> > > Will upload a new version in another series.
> > >
> > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> > > <jacopo.mondi@ideasonboard.com> wrote:
> > > >
> > > > Hi Harvey
> > > >
> > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
> > >
> > > Done
> > >
> > > >
> > > > >       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
> > >
> > > Removed
> > >
> > > >
> > > > >
> > > > >  #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.
> > >
> > > Updated.
> > >
> > > >
> > > > > + *
> > > > > + * \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
> > >
> > > Done
> > >
> > > > >
> > > > > +     /*
> > > > > +      * 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.
> > >
> > > Let me remove the todo then?
> > >
> > > >
> > > > > +      */
> > > > > +     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
> > >
> > > Thanks! Done.
> > >
> > > >
> > > > > + * \param[in] metadata The metadata completed
> > > >
> > > > The completed metadata list
> > >
> > > Updated.
> > >
> > > >
> > > > > + *
> > > > > + * 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.
> > >
> > > Thanks! Done.
> > >
> > > >
> > > > > + *
> > > > > + * \return ControlList that hasn't been completed before
> > > >
> > > > \return A ControlList of metadata that hasn't been notified to the
> > > > application yet
> > >
> > > Done
> > >
> > > >
> > > > > + */
> > > > > +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_ ?
> > >
> > > Nah, I should've added it here.
> > > Updated.
> > >
> > > Actually, I'm considering if we should keep the values in
> > > `completedMetadata_`, and refresh `Request::metadata()` when calling
> > > signal requestCompleted, to prevent the pipeline handler updating
> >
> > Is this an "OR keep ids in completedMetadata_ OR accumulate them in
> > Request::metadata()" ?
>
> I meant keeping ids & values in `completedMetadata_`.
>
> >
> > > metadata tags that have been sent to applications. This would ensure
> > > the alignment of metadata between signal metadataAvailable and the
> > > completed metadata in Request.
> > > WDYT?
> >
> > Unless there are reasons I am missing why this isn't possible, I think
> > having the accumulated metadata in Request::metadata() is a good idea.
> >
> > Even more so as you're already doing it in
> > PipelineHandler::completeMetadata()
> >
> >         void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> >         {
> >              const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
> >              if (!validMetadata.empty()) {
> >                      request->metadata().merge(validMetadata);
> >
> > Hence I think you can use Request::metadata_ to check what metadata
> > have already been completed with something like
> >
> > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> > {
> >         ControlList resultMetadata;
> >         for (auto &[id, value] : metadata) {
> >                 if (!metadata_.contains(id) && !resultMetadata.count(id))
> >                         resultMetadata.set(id, value);
> >         }
> >
> >         metadata_.merge(resultMetadata);
> >
> >         return resultMetadata;
> > }
> >
> > Do you think this could work ?
>
> Exactly, that's what I was thinking. Thanks!
> Only a nit that we need:
> `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`
>
> Will include this in the next version.

Sorry, I just found that this may result in performance issues, when
we overwrite metadata values that are large in size repetitively. How
about we overwrite it once in `PipelineHandler::requestComplete`, and
expect applications wouldn't use `Request::metadata()` directly before
signal requestCompleted is invoked?

BR,
Harvey

>
> BR,
> Harvey
>
> >
> > Thanks
> >   j
> >
> > >
> > > BR,
> > > Harvey
> > >
> > >
> > > >
> > > > 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
> > > > >

Hi Harvey

On Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:
> Hi Jacopo,
>
> On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote:
> >
> > Hi Jacopo,
> >
> > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hi Harvey
> > >
> > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> > > > Hi Jacopo,
> > > >
> > > > Will upload a new version in another series.
> > > >
> > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> > > > <jacopo.mondi@ideasonboard.com> wrote:
> > > > >
> > > > > Hi Harvey
> > > > >
> > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > > > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > > > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
> > > >
> > > > Done
> > > >
> > > > >
> > > > > >       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
> > > >
> > > > Removed
> > > >
> > > > >
> > > > > >
> > > > > >  #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.
> > > >
> > > > Updated.
> > > >
> > > > >
> > > > > > + *
> > > > > > + * \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
> > > >
> > > > Done
> > > >
> > > > > >
> > > > > > +     /*
> > > > > > +      * 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.
> > > >
> > > > Let me remove the todo then?
> > > >
> > > > >
> > > > > > +      */
> > > > > > +     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
> > > >
> > > > Thanks! Done.
> > > >
> > > > >
> > > > > > + * \param[in] metadata The metadata completed
> > > > >
> > > > > The completed metadata list
> > > >
> > > > Updated.
> > > >
> > > > >
> > > > > > + *
> > > > > > + * 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.
> > > >
> > > > Thanks! Done.
> > > >
> > > > >
> > > > > > + *
> > > > > > + * \return ControlList that hasn't been completed before
> > > > >
> > > > > \return A ControlList of metadata that hasn't been notified to the
> > > > > application yet
> > > >
> > > > Done
> > > >
> > > > >
> > > > > > + */
> > > > > > +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_ ?
> > > >
> > > > Nah, I should've added it here.
> > > > Updated.
> > > >
> > > > Actually, I'm considering if we should keep the values in
> > > > `completedMetadata_`, and refresh `Request::metadata()` when calling
> > > > signal requestCompleted, to prevent the pipeline handler updating
> > >
> > > Is this an "OR keep ids in completedMetadata_ OR accumulate them in
> > > Request::metadata()" ?
> >
> > I meant keeping ids & values in `completedMetadata_`.
> >
> > >
> > > > metadata tags that have been sent to applications. This would ensure
> > > > the alignment of metadata between signal metadataAvailable and the
> > > > completed metadata in Request.
> > > > WDYT?
> > >
> > > Unless there are reasons I am missing why this isn't possible, I think
> > > having the accumulated metadata in Request::metadata() is a good idea.
> > >
> > > Even more so as you're already doing it in
> > > PipelineHandler::completeMetadata()
> > >
> > >         void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> > >         {
> > >              const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
> > >              if (!validMetadata.empty()) {
> > >                      request->metadata().merge(validMetadata);
> > >
> > > Hence I think you can use Request::metadata_ to check what metadata
> > > have already been completed with something like
> > >
> > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> > > {
> > >         ControlList resultMetadata;
> > >         for (auto &[id, value] : metadata) {
> > >                 if (!metadata_.contains(id) && !resultMetadata.count(id))
> > >                         resultMetadata.set(id, value);
> > >         }
> > >
> > >         metadata_.merge(resultMetadata);
> > >
> > >         return resultMetadata;
> > > }
> > >
> > > Do you think this could work ?
> >
> > Exactly, that's what I was thinking. Thanks!
> > Only a nit that we need:
> > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`
> >
> > Will include this in the next version.
>
> Sorry, I just found that this may result in performance issues, when
> we overwrite metadata values that are large in size repetitively. How
> about we overwrite it once in `PipelineHandler::requestComplete`, and
> expect applications wouldn't use `Request::metadata()` directly before
> signal requestCompleted is invoked?

I'm sorry but doesn't

         ControlList resultMetadata;
         for (auto &[id, value] : metadata) {
                 if (!metadata_.contains(id) && !resultMetadata.count(id))
                         resultMetadata.set(id, value);
         }

         metadata_.merge(resultMetadata);

mean we never overwrite an entry which is already part of metadata_ ?

>
> BR,
> Harvey
>
> >
> > BR,
> > Harvey
> >
> > >
> > > Thanks
> > >   j
> > >
> > > >
> > > > BR,
> > > > Harvey
> > > >
> > > >
> > > > >
> > > > > 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
> > > > > >

Hi Jacopo,

On Thu, Dec 5, 2024 at 11:21 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:
> > Hi Jacopo,
> >
> > On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote:
> > >
> > > Hi Jacopo,
> > >
> > > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
> > > <jacopo.mondi@ideasonboard.com> wrote:
> > > >
> > > > Hi Harvey
> > > >
> > > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > Will upload a new version in another series.
> > > > >
> > > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> > > > > <jacopo.mondi@ideasonboard.com> wrote:
> > > > > >
> > > > > > Hi Harvey
> > > > > >
> > > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > > > > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
> > > > >
> > > > > Done
> > > > >
> > > > > >
> > > > > > >       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
> > > > >
> > > > > Removed
> > > > >
> > > > > >
> > > > > > >
> > > > > > >  #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.
> > > > >
> > > > > Updated.
> > > > >
> > > > > >
> > > > > > > + *
> > > > > > > + * \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
> > > > >
> > > > > Done
> > > > >
> > > > > > >
> > > > > > > +     /*
> > > > > > > +      * 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.
> > > > >
> > > > > Let me remove the todo then?
> > > > >
> > > > > >
> > > > > > > +      */
> > > > > > > +     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
> > > > >
> > > > > Thanks! Done.
> > > > >
> > > > > >
> > > > > > > + * \param[in] metadata The metadata completed
> > > > > >
> > > > > > The completed metadata list
> > > > >
> > > > > Updated.
> > > > >
> > > > > >
> > > > > > > + *
> > > > > > > + * 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.
> > > > >
> > > > > Thanks! Done.
> > > > >
> > > > > >
> > > > > > > + *
> > > > > > > + * \return ControlList that hasn't been completed before
> > > > > >
> > > > > > \return A ControlList of metadata that hasn't been notified to the
> > > > > > application yet
> > > > >
> > > > > Done
> > > > >
> > > > > >
> > > > > > > + */
> > > > > > > +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_ ?
> > > > >
> > > > > Nah, I should've added it here.
> > > > > Updated.
> > > > >
> > > > > Actually, I'm considering if we should keep the values in
> > > > > `completedMetadata_`, and refresh `Request::metadata()` when calling
> > > > > signal requestCompleted, to prevent the pipeline handler updating
> > > >
> > > > Is this an "OR keep ids in completedMetadata_ OR accumulate them in
> > > > Request::metadata()" ?
> > >
> > > I meant keeping ids & values in `completedMetadata_`.
> > >
> > > >
> > > > > metadata tags that have been sent to applications. This would ensure
> > > > > the alignment of metadata between signal metadataAvailable and the
> > > > > completed metadata in Request.
> > > > > WDYT?
> > > >
> > > > Unless there are reasons I am missing why this isn't possible, I think
> > > > having the accumulated metadata in Request::metadata() is a good idea.
> > > >
> > > > Even more so as you're already doing it in
> > > > PipelineHandler::completeMetadata()
> > > >
> > > >         void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> > > >         {
> > > >              const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
> > > >              if (!validMetadata.empty()) {
> > > >                      request->metadata().merge(validMetadata);
> > > >
> > > > Hence I think you can use Request::metadata_ to check what metadata
> > > > have already been completed with something like
> > > >
> > > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> > > > {
> > > >         ControlList resultMetadata;
> > > >         for (auto &[id, value] : metadata) {
> > > >                 if (!metadata_.contains(id) && !resultMetadata.count(id))
> > > >                         resultMetadata.set(id, value);
> > > >         }
> > > >
> > > >         metadata_.merge(resultMetadata);
> > > >
> > > >         return resultMetadata;
> > > > }
> > > >
> > > > Do you think this could work ?
> > >
> > > Exactly, that's what I was thinking. Thanks!
> > > Only a nit that we need:
> > > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`
> > >
> > > Will include this in the next version.
> >
> > Sorry, I just found that this may result in performance issues, when
> > we overwrite metadata values that are large in size repetitively. How
> > about we overwrite it once in `PipelineHandler::requestComplete`, and
> > expect applications wouldn't use `Request::metadata()` directly before
> > signal requestCompleted is invoked?
>
> I'm sorry but doesn't
>
>          ControlList resultMetadata;
>          for (auto &[id, value] : metadata) {
>                  if (!metadata_.contains(id) && !resultMetadata.count(id))
>                          resultMetadata.set(id, value);
>          }
>
>          metadata_.merge(resultMetadata);
>
> mean we never overwrite an entry which is already part of metadata_ ?

Ah got it. I misunderstood your code.

Regarding `metadata_`, I assume you meant `_o<Request>()->metadata_`.

The problem is, currently pipeline handler implementations directly
update `Request::metadata_` instead of using
`PipelineHandler::metadataAvailable()`. What I want to do is, if a
pipeline handler implementation uses
`PipelineHandler::metadataAvailable()` to complete metadata id A and
value V1, and modify `Request::metadata_->set(A, V2)`, how do we
revert the value from V2 to V1 in
`PipelineHandler::completeRequest()`?

Also, in your implementation, if a pipeline handler implementation
does `Request::metadata_->set(A, V2)`, and uses
`PipelineHandler::metadataAvailable()` to complete metadata id A and
value V1, [A, V1] will be sent to the application with the new signal,
while `Request::metadata_.get(A)` will remain V2.

WDYT?

BR,
Harvey

>
> >
> > BR,
> > Harvey
> >
> > >
> > > BR,
> > > Harvey
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > >
> > > > > BR,
> > > > > Harvey
> > > > >
> > > > >
> > > > > >
> > > > > > 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
> > > > > > >

HI Harvey,

On Thu, Dec 05, 2024 at 11:40:18PM +0800, Cheng-Hao Yang wrote:
> Hi Jacopo,
>
> On Thu, Dec 5, 2024 at 11:21 PM Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hi Harvey
> >
> > On Thu, Dec 05, 2024 at 10:57:48PM +0800, Cheng-Hao Yang wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Dec 5, 2024 at 8:46 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote:
> > > >
> > > > Hi Jacopo,
> > > >
> > > > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
> > > > <jacopo.mondi@ideasonboard.com> wrote:
> > > > >
> > > > > Hi Harvey
> > > > >
> > > > > On Thu, Dec 05, 2024 at 01:22:32PM +0800, Cheng-Hao Yang wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > Will upload a new version in another series.
> > > > > >
> > > > > > On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
> > > > > > <jacopo.mondi@ideasonboard.com> wrote:
> > > > > > >
> > > > > > > Hi Harvey
> > > > > > >
> > > > > > > On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > > > > > > > From: Han-Lin Chen <hanlinchen@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@chromium.org>
> > > > > > > > Co-developed-by: Harvey Yang <chenghaoyang@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()"
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > >
> > > > > > > >       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
> > > > > >
> > > > > > Removed
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >  #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.
> > > > > >
> > > > > > Updated.
> > > > > >
> > > > > > >
> > > > > > > > + *
> > > > > > > > + * \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
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > >
> > > > > > > > +     /*
> > > > > > > > +      * 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.
> > > > > >
> > > > > > Let me remove the todo then?
> > > > > >
> > > > > > >
> > > > > > > > +      */
> > > > > > > > +     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
> > > > > >
> > > > > > Thanks! Done.
> > > > > >
> > > > > > >
> > > > > > > > + * \param[in] metadata The metadata completed
> > > > > > >
> > > > > > > The completed metadata list
> > > > > >
> > > > > > Updated.
> > > > > >
> > > > > > >
> > > > > > > > + *
> > > > > > > > + * 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.
> > > > > >
> > > > > > Thanks! Done.
> > > > > >
> > > > > > >
> > > > > > > > + *
> > > > > > > > + * \return ControlList that hasn't been completed before
> > > > > > >
> > > > > > > \return A ControlList of metadata that hasn't been notified to the
> > > > > > > application yet
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > >
> > > > > > > > + */
> > > > > > > > +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_ ?
> > > > > >
> > > > > > Nah, I should've added it here.
> > > > > > Updated.
> > > > > >
> > > > > > Actually, I'm considering if we should keep the values in
> > > > > > `completedMetadata_`, and refresh `Request::metadata()` when calling
> > > > > > signal requestCompleted, to prevent the pipeline handler updating
> > > > >
> > > > > Is this an "OR keep ids in completedMetadata_ OR accumulate them in
> > > > > Request::metadata()" ?
> > > >
> > > > I meant keeping ids & values in `completedMetadata_`.
> > > >
> > > > >
> > > > > > metadata tags that have been sent to applications. This would ensure
> > > > > > the alignment of metadata between signal metadataAvailable and the
> > > > > > completed metadata in Request.
> > > > > > WDYT?
> > > > >
> > > > > Unless there are reasons I am missing why this isn't possible, I think
> > > > > having the accumulated metadata in Request::metadata() is a good idea.
> > > > >
> > > > > Even more so as you're already doing it in
> > > > > PipelineHandler::completeMetadata()
> > > > >
> > > > >         void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> > > > >         {
> > > > >              const ControlList validMetadata = request->_d()->addCompletedMetadata(metadata);
> > > > >              if (!validMetadata.empty()) {
> > > > >                      request->metadata().merge(validMetadata);
> > > > >
> > > > > Hence I think you can use Request::metadata_ to check what metadata
> > > > > have already been completed with something like
> > > > >
> > > > > ControlList Request::Private::addCompletedMetadata(const ControlList &metadata)
> > > > > {
> > > > >         ControlList resultMetadata;
> > > > >         for (auto &[id, value] : metadata) {
> > > > >                 if (!metadata_.contains(id) && !resultMetadata.count(id))
> > > > >                         resultMetadata.set(id, value);
> > > > >         }
> > > > >
> > > > >         metadata_.merge(resultMetadata);
> > > > >
> > > > >         return resultMetadata;
> > > > > }
> > > > >
> > > > > Do you think this could work ?
> > > >
> > > > Exactly, that's what I was thinking. Thanks!
> > > > Only a nit that we need:
> > > > `metadata_.merge(resultMetadata, ControlList::MergePolicy::OverwriteExisting);`
> > > >
> > > > Will include this in the next version.
> > >
> > > Sorry, I just found that this may result in performance issues, when
> > > we overwrite metadata values that are large in size repetitively. How
> > > about we overwrite it once in `PipelineHandler::requestComplete`, and
> > > expect applications wouldn't use `Request::metadata()` directly before
> > > signal requestCompleted is invoked?
> >
> > I'm sorry but doesn't
> >
> >          ControlList resultMetadata;
> >          for (auto &[id, value] : metadata) {
> >                  if (!metadata_.contains(id) && !resultMetadata.count(id))
> >                          resultMetadata.set(id, value);
> >          }
> >
> >          metadata_.merge(resultMetadata);
> >
> > mean we never overwrite an entry which is already part of metadata_ ?
>
> Ah got it. I misunderstood your code.
>
> Regarding `metadata_`, I assume you meant `_o<Request>()->metadata_`.
>
> The problem is, currently pipeline handler implementations directly
> update `Request::metadata_` instead of using
> `PipelineHandler::metadataAvailable()`. What I want to do is, if a
> pipeline handler implementation uses
> `PipelineHandler::metadataAvailable()` to complete metadata id A and
> value V1, and modify `Request::metadata_->set(A, V2)`, how do we
> revert the value from V2 to V1 in
> `PipelineHandler::completeRequest()`?
>
> Also, in your implementation, if a pipeline handler implementation
> does `Request::metadata_->set(A, V2)`, and uses
> `PipelineHandler::metadataAvailable()` to complete metadata id A and
> value V1, [A, V1] will be sent to the application with the new signal,
> while `Request::metadata_.get(A)` will remain V2.
>
> WDYT?

give me a few days and I'll get back with a more consistent
proposal. I admit I have lost track of this code snippets sent
over email :)

Thanks
  j

>
> BR,
> Harvey
>
> >
> > >
> > > BR,
> > > Harvey
> > >
> > > >
> > > > BR,
> > > > Harvey
> > > >
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > > >
> > > > > > BR,
> > > > > > Harvey
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > 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
> > > > > > > >