[PATCH 1/8] libcamera: camera: Introduce metadataAvailable signal

[Jacopo Mondi]

Hi Harvey

On Thu, Jan 02, 2025 at 05:13:28PM +0100, Cheng-Hao Yang wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Dec 6, 2024 at 5:07 PM Jacopo Mondi
> <jacopo.mondi at ideasonboard.com> wrote:
> >
> > Add a new signal to the Camera class that allows applications to
> > receive notifications for early completion of metadata results.
> >
> > To avoid expensive copies of the metadata results the signal
> > transports the ids of the metadata keys that are ready. Applications
> > can use these ids to access the metadata results from
> > Request::metadata().
>
> Yes, it avoids expensive copies, while I think we should restrict
> usages of `Request->metadata()` both in the application and pipeline
> handlers as well:
> 1. They should both access the metadata ControlList in the
> CameraManager's thread, which is the pipeline handlers' owner thread,
> to avoid race conditions. (I think `std::unordered_map` of
> `ControlList::controls_` is not thread-safe, as a set of the map might
> lead to rehashing. Please correct me if I am wrong.)

I think you're right, nice catch. Signals across threads are
asynchronous and there's a window indeed where the map can be accessed
by the app and the pipeline handler concurrently. Before this series
this was not an issue because apps only get the metadata list at
request completion time when the pipeline handler is done (or should
be done).

I think your concern is also supported by the following part of
https://en.cppreference.com/w/cpp/container/unordered_map/operator_at

-------------------------------------------------------------------------------
If after the operation the new number of elements is greater than old
max_load_factor() * bucket_count() a rehashing takes place.  If
rehashing occurs (due to the insertion), all iterators are
invalidated. Otherwise (no rehashing), iterators are not invalidated.
-------------------------------------------------------------------------------

> 2. Pipeline handlers should not directly modify `Request->metadata()`,
> to avoid overwriting. Instead, they should utilize the helper
> functions `metadataAvailable` in the third patch in this series.
>
> I've seen (2.) has been implemented in the following patches, thanks.
> Do you think we can ensure (1.) though? Maybe at least documenting it?

I'm afraid documenting it is not enough, access to the metadata list
should be locked if the list is made available to the application before
the request has completed...

I initially thought this wouldn't be hard. After all, in this series
pipeline handlers can access Request::metadata_ in rw mode only
through PipelineHandler::metadataAvailable() we could simply lock
there and lock Request::metadata().

However ControlList provides const iterators and an application might
decide to iterate the control list while the pipeline handler
populates it with more metadata.

I wonder what are the consequences of increasing
https://en.cppreference.com/w/cpp/container/unordered_map/max_load_factor
to avoid rehashing...

I'll ask around!

Thanks!
  j

>
> BR,
> Harvey
>
> >
> > The signal is an opt-in feature for applications and the sum of
> > all metadata results notified through this signal is available
> > in Request::metadata() at request completion time.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > ---
> >  include/libcamera/camera.h |  2 ++
> >  src/libcamera/camera.cpp   | 42 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 94cee7bd86bb..20d51c191ecd 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -13,6 +13,7 @@
> >  #include <set>
> >  #include <stdint.h>
> >  #include <string>
> > +#include <unordered_set>
> >
> >  #include <libcamera/base/class.h>
> >  #include <libcamera/base/flags.h>
> > @@ -122,6 +123,7 @@ public:
> >
> >         const std::string &id() const;
> >
> > +       Signal<Request *, std::unordered_set<const ControlId *>> metadataAvailable;
> >         Signal<Request *, FrameBuffer *> bufferCompleted;
> >         Signal<Request *> requestCompleted;
> >         Signal<> disconnected;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 4c865a46af53..63e78b24e271 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -886,6 +886,48 @@ const std::string &Camera::id() const
> >         return _d()->id_;
> >  }
> >
> > +/**
> > + * \var Camera::metadataAvailable
> > + * \brief Signal emitted when metadata for a request are available
> > + *
> > + * The metadataAvailable signal notifies applications about the availability
> > + * of metadata for a request before the request completes.
> > + *
> > + * As metadata results could be large in size, the signal transports the ids
> > + * of the metadata that have just been made available, but the actual control
> > + * values are stored in the Camera::metadata() list.
> > + *
> > + * Applications can access the value of the newly available metadata results
> > + * with:
> > + *
> > + * \code
> > +
> > +       void metadataAvailableHandler(Request *request,
> > +                                     std::unordered_set<const ControlId *> ids)
> > +       {
> > +               const ControlList &metadata = request->metadata();
> > +
> > +               for (const auto id : ids) {
> > +                       ControlValue &value = metadata.get(id->id());
> > +
> > +                       ....
> > +               }
> > +       }
> > +   \endcode
> > + *
> > + * This signal is emitted multiple times for the same request, it is in facts
> > + * emitted by the framework every time a new metadata list is made available
> > + * by the Camera to the application.
> > + *
> > + * The sum of all metadata lists reported through this signal is equal to
> > + * Request::metadata() list when the Request completes.
> > + *
> > + * Application can opt-in to handle this signal to receive fast notifications
> > + * of metadata availability or can equally access the full metadata list
> > + * at Request complete time through Request::metadata() if they have no interest
> > + * in early metadata notification.
> > + */
> > +
> >  /**
> >   * \var Camera::bufferCompleted
> >   * \brief Signal emitted when a buffer for a request queued to the camera has
> > --
> > 2.47.1
> >