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

[Cheng-Hao Yang]

Hi Jacopo,

On Thu, Nov 14, 2024 at 6:17 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Thu, Nov 14, 2024 at 05:11:42PM +0800, Cheng-Hao Yang wrote:
> > Hi Jacopo,
> >
> > On Thu, Nov 14, 2024 at 4:36 PM Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > 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).
> >
> > I think this is how we expect a pipeline handler to behave and how an
> > application to handle signals.
>
> Do you expect all pipelines to use the new signal ?

Not really, while I still think that we need to ensure the new signal sends
a complete set of metadata keys.

>
> I quickly checked rkisp1 where most metadata are computed by the IPA
> in one go after parsing statistics. Only the frame timestamp and the
> scaler crop are available at different times (the buffer completion,
> so possibily later than the IPA computed ones).
>
> >
> > In the current implementation, it's safer for applications that whether
> > a pipeline handler calls completeMetadata, applications still get full
> > metadata set with the new signal, and they don't need to rely on
> > signal requestCompleted. In other words, I want to guarantee that
> > applications can choose to use either metadataAvailable or
> > requestCompleted.
> > Triggering the new signal if there's any metadata left is just a safer
> > option to support all pipeline handlers.
>
> My concern is about sending over a signal a possibly long list of
> controls two times in the case all metadata are sent in a single chunk
> at requestComplete() time.
>
> >
> > Of course we can choose to check if
> > `Request::Private::completedMetadata_` is empty, and only handle
> > this case, and expect the proper implementations of pipeline
> > handlers call completeMetadata properly without missing any
> > metadata. I just feel that it's unnecessary.
> > Also, I'm not sure if there's a case that pipeline handler doesn't
> > want to report some metadata earlier, and would rather send
> > them altogether when a request is completed. In this case,
> > your approach would require a pipeline handler to record
> > those unsent metadata on its own.
> >
> > WDYT?
> >
> > >
> > > > >
> > > > > - 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 ?
> >
> > Similar to the above reasons:
> > Currently all pipeline handlers merge metadata into request.metadata()
> > directly, except for the upcoming mtkisp7. We were expecting every
> > pipeline handler to migrate to `PipelineHandler::completeMetadata`
> > eventually.
> >
> > Let me know if you think this todo doesn't make sense, as we should
> > still allow the previous way to merge into request.metadata() directly,
> > or there's a case that a pipeline handler doesn't want to report some
> > metadata earlier.
> >
>
> I'm just not 100% sure all pipelines will be capable of actually send
> metadata earlier. True, in the RkISP1 case I used as an example as
> soon as the IPA has processed stats and computed metadata the pipeline
> can call metadataComplete() but we have no guarantee the application
> handles the signal, so we end up sending the same metadata list twice
> (one in chunks, the other as Request::metadata() at requestComplete()
> time.

I understand your concern, while `libcamera::Signal` doesn't do anything
if there's no slot connected. We still prepare the data chunks though.
We can also check if `libcamera::Signal::slots()` is empty before preparing
the data to be sent. WDYT?

BR,
Harvey

>
> I'll check with others what are the implications in terms of
> performances of this.
>
> Thanks
>   j
>
> > >
> > > > > > +     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.
> >
> > True that a WARN is an option.
> > I would prefer the current approach though, otherwise I need to
> > implement the logic in Android Adapter instead.
> >
> > Please let me know if anyone insists on the other approach.
> >
> > > Also, the expectations that a metadata is returned once
> > > per Request has to be documented in the documentation of
> > > PipelineHandler::completeMetadata()
> >
> > I'll update the doc in the next version :)
> >
> > BR,
> > Harvey
> >
> > >
> > > 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
> > > > > >