[PATCH 1/1] libcamera: Camera: Add signals for completion of metadata as a partial result
Cheng-Hao Yang
chenghaoyang at chromium.org
Thu Nov 14 07:29:20 CET 2024
Hi Jacopo,
I'll upload a new version when all discussion is done.
On Wed, Nov 13, 2024 at 6:47 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Harvey,
> sorry for the late feedback
>
> On Thu, Sep 12, 2024 at 04:52:48AM +0000, Harvey Yang wrote:
> > From: Han-Lin Chen <hanlinchen at chromium.org>
> >
> > Allows pipeline handler to signal metadata completion by adding the
> > following signals to Camera:
> >
> > Signal<Request *, const ControlList &> metadataCompleted;
>
> Bikeshedding on the name a bit, as this is allowed to be called
> multiple times, with partial metadata, I would call it
> 'metadataAvailable'.
Sure, 'metadataAvailable' does make more sense.
Thanks!
>
> >
> > Together with the bufferCompleted signal, the pipeline handler is allowed to
> > return buffers and partial metadata at any stage of processing.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> > include/libcamera/camera.h | 1 +
> > include/libcamera/internal/pipeline_handler.h | 1 +
> > include/libcamera/request.h | 5 +++
> > src/libcamera/camera.cpp | 6 +++
> > src/libcamera/pipeline_handler.cpp | 37 +++++++++++++++++++
> > src/libcamera/request.cpp | 20 ++++++++++
> > 6 files changed, 70 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 94cee7bd..08c5c58c 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -122,6 +122,7 @@ public:
> >
> > const std::string &id() const;
> >
> > + Signal<Request *, const ControlList &> metadataCompleted;
> > Signal<Request *, FrameBuffer *> bufferCompleted;
> > Signal<Request *> requestCompleted;
> > Signal<> disconnected;
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 0d380803..1af63a23 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -58,6 +58,7 @@ public:
> > void registerRequest(Request *request);
> > void queueRequest(Request *request);
> >
> > + void completeMetadata(Request *request, const ControlList &metadata);
> > bool completeBuffer(Request *request, FrameBuffer *buffer);
> > void completeRequest(Request *request);
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index e214a9d1..0200f4a1 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -12,6 +12,7 @@
> > #include <ostream>
> > #include <stdint.h>
> > #include <string>
> > +#include <unordered_set>
> >
> > #include <libcamera/base/class.h>
> > #include <libcamera/base/signal.h>
> > @@ -64,6 +65,8 @@ public:
> >
> > std::string toString() const;
> >
> > + ControlList addCompletedMetadata(const ControlList &metadata);
> > +
> > private:
> > LIBCAMERA_DISABLE_COPY(Request)
> >
> > @@ -73,6 +76,8 @@ private:
> >
> > const uint64_t cookie_;
> > Status status_;
> > +
> > + std::unordered_set<unsigned int> completedMetadata_;
> > };
> >
> > std::ostream &operator<<(std::ostream &out, const Request &r);
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index a86f552a..5ffae23e 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -892,6 +892,12 @@ const std::string &Camera::id() const
> > * completed
> > */
> >
> > +/**
> > + * \var Camera::metadataCompleted
> > + * \brief Signal emitted when some metadata for a request is completed as a
> > + * partial result
>
> Currently, we return metadata as part of a Request signalled with
> Camera::requestCompleted. Existing applications do not handle the new
> signal and I don't think we should make it mandatory to do so.
> According to the implementation of this patch, the same application
> running on a pipeline handler that calls
> PipelineHandler::completeMetadata and on a pipeline handler that
> doesn't will receive different sets of metadata.
>
> - If a pipeline handler calls PipelineHandler::completeMetadata() it
> will send metadata in chunks and only the ones not previously
> signalled with be part of Request::metadata().
Actually, it's not true.
In this patch, `PipelineHandler::completeMetadata` will merge the
partially available metadata into Request:
`request->metadata().merge(validMetadata);`
Therefore, whether an application handles the new signal, it can
still get access to all the metadata in a request via
`Request.metadata()`. This should ease your concern of the old
and new implementations of applications.
>
> - If a pipeline handler doesn't call PipelineHandler::completeMetadata()
> the full metadata buffer will be in Request::metadata().
>
> How does an application know what to expect ?
>
> Imo we should advertise if the camera supports partial metadata or not
> (maybe through a property). If it does applications should opt-in to
> receive partial metadata. This could even be done per-configuration by
> adding a field to CameraConfiguration ?
>
> If applications enable early metdata completion they agree to handle
> the new signal, and the behaviour will be the one implemented in this
> patch. If they do not enable early metadata completion then
> PipelineHandler::completeMetadata() should simply merge the partial
> metadata in Request::metadata() and not emit the new signal but return
> the full metadata buffer as part of the Request in requestCompleted,
> as they currently do so that pipelines that use early completion and
> pipelines that do not will behave identically from an application
> perspective.
>
> What do you think ?
>
> > + */
> > +
> > /**
> > * \var Camera::requestCompleted
> > * \brief Signal emitted when a request queued to the camera has completed
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index e5940469..5d2999cb 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -535,6 +535,28 @@ bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> > return request->_d()->completeBuffer(buffer);
> > }
> >
> > +/**
> > + * \brief Complete part of metadata for a request
> > + * \param[in] request The request the buffer belongs to
>
> * \param[in] request The Request the metadata buffer belongs to
Perhaps:
`* \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 completion of
> > + * the \a metadata part of the \a request. It notifies applications of metadata
> > + * completion.
>
> I think a little more details are needed.
>
> * This function could be called by pipeline handlers to signal
> * availability of \a metadata before \a request completes. Early
> * metadata completion allows to notify applications about the
> * availability of a partial metadata buffer before the associated
> * Request has completed.
Thanks! Updated.
>
> 1) According to this implementation metadata can be completed
> out-of-order (partial metadata for Request X+1 can be signalled before
> Request X completes). Is this desirable ?
Yes, this is what we expect, and Android also allows that.
WDYT?
>
> > + *
> > + * \context This function shall be called from the CameraManager thread.
> > + */
> > +void PipelineHandler::completeMetadata(Request *request, const ControlList &metadata)
> > +{
> > + const ControlList &validMetadata = request->addCompletedMetadata(metadata);
>
> Request::addCompleteMetadata constructs and returns a ControlList,
> I'm surprised the compiler doesn't complain you're taking a reference
> to a temporary object (it's apparently legal for const references only).
>
> However, as a temporary will be constructed anyway this doesn't
> buy you any gain, possibily the contrary, as this will disallow C++
> copy elision from happening (afaiu). I think you should drop &
Right, it's definitely a mistake. Removed.
>
>
> > + if (!validMetadata.empty()) {
> > + request->metadata().merge(validMetadata);
> > +
> > + Camera *camera = request->_d()->camera();
> > + camera->metadataCompleted.emit(request, validMetadata);
> > + }
> > +}
> > +
> > /**
> > * \brief Signal request completion
> > * \param[in] request The request that has completed
> > @@ -557,6 +579,21 @@ void PipelineHandler::completeRequest(Request *request)
> >
> > Camera::Private *data = camera->_d();
> >
> > + /*
> > + * Collect metadata which is not yet completed by the Camera, and
> > + * create one partial result to cover the missing metadata before
> > + * completing the whole request. This guarantees the aggregation of
> > + * metadata in completed partial results equals to the global metadata
> > + * in the request.
> > + *
> > + * \todo: Forbid merging metadata into request.metadata() directly and
> > + * force calling completeMetadata() to report metadata.
> > + */
> > + const ControlList &validMetadata = request->addCompletedMetadata(
> > + request->metadata());
> > + if (!validMetadata.empty())
> > + camera->metadataCompleted.emit(request, validMetadata);
> > +
> > while (!data->queuedRequests_.empty()) {
> > Request *req = data->queuedRequests_.front();
> > if (req->status() == Request::RequestPending)
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 8c56ed30..3ce23a8e 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -598,6 +598,26 @@ std::string Request::toString() const
> > return ss.str();
> > }
> >
> > +/**
> > + * \brief Add completed metadata, as a partial result
> > + * \param[in] metadata The metadata completed
> > + *
> > + * Request will record the entries that has been sent to the application, to
> > + * prevent duplicated controls.
> > + *
> > + * \return ControlList that hasn't been completed before
> > + */
> > +ControlList Request::addCompletedMetadata(const ControlList &metadata)
> > +{
> > + ControlList resultMetadata;
> > + for (auto &[id, value] : metadata) {
> > + if (!completedMetadata_.count(id))
>
> completedMetadata_ should be cleared in Request::reset().
Nice catch! Then how about we move the member variable and the member
function to `Request::Private` instead?
>
> I wonder if we should actually prevent the same metadata to be
> returned multiple times by the pipeline handler or not.
This is actually prohibited by Android camera hal [1]:
`Partial metadata submitted should not include any metadata key
returned in a previous partial result for a given frame.`
We can certainly prevent that in Android Adapter only, while I think
having this restriction in libcamera core library also makes sense.
WDYT? I can also update the description of the new signal.
[1]: https://source.android.com/reference/hal/structcamera3__capture__result#:~:text=partial_result-,Detailed%20Description,the%20result%20metadata%20to%20NULL.
BR,
Harvey
>
> > + resultMetadata.set(id, value);
> > + }
> > +
> > + return resultMetadata;
> > +}
> > +
> > /**
> > * \brief Insert a text representation of a Request into an output stream
> > * \param[in] out The output stream
> > --
> > 2.46.0.598.g6f2099f65c-goog
> >
More information about the libcamera-devel
mailing list