[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 09:00:35 CET 2024
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()" ?
> 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
> > >
More information about the libcamera-devel
mailing list