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

[Cheng-Hao Yang]

Hi Jacopo,

Will upload a new version in another series.

On Thu, Nov 28, 2024 at 5:01 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Wed, Nov 27, 2024 at 09:25:51AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen at chromium.org>
> >
> > Allows pipeline handler to signal metadata completion by adding the
> > following signals to Camera:
> >
> > Signal<Request *, const ControlList &> metadataAvailable;
> >
> > Together with the bufferCompleted signal, the pipeline handler is allowed to
> > return buffers and partial metadata at any stage of processing.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  include/libcamera/camera.h                    |  1 +
> >  include/libcamera/internal/pipeline_handler.h |  1 +
> >  include/libcamera/internal/request.h          |  4 ++
> >  include/libcamera/request.h                   |  1 +
> >  src/libcamera/camera.cpp                      |  6 +++
> >  src/libcamera/pipeline_handler.cpp            | 41 +++++++++++++++++++
> >  src/libcamera/request.cpp                     | 21 ++++++++++
> >  7 files changed, 75 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 94cee7bd8..eb7cdf81b 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -122,6 +122,7 @@ public:
> >
> >       const std::string &id() const;
> >
> > +     Signal<Request *, const ControlList &> metadataAvailable;
> >       Signal<Request *, FrameBuffer *> bufferCompleted;
> >       Signal<Request *> requestCompleted;
> >       Signal<> disconnected;
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index fb28a18d0..6c6cad66f 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -58,6 +58,7 @@ public:
> >       void registerRequest(Request *request);
> >       void queueRequest(Request *request);
> >
> > +     void completeMetadata(Request *request, const ControlList &metadata);
>
> nit: "complete" seems to suggest we're done with it, while instead
> the function should probably follow the signal name
> "metadataAvailable()"

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