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

Cheng-Hao Yang chenghaoyang at chromium.org
Mon Jan 20 22:28:25 CET 2025


Hi Jacopo,

Friendly ping: Did you get more opinions from others?

On Fri, Jan 3, 2025 at 6:01 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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.

Yes, this is one of the issues, if we allow other threads to access ControlList.

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

Hmm, it might be a bit hacky though, and the base class might need to
know how many control ids will be set in advance.

BR,
Harvey

>
> 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
> > >


More information about the libcamera-devel mailing list