[libcamera-devel] [PATCH v3 26/38] ipa: Add core.mojom
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Mon Oct 5 10:48:42 CEST 2020
Hi Laurent,
Thank you for the review.
On Sun, Oct 04, 2020 at 12:33:04PM +0300, Laurent Pinchart wrote:
> Hi Paul,
>
> Thank you for the patch.
>
> On Fri, Oct 02, 2020 at 11:31:42PM +0900, Paul Elder wrote:
> > Add a base mojom file to contain empty mojom definitions of libcamera
> > objects, as well as documentation for the IPA interfaces that need to be
> > defined in the mojom files.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v3:
> > - add doc that structs need to be defined
> > - add doc to recommend namespacing
> > - change indentation
> > - add requirement that base controls *must* be defined in
> > libcamera::{pipeline_name}::Controls
> >
> > New in v2
> > ---
> > include/libcamera/ipa/core.mojom | 83 ++++++++++++++++++++++++++
> > include/libcamera/ipa/meson.build | 18 +++++-
> > src/libcamera/proxy/meson.build | 2 +-
> > src/libcamera/proxy/worker/meson.build | 2 +-
> > 4 files changed, 100 insertions(+), 5 deletions(-)
> > create mode 100644 include/libcamera/ipa/core.mojom
> >
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > new file mode 100644
> > index 00000000..81d68bae
> > --- /dev/null
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +
> > +/*
> > + * Any libcamera objects that are used by any interfaces must be declared
> > + * here, and a de/serializer be implemented in IPADataSerializer. In addition,
> > + * the corresponding header file (or forward-declarations) must be placed in
> > + * {pipeline_name}.h.
> > + *
> > + * For libcamera types, the [hasFd] attribute is needed to notify the compiler
> > + * that the struct has an fd.
> > + */
> > +struct CameraSensorInfo {};
> > +struct ControlInfoMap {};
> > +struct ControlList {};
> > +struct FileDescriptor {};
>
> Doesn't FileDescriptor contain a file descriptor ?
Well yes but actually no. FileDescriptor *is* the file descriptor that
the code generator searches for in the struct encapsulation tree, so it
doesn't need to be tagged with [hasFd]. Just like how custom data
structures in the mojom file don't need [hasFd] either, because mojo can
see which stucts have FileDescriptor and which structs have those
structs.
> > +[hasFd] struct IPABuffer {};
This guy needs it because none of its mojo members have FileDescriptor,
but its C++ members do.
> > +struct IPASettings {};
> > +struct IPAStream {};
> > +
> > +/*
> > + * Any custom structs that the IPA interfaces use must be defined here,
> > + * usin the mojo interface definition language.
>
> s/usin/using/
ack
> > + *
> > + * It is recommended to use namespacing, to avoid potential collisions with
> > + * libcamera types. Use mojom's module directive for this.
> > + */
>
> I'm not entirely sure to follow you, there's no structure defined in
> this file in the whole series. Is this comment meant to explain that an
> IPA-specific .mojom file should define custom structures ? If so, I
> think this would be best documented in Documentation/guides/. It doesn't
> have to be handled in this series, but let's keep it in mind.
Ah, Documentation/guides/ that's where I'm supposed to put it.
I thought that, since I needed this file anyway for the common structs,
I might as well but docs here too.
> If the comment means something else, please enlighten me :-)
>
> > +
> > +/*
> > + * \class IPACoreInterface
> > + *
> > + * This mojo interface describes the main interface of the IPA. It must be
> > + * defined.
> > + *
> > + * The main interface must be named as IPA{pipeline_name}Interface.
> > + * IPACoreInterface serves as an example and documentation, where
> > + * {pipeline_name} is Core.
>
> OK, I think this confirms my understanding of the previous comment
> block. All this should probably be moved to Documentation/guides/ in the
> future. It's fine for now.
Oh I can leave it here for now, cool :)
> > + *
> > + * At minimum, the following three functions must be present (and implemented):
> > + * init(IPASettings settings) => (int32 ret);
> > + * start() => (int32 ret);
> > + * stop();
>
> It would be really nice if mojom supported inheritance for interfaces.
Yeah :/
> > + *
> > + * All input parameters will become const references, except for primitives,
>
> What's a primitive ? Is it a concept defined by mojo ?
I was referring to C++ primitives... Are they called PODs instead?
bool, [u]int{8,16,32,64}_t, float, double
> > + * which will simply become const. Output parameters will become pointers,
> > + * unless there is only one primitive output parameter, in which case it will
> > + * become a regular return value.
> > + *
> > + * const is not allowed inside of arrays and maps. mojo arrays will become C++
> > + * vectors.
>
> s/vectors/std::vector<>/
>
> Do these limitations/constraints come from mojo, or from our code
> generator ?
>From mojo.
> > + *
> > + * By default, all methods defined in the main interface are synchronous. This
> > + * means that in the case of IPC (ie. isolated IPA), the function call will not
> > + * return until the return value or output parameters are ready. To specify an
> > + * asynchronous function, the [async] attribute can be used. Asynchronous
> > + * methods must not have any return value or output parameters, since in the
> > + * case of IPC the call needs to return immediately.
> > + *
> > + * In addition the following must be defined in {pipeline_name}.h in the
> > + * libcamera namespace:
> > + *
> > + * static ControlInfoMap {pipeline_name}::Controls;
>
> Should we explain why, and what this is ?
Yeah, probably.
> I would s/Controls/controls/ as we start variable names with a
> lower-case letter.
Okay. raspberrypi.h has Controls right now though.
> > + *
> > + * It may be empty.
> > + */
> > +
> > +/*
> > + * \class IPACoreCallbackInterface
>
> I wonder if this should be called IPACoreEventInterface. Callbacks imply
> that the IPA can call back into the pipeline handler to drive it, which
> we don't want. Of course it's just a semantic difference, one can still
> implement an IPA that drives the pipeline handler with events, but
> making clear what is acceptable and what isn't would be useful.
Ooh yeah probably.
> > + *
> > + * This mojo interface describes the callback interface of the IPA. It must be
> > + * defined. If there are no callback functions, then it may be empty.
> > + *
> > + * The callback interface must be named as IPA{pipeline_name}CallbackInterface.
> > + * IPACoreCallbackInterface serves as an example and documentation, where
> > + * {pipeline_name} is Core.
> > + *
> > + * Methods defined in the callback interface shall not return anything. This
> > + * also means that these methods must be asynchronous and not synchronous.
> > + * Therefore the [async] tag is not necessary.
>
> I find this slightly confusing, it's not entirely clear why [async]
> isn't necessary. Would the following reflect a correct understanding ?
>
> * Methods defined in the callback interface are implictly asynchronous.
> * They thus can't return any value. Specifying the [async] tag is not
> * necessary.
Ah yeah, that's better.
> Do I also understand correctly that specifying the [async] tag will not
> cause any issue ?
Yeah, it won't. Callback methods are always async; if it has [async] tag
it'll just be ignored.
I don't have validation though, to enforce that callback methods return
void. I guess Signals can't return anything anyway? Maybe it's fine.
> > + *
> > + * Methods defined in the callback interface will become Signals in the IPA
> > + * interface. The IPA can emit signals, while the pipeline handler can connect
> > + * slots to them.
> > + */
> > diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> > index 337948d2..590a1464 100644
> > --- a/include/libcamera/ipa/meson.build
> > +++ b/include/libcamera/ipa/meson.build
> > @@ -13,6 +13,17 @@ install_headers(libcamera_ipa_headers,
> > # Prepare IPA/IPC generation components
> > #
> >
> > +core_mojom_file = 'core.mojom'
>
> It would be nice to handle the dependencies automatically. meson
> supports a depfile argument in custom_target(), which we could use to
> get mojom to output a list of dependencies parsed from the import
> statements. Something for later, as we would need to extend the mojo
> parser to support this feature.
Hm if we want to avoid modifying mojo, I think we can define another
generator (or extend the current one). It'll take the mojo module file
as an input, and extract and output the import statements.
Will IPA interfaces get so big that they'll span multiple files? In
order to enforce pipeline handler-driven pipeline I would expect that
they'd need to be short and simple.
> > +ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',
> > + input : core_mojom_file,
> > + output : core_mojom_file + '-module',
> > + command : [
> > + mojom_parser,
> > + '--output-root', meson.build_root(),
> > + '--input-root', meson.source_root(),
> > + '--mojoms', '@INPUT@'
> > + ])
>
> So the mojo parser requires compiling all .mojom files explicitly, it
> doesn't compile the imports automatically ? Not very nice :-(
Yeah :/
> > +
> > ipa_mojom_files = []
> >
> > ipa_mojoms = []
> > @@ -30,6 +41,7 @@ foreach file : ipa_mojom_files
> > mojom = custom_target(file.split('.')[0] + '_mojom_module',
> > input : file,
> > output : file + '-module',
> > + depends : ipa_mojom_core,
> > command : [
> > mojom_parser,
> > '--output-root', meson.build_root(),
> > @@ -41,7 +53,7 @@ foreach file : ipa_mojom_files
> > header = custom_target(name + '_generated_h',
> > input : mojom,
> > output : name + '_generated.h',
> > - depends : mojom_templates,
> > + depends : [mojom_templates, ipa_mojom_core],
> > command : [
> > mojom_generator, 'generate',
> > '-g', 'libcamera',
> > @@ -55,7 +67,7 @@ foreach file : ipa_mojom_files
> > serializer = custom_target(name + '_serializer_h',
> > input : mojom,
> > output : name + '_serializer.h',
> > - depends : mojom_templates,
> > + depends : [mojom_templates, ipa_mojom_core],
> > command : [
> > mojom_generator, 'generate',
> > '-g', 'libcamera',
> > @@ -69,7 +81,7 @@ foreach file : ipa_mojom_files
> > proxy_header = custom_target(name + '_proxy_h',
> > input : mojom,
> > output : 'ipa_proxy_' + name + '.h',
> > - depends : mojom_templates,
> > + depends : [mojom_templates, ipa_mojom_core],
> > command : [
> > mojom_generator, 'generate',
> > '-g', 'libcamera',
> > diff --git a/src/libcamera/proxy/meson.build b/src/libcamera/proxy/meson.build
> > index ac68ad08..f27b453a 100644
> > --- a/src/libcamera/proxy/meson.build
> > +++ b/src/libcamera/proxy/meson.build
> > @@ -10,7 +10,7 @@ foreach mojom : ipa_mojoms
> > proxy = custom_target(mojom['name'] + '_proxy_cpp',
> > input : mojom['mojom'],
> > output : 'ipa_proxy_' + mojom['name'] + '.cpp',
> > - depends : [mojom_templates],
> > + depends : [mojom_templates, ipa_mojom_core],
> > command : [
> > mojom_generator, 'generate',
> > '-g', 'libcamera',
> > diff --git a/src/libcamera/proxy/worker/meson.build b/src/libcamera/proxy/worker/meson.build
> > index 5490811b..353e5cf1 100644
> > --- a/src/libcamera/proxy/worker/meson.build
> > +++ b/src/libcamera/proxy/worker/meson.build
> > @@ -11,7 +11,7 @@ foreach mojom : ipa_mojoms
> > worker = custom_target(mojom['name'] + '_proxy_worker',
> > input : mojom['mojom'],
> > output : 'ipa_proxy_' + mojom['name'] + '_worker.cpp',
> > - depends : [mojom_templates],
> > + depends : [mojom_templates, ipa_mojom_core],
> > command : [
> > mojom_generator, 'generate',
> > '-g', 'libcamera',
Thanks,
Paul
More information about the libcamera-devel
mailing list