[PATCH 1/8] libcamera: camera: Introduce metadataAvailable signal
Barnabás Pőcze
pobrn at protonmail.com
Tue Jan 21 14:25:56 CET 2025
2025. január 21., kedd 13:52 keltezéssel, Cheng-Hao Yang <chenghaoyang at chromium.org> írta:
> Hi Barnabás,
>
> On Tue, Jan 21, 2025 at 11:32 AM Barnabás Pőcze <pobrn at protonmail.com> wrote:
> >
> > Hi
> >
> >
> > 2025. január 3., péntek 18:01 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> >
> > > 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).
> >
> > Based on my understanding, signals are delivered synchronously unless the target
> > object derives from `libcamera::Object`. So application callbacks will be invoked
> > directly from the CameraManager's thread (or whichever internal thread is used).
> > As far as I am aware, inheriting `libcamera::Object` in application code is not
> > supported and it just would not work currently because `libcamera::Thread` is not
> > public, so the application would have no way of running the internal event loop.
>
> In Android adapter though, it can directly use `libcamera::Thread` on
> CameraStream:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_stream.h#n17
>
> Also, other applications can use std::thread and cause similar issues,
> IIUC. It doesn't need to be `libcamera::Thread`, right?
>
> >
> >
> > >
> > > 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.
> >
> > Since the signal is dispatched synchronously, I think that is not possible*, so
> > the current setup might be acceptable. (Or am I missing something?) The burden of
> > synchronization falls fully on the user, but considering that the callback runs
> > in an internal thread, the user code would probably have to do some kind of
> > synchronization in any case.
>
> The user does have the responsibility to keep the synchronization,
> while as I mentioned above, pipeline handler also needs to avoid
> modifying metadata ControlList in other threads, right?
>
> >
> > [*]: Unless the user code saves iterators or references to the metadata control
> > list and reuses them after the signal handler returns in some other thread before
> > the request completes. But I am wondering if that is a significant concern?
> >
> > I suppose the parametrization of the signal could be changed to discourage the
> > direct use of `Request`. Although things like the request cookie would still
> > need to be available in some way.
> >
> > Another thing, libdbus "locks" messages when they are submitted, maybe a
> > similar idea could be implemented here as well with `Request`.
>
> In this case though, metadata would be updated and sent multiple
> times. This series of patches aims to reduce duplicating data, so it
> uses the same map. Does that mean we need to lock the whole map
> whenever accessing and modifying it?
Well, the "locking" comment was just a tangential idea, as a potential way to
minimize the misuse of a `Request` object that is in-flight.
As far as I understand, the way this patch series currently is, there is no need for
locking or extra synchronization when accessing the metadata inside the `metadataAvailable`
signal handler. However, as soon as one wants to access the metadata `ControlValue`s outside
the signal handler, then copies of the `ControlValue`s must be made (inside the signal handler).
Unfortunately, as far as I am aware, at the moment there is no concrete plan yet for addressing
this shortcoming that could be implemented at once.
Regards,
Barnabás Pőcze
>
> BR,
> Harvey
>
> >
> >
> > Regards,
> > Barnabás Pőcze
> >
> > >
> > > 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
> > > > >
> > >
>
More information about the libcamera-devel
mailing list