[libcamera-devel] [PATCH v4 20/37] libcamera: IPAInterface: remove ipa_context and functions from documentation
Jacopo Mondi
jacopo at jmondi.org
Wed Nov 18 11:41:50 CET 2020
Hi Paul,
On Wed, Nov 18, 2020 at 07:03:45PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Jacopo,
>
> On Tue, Nov 17, 2020 at 05:49:01PM +0100, Jacopo Mondi wrote:
> > Hi Paul,
> > nice cleanup
> >
> > On Fri, Nov 06, 2020 at 07:36:50PM +0900, Paul Elder wrote:
> > > Remove all the documentation related to ipa_context and the C IPA API,
> > > as well as the documentation about the functions in the IPAInterface.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > No change in v4
> > >
> > > No change in v3
> > >
> > > New in v2
> > > ---
> > > src/libcamera/ipa_interface.cpp | 517 +++-----------------------------
> > > 1 file changed, 34 insertions(+), 483 deletions(-)
> > >
> > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp
> > > index 23fc56d7..d989c75d 100644
> > > --- a/src/libcamera/ipa_interface.cpp
> > > +++ b/src/libcamera/ipa_interface.cpp
> > > @@ -15,371 +15,52 @@
[snip]
> > > *
> > > * \code{.c}
> > > - * struct ipa_context *ipaCreate();
> > > + * IPAInterface *ipaCreate();
> > > * \endcode
> > > *
> > > - * The ipaCreate() function creates an instance of an IPA context, which models
> > > + * The ipaCreate() function creates an instance of an IPA interface, which models
> > > * a context of execution for the IPA. IPA modules shall support creating one
> > > * context per camera, as required by their associated pipeline handler.
> > > *
> > > - * The IPA module context operations are defined in the struct ipa_context_ops.
> > > - * They model a low-level interface to configure the IPA, notify it of events,
> > > - * and receive IPA actions through callbacks. An IPA module stores a pointer to
> > > - * the operations corresponding to its context in the ipa_context::ops field.
> > > - * That pointer is immutable for the lifetime of the context, and may differ
> > > - * between different contexts created by the same IPA module.
> > > + * The IPA module interface operations are defined in the mojom file
> > > + * corresponding to the pipeline handler, in
> > > + * include/libcamera/ipa/{pipeline_name}.mojom. These interface operations and
> > > + * their parameters are completely customizable, and are meant to be written
> > > + * by the pipeline author.
> >
> > I would drop the last phrase from 'These..' on maybe ?
>
> Hm, okay.
>
If you feel it has value, feel free to keep it
> > > *
> > > * The IPA interface defines base data types and functions to exchange data. On
> > > * top of this, each pipeline handler is responsible for defining the set of
> > > * events and actions used to communicate with their IPA. These are collectively
> > > * referred to as IPA operations and define the pipeline handler-specific IPA
> > > * protocol. Each operation defines the data that it carries, and how that data
> > > - * is encoded in the ipa_context_ops functions arguments.
> > > + * is encoded in the mojom file.
> >
> > Is data encoding part of the mojom file?
>
> I suppose "data encoding" isn't the proper word :/
> It defines the interface, and any structs that are used.
>
> > I would here as well drop the last three lines as of course operations
> > have parameters.
>
> Ah, yes :)
>
So just re-stating that the 'IPA protocol is defined in the mojom
file' should be enough ?
> > > *
> > > * \todo Add reference to how pipelines shall document their protocol.
> >
> > Does this still apply ?
>
> tbh, I think it does. Listing the function prototypes in the IPA
> interface isn't sufficient for explaining how they should be used.
>
Ack, let's keep it
> > > *
> > > * IPAs can be isolated in a separate process. This implies that arguments to
> > > - * the IPA interface functions may need to be transferred over IPC. All
> > > - * arguments use Plain Old Data types and are documented either in the form of C
> > > - * data types, or as a textual description of byte arrays for types that can't
> > > - * be expressed using C data types (such as arrays of mixed data types). IPA
> > > - * modules can thus use the C API without calling into libcamera to access the
> > > - * data passed to the IPA context operations.
> > > + * the IPA interface functions may need to be transferred over IPC. An IPA
> > > + * proxy is auto-generated based on the mojom file, which abstracts away the
> > > + * de/serialization from the pipeline handler and the IPA implementation. Thus
> > > + * any C++ struct that is defined in the mojom file, or the C++ libcamera
> > > + * objects that are mentioned in core.mojom, can be used directly.
> >
> > I still fail to see where the isolation flag is collected from, that's
> > why I skept review of a previous patch that add the parameter
>
> In ipa_manager.h, in IPAManager::createIPA:
>
> IPAModule *m = nullptr;
> ...
> std::unique_ptr<P> proxy = std::make_unique<P>(m, !self_->isSignatureValid(m));
>
> I don't think checking the environment variable for isolation is supported yet.
>
> So in the raspberrypi pipeline handler, for example, you have in loadIPA():
>
> ipa_ = IPAManager::createIPA<ipa::rpi::IPAProxyRPi>(pipe_, 1, 1);
>
> The IPAManager handles isolation, by passing it as a boolean to the
> Proxy constructor. The Proxy has an internal switch for thread mode
> vs process mode, stored in a private const bool isolate_.
>
Thanks!
[snip]
> > > -/**
> > > - * \var ipa_context_ops::process_event
> > > - * \brief Process an event from the pipeline handler
> > > - * \param[in] ctx The IPA context
> > > - *
> > > - * \sa libcamera::IPAInterface::processEvent()
> > > + * The IPA interface, as defined in the respective mojom file, is the API
> > > + * exposed to pipeline handlers to communicate with IPA modules. IPA modules
> > > + * may use the IPAInterface API internally if they want to benefit from the
> > > + * data and helper classes offered by libcamera.
> >
> > I'm not sure I got what you mean with "IPAs using the IPAInterface
> > internally". Isn't that the only way to communicate back with the
> > pipieline handler ?
>
> Now that you mention it, I can't actually remember what this was
> supposed to mean...
>
> > Anyway, I still wonder how this works for IPAs that do not link
> > against libcamera and do not use libcamera defined structures and
> > types.
>
> If the IPA interface uses libcamera-specific structs... then all IPA
> implementations will have to use that. I think we alrady have that issue
> with ControlLists. I'm not sure what the solution is.
>
Well, the IPA interface can very well use libcamera types, from my
recollection of the initial design the 'issue' is on the other side.
If an IPA doesn't link against libcamera (and can potentially be
written in plain C or other languages) it needs to implement
de-serialization of each type by itself. After all they receive a byte
array from a socket, and from there on they're free to interpret those
data as they like.
My question is how does this play with workers, as the 'IPA side' of
the IPC should be handled by the IPA code that does not link against
libcamera. I assume 'isolated and non-linked' IPA would have to
implement the worker part as part of the IPA itself.
In example, they can't do (from ipa_proxy_raspberrypi_worker.cpp)
IPASettings settings = IPADataSerializer<IPASettings>::deserialize(
_message.data.begin() + settingsStart,
_message.data.end());
int32_t _callRet = ipa_->init(settings);
As they don't have access to anything-libcamera, so they have to read
the byte array from the socket (or else), interpret the data and run
the IPA code on their custom de-serialized representation of the
libcamera serialized types.
I'm not sure if 'non-linked' IPAs are still a thing though. It's mostly
about licensing concern from vendors that fear LGPL is not enough to
guarantee nobody could ever claim access to their precious secret sauce.
It's not something we should however consider in the design of the IPA
IPC as they're ruled out. Probably they won't even use the IPA
interface generator or any of this facilities (if not the
serializers/deserializers on the pipeline handler side, the IPA proxy
maybe ?)
> > > */
> > >
> > > /**
> > > @@ -387,9 +68,10 @@
> > > * \brief Entry point to the IPA modules
> > > *
> > > * This function is the entry point to the IPA modules. It is implemented by
> > > - * every IPA module, and called by libcamera to create a new IPA context.
> > > + * every IPA module, and called by libcamera to create a new IPA interface
> > > + * instance.
> > > *
> > > - * \return A newly created IPA context
> > > + * \return A newly created IPA interface instance
> > > */
> > >
> > > namespace libcamera {
> > > @@ -499,157 +181,26 @@ namespace libcamera {
> > > * \class IPAInterface
> > > * \brief C++ Interface for IPA implementation
> > > *
> > > - * This pure virtual class defines a C++ API corresponding to the ipa_context,
> > > - * ipa_context_ops and ipa_callback_ops API. It is used by pipeline handlers to
> > > + * This pure virtual class defines a skeletal C++ API for IPA modules.
> > > + * Specializations of this class must be defined in a mojom file in
> > > + * include/libcamera/ipa/ (see include/libcamera/ipa/core.mojom for details
> > > + * on how to do so). The specialized class is used by pipeline handlers to
> > > * interact with IPA modules, and may be used internally in IPA modules if
> > > * desired to benefit from the data and helper classes provided by libcamera.
> >
> > That's what you repeat above here and I'm not sure anymore what we
> > meant at the time.
>
> Yeah, I can't remember either...
>
> I'll get rid of it.
>
Thanks
> > > *
> > > - * Functions defined in the ipa_context_ops structure are mapped to IPAInterface
> > > - * methods, while functions defined in the ipa_callback_ops are mapped to
> > > - * IPAInterface signals. As with the C API, the IPA C++ interface uses
> > > - * serializable data types only. It reuses structures defined by the C API, or
> > > - * defines corresponding classes using C++ containers when required.
> > > - *
> > > * Due to process isolation all arguments to the IPAInterface methods and
> > > * signals may need to be transferred over IPC. The class thus uses serializable
> > > * data types only. The IPA C++ interface defines custom data structures that
> > > * mirror core libcamera structures when the latter are not suitable, such as
> > > * IPAStream to carry StreamConfiguration data.
> >
> > Does this still apply ?
>
> I think it still does. Although in theory we could define de/serializers
> for the libcamera structures directly, instead of continuing to provide
> the mirrored structures. The mirrored structures are nice and simplified
> though.
Ok, I now see what was meant here. It's fine then!
>
> > > *
> > > - * As for the functions defined in struct ipa_context_ops, the methods defined
> > > - * by this class shall not return data from the IPA.
> > > + * Custom data structures may also be defined in the mojom file, in which case
> > > + * the de/serialization will automatically be generated. If any other libcamera
> > > + * structures are to be used as parameters, then a de/serializer for them must
> > > + * be implemented in IPADataSerializer.
> >
> > Isn't it the other way around ? serializer specializations are defined
> > for libcamera types, but if custom ones are used they need a
> > specialized IPADataSerializer implementation ?
>
> No, what I'm saying is that the specialized IPADataSerializer
> implementation for custom data structures will be auto-generated. On the
Ok, I missed that, as I've seen the IPADataSerializer<T> where T is a
libcamera type.
Do we have an example of serializers generated for custom types in
this series ?
> other hand, if there are any libcamera structures that someone wants to
> send to the IPA (that we don't already support), then we need to
> hand-define a specialized IPADataSerializer implementation (like I did
> before for everything in ipa_interface.h).
Ack!
With the few changes you've agreed to take in:
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
>
>
> Thanks,
>
> Paul
> > > *
> > > * The pipeline handler shall use the IPAManager to locate a compatible
> > > * IPAInterface. The interface may then be used to interact with the IPA module.
> > > */
> > >
> > > -/**
> > > - * \fn IPAInterface::init()
> > > - * \brief Initialise the IPAInterface
> > > - * \param[in] settings The IPA initialization settings
> > > - *
> > > - * This function initializes the IPA interface. It shall be called before any
> > > - * other function of the IPAInterface. The \a settings carry initialization
> > > - * parameters that are valid for the whole life time of the IPA interface.
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::start()
> > > - * \brief Start the IPA
> > > - *
> > > - * This method informs the IPA module that the camera is about to be started.
> > > - * The IPA module shall prepare any resources it needs to operate.
> > > - *
> > > - * \return 0 on success or a negative error code otherwise
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::stop()
> > > - * \brief Stop the IPA
> > > - *
> > > - * This method informs the IPA module that the camera is stopped. The IPA module
> > > - * shall release resources prepared in start().
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::configure()
> > > - * \brief Configure the IPA stream and sensor settings
> > > - * \param[in] sensorInfo Camera sensor information
> > > - * \param[in] streamConfig Configuration of all active streams
> > > - * \param[in] entityControls Controls provided by the pipeline entities
> > > - * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > - * \param[out] result Pipeline-handler-specific configuration result
> > > - *
> > > - * This method shall be called when the camera is started to inform the IPA of
> > > - * the camera's streams and the sensor settings. The meaning of the numerical
> > > - * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
> > > - * protocol.
> > > - *
> > > - * The \a sensorInfo conveys information about the camera sensor settings that
> > > - * the pipeline handler has selected for the configuration. The IPA may use
> > > - * that information to tune its algorithms.
> > > - *
> > > - * The \a ipaConfig and \a result parameters carry custom data passed by the
> > > - * pipeline handler to the IPA and back. The pipeline handler may set the \a
> > > - * result parameter to null if the IPA protocol doesn't need to pass a result
> > > - * back through the configure() function.
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::mapBuffers()
> > > - * \brief Map buffers shared between the pipeline handler and the IPA
> > > - * \param[in] buffers List of buffers to map
> > > - *
> > > - * This method informs the IPA module of memory buffers set up by the pipeline
> > > - * handler that the IPA needs to access. It provides dmabuf file handles for
> > > - * each buffer, and associates the buffers with unique numerical IDs.
> > > - *
> > > - * IPAs shall map the dmabuf file handles to their address space and keep a
> > > - * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used
> > > - * in all other IPA interface methods to refer to buffers, including the
> > > - * unmapBuffers() method.
> > > - *
> > > - * All buffers that the pipeline handler wishes to share with an IPA shall be
> > > - * mapped with this method. Buffers may be mapped all at once with a single
> > > - * call, or mapped and unmapped dynamically at runtime, depending on the IPA
> > > - * protocol. Regardless of the protocol, all buffers mapped at a given time
> > > - * shall have unique numerical IDs.
> > > - *
> > > - * The numerical IDs have no meaning defined by the IPA interface, and IPA
> > > - * protocols shall not give them any specific meaning either. They should be
> > > - * treated as opaque handles by IPAs, with the only exception that ID zero is
> > > - * invalid.
> > > - *
> > > - * \sa unmapBuffers()
> > > - *
> > > - * \todo Provide a generic implementation of mapBuffers and unmapBuffers for
> > > - * IPAs
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::unmapBuffers()
> > > - * \brief Unmap buffers shared by the pipeline to the IPA
> > > - * \param[in] ids List of buffer IDs to unmap
> > > - *
> > > - * This method removes mappings set up with mapBuffers(). Buffers may be
> > > - * unmapped all at once with a single call, or selectively at runtime, depending
> > > - * on the IPA protocol. Numerical IDs of unmapped buffers may be reused when
> > > - * mapping new buffers.
> > > - *
> > > - * \sa mapBuffers()
> > > - */
> > > -
> > > -/**
> > > - * \fn IPAInterface::processEvent()
> > > - * \brief Process an event from the pipeline handler
> > > - * \param[in] data IPA operation data
> > > - *
> > > - * This operation is used by pipeline handlers to inform the IPA module of
> > > - * events that occurred during the on-going capture operation.
> > > - *
> > > - * The event notified by the pipeline handler with this method is handled by the
> > > - * IPA, which interprets the operation parameters according to the separately
> > > - * documented IPA protocol.
> > > - */
> > > -
> > > -/**
> > > - * \var IPAInterface::queueFrameAction
> > > - * \brief Queue an action associated with a frame to the pipeline handler
> > > - * \param[in] frame The frame number for the action
> > > - * \param[in] data IPA operation data
> > > - *
> > > - * This signal is emitted when the IPA wishes to queue a FrameAction on the
> > > - * pipeline. The pipeline is still responsible for the scheduling of the action
> > > - * on its timeline.
> > > - *
> > > - * This signal is emitted by the IPA to queue an action to be executed by the
> > > - * pipeline handler on a frame. The type of action is identified by the
> > > - * \a data.operation field, as defined by the IPA protocol, and the rest of the
> > > - * \a data is interpreted accordingly. The pipeline handler shall queue the
> > > - * action and execute it as appropriate.
> > > - *
> > > - * The signal is only emitted when the IPA is running, that is after start() and
> > > - * before stop() have been called.
> > > - */
> > > -
> > > } /* namespace libcamera */
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list