[PATCH 1/1] libcamera: Camera: Add signals for completion of metadata as a partial result
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Nov 14 09:36:35 CET 2024
Hi Harvey
On Thu, Nov 14, 2024 at 02:29:20PM +0800, Cheng-Hao Yang wrote:
> 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);`
>
Ups, sorry I missed it!
> 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.
>
Yes, if this time I got this right, the full metadata pack will be
available in Request::metadata() at requestCompleted time, but it will
be completed in chunks if the pipeline handler calls
completeMetadata() earlier.
There's only one thing left that I'm not sure about: in
requestCompleted()
const ControlList &validMetadata = request->addCompletedMetadata(
request->metadata());
if (!validMetadata.empty())
camera->metadataCompleted.emit(request, validMetadata);
If the pipeline handler never calls completeMetadata() then
validMetadata == request->metadata(). So that applications will
receive the same metadata buffer in metadataCompleted and
requestCompleted. I wonder if we should emit the metadataCompleted
signal at all if the pipeline handler never calls completeMetadata()
(it would be easy to check, just make sure that Request:completedMetadata_
is empty with an helper function).
> >
> > - 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`
> ?
>
ack
> >
> >
> > > + * \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?
>
I'll ask others what to they think
> >
> > > + *
> > > + * \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.
> > > + */
Could you please clarify me what this \todo means ?
> > > + 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?
>
Yes, they both do not need to be exposed to applications indeed.
> >
> > 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.
I can't find in the text where this is forbidden, however I'm not
pushing for the other option, I think it's actually saner to only
signal a metadata once per Request. I wonder however if this shouldn't
be a WARN as it means the pipeline handler is doing something
unexpected. Also, the expectations that a metadata is returned once
per Request has to be documented in the documentation of
PipelineHandler::completeMetadata()
Thanks
j
>
> 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