[PATCH v2 1/9] libcamera: Camera: Add signals for completion of metadata as a partial result
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Dec 5 16:21:48 CET 2024
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 at chromium.org> wrote:
> >
> > Hi Jacopo,
> >
> > On Thu, Dec 5, 2024 at 4:00 PM Jacopo Mondi
> > <jacopo.mondi at 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 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
> > >
> > > 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
> > > > > >
More information about the libcamera-devel
mailing list