[libcamera-devel] [PATCH v4 20/37] libcamera: IPAInterface: remove ipa_context and functions from documentation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Nov 25 20:28:48 CET 2020
Hi Paul,
Thank you for the patch.
On Thu, Nov 19, 2020 at 12:01:55PM +0900, paul.elder at ideasonboard.com wrote:
> On Wed, Nov 18, 2020 at 11:41:50AM +0100, Jacopo Mondi wrote:
> > On Wed, Nov 18, 2020 at 07:03:45PM +0900, paul.elder at ideasonboard.com wrote:
> > > On Tue, Nov 17, 2020 at 05:49:01PM +0100, Jacopo Mondi wrote:
> > > > 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
s/interface operations/operations/
> > > > > + * 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
>
> Nah, I'll drop 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.
This needs to be reworked, as the IPAInterface class doesn't define base
functions anymore. How about the following ?
* The IPA interface is specific to each pipeline handler. The pipeline handlers
* define a set of operations used to communicate with their IPA modules. The
* operations, along with the data structures they use, are collectively
* referred to as the IPA protocol.
*
* The IPA protocol is defined using the
* <a href="https://chromium.googlesource.com/chromium/src/+/master/mojo/public/tools/bindings/README.md">Mojo interface definition language</a>,
* in a Mojo module file stored in include/libcamera/ipa/{pipeline_name}.mojom.
* The Mojo module contains two Mojo interfaces: IPAInterface defines the
* operations exposed by the IPA and called by the pipeline handler, and
* IPAEventInterface defines the events generated by the IPA and received by the
* pipeline handler.
I've used IPAInterface and IPAEventInterface here as it occurred to me
that the corresponding C++ classes are stored in IPA-specific
namespaces, so we don't need to call them IPA{pipeline_name}Interface
and IPA{pipeline_name}EventInterface. If you think we should retain the
pipeline name, feel free to update the text.
Going forward, thinking about closed-source IPAs, and the fact they may
not be able to use the Signal class, we may need to create two separate
instances of IPAInterface, one for each direction, but that's a topic
for later.
> > > > 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 ?
>
> Yeah.
>
> > > > > *
> > > > > * \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
s/de\/serialization/(de)serialization/ ?
> > > > > + * any C++ struct that is defined in the mojom file, or the C++ libcamera
s/struct/structure/
> > > > > + * objects that are mentioned in core.mojom, can be used directly.
s/mentioned/listed/ ?
> > > >
> > > > 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 wonder if this becomes redundant with the proposed update above.
> > > > 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.
>
> Ah, I see your concern now.
>
> Not anymore! This whole series means that the IPA must be implemented in
> C++, and they receive C++ structs. If I remember correctly, including
> headers is allowed, and so the IPA implementations only have to include
> the generated header (eg. raspberrypi_ipa_interface.h) and they'll have
> the specialized IPAInterface definition that they need to implement, and
> the definition of all the custom structs and enums that the specialized
> IPAInterface uses.
>
> So the issue I'm seeing now is if this generated header includes
> libcamera headers... including ControlList.
>
> Hm, I think I see another issue here. If an IPA is compiled with an old
> version of an generated IPA header and tries to use it with a new
> libcamera... oh wait, that's solved by the versioning that we built into
> IPA modules a long time ago :)
>
> > 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.
>
> How it works is that the Proxy, on construction, constructs an IPAIPC,
> passing the path to the proxy worker and the IPA module:
>
> ipc_ = std::make_unique<IPAIPCUnixSocket>(ipam->path().c_str(), proxyWorkerPath.c_str());
>
> Then the IPAIPC creates a new process, running the Proxy worker, and
> passing the path to the IPA module and the socket fd:
>
> args.push_back(ipaModulePath);
> ...
> args.push_back(std::to_string(fd));
> fds.push_back(fd);
> ...
> int ret = proc_->start(ipaProxyWorkerPath, args, fds);
>
> Then the worker constructs an IPAModule instance from the IPA module
> path, and creates an IPAInterface from it (just like in the unisolated
> case):
>
> std::unique_ptr<IPAModule> ipam = std::make_unique<IPAModule>(argv[1]);
> ...
> ipa_ = dynamic_cast<IPARPiInterface *>(ipam->createInterface());
>
> > 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);
>
> So the proxy worker deals with receiving the RPC, and does exactly this.
> The IPA no longer has to deal with deserialization, it just receives C++
> objects as direct function call parameters.
>
> Which brings another concern, about how Signals will work for the other
> direction... Signal isn't header-only...
>
> Ngh, between Signal and ControlList, could we provide a sub-library that
> closed IPAs can link to? Or is that being too nice? :p
I think we'll have to solve this with a combination of a small
BSD-licensed library, and pushing some of the more complex work
(possibly including control list (de)serialization) to the closed-source
IPA. I'm fine addressing this issue later once this series gets merged.
> > 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 ?)
>
> I wouldn't expect them to write mojom files and run the generator, but I
> would expect them to use the generated header.
>
> > > > > */
> > > > >
> > > > > /**
> > > > > @@ -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
Unless I'm mistaken, core.mojom doesn't document this.
> > > > > + * 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
Same here, (de)serialization ?
> > > > > + * 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 ?
>
> In the RFC oh so long ago, yes :p (although that was hand-generated)
> But not in this series.
>
> If you run ninja you can see the generated serializer code in
> $BUILD/include/libcamera/ipa/{pipeline_name}_ipa_serializer.h
>
> > > 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!
>
> It shan't be that bad. If it's complicated then we can reject it or
> simplify it, and if it's simple, then it's not that bad :)
>
> > With the few changes you've agreed to take in:
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > > > *
> > > > > * 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.
> > > > > - */
The documentation is moved to raspberrypi.mojom, so it won't end up in
the generated documentation :-S It would be nice if we could consolidate
this a little bit, but I'm not sure how. If there's no straightforward
solution could you add a \todo comment ? I'll see if I can find a good
solution on top.
> > > > > -
> > > > > -/**
> > > > > - * \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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list