[libcamera-devel] [PATCH] ipa: core.mojom: Rework core file documentation

[paul.elder at ideasonboard.com]

Hi Jacopo,

On Thu, Jul 15, 2021 at 05:32:00PM +0200, Jacopo Mondi wrote:
> The comment block at the beginning of the core.mojom file is meant to
> provide an overview of how to use libcamera defined types in the definition
> of mojom interfaces.
> 
> As the IPA/IPC interface definition mechanism evolved, the documentation
> has not been updated accordingly.

It's a good clean up!

> 
> Update the file comment to match the most recent of th IPA/IPC

s/comment/comments/

s/of th//

> interface definition and generation mechanism.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/ipa/core.mojom | 60 +++++++++++++++++++-------------
>  1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> index b32f30939454..d2017369b597 100644
> --- a/include/libcamera/ipa/core.mojom
> +++ b/include/libcamera/ipa/core.mojom
> @@ -15,37 +15,47 @@ module libcamera;
>   *
>   * Attributes:
>   * - skipHeader - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a C++ header definition
> - *     generated
> + *   - do not generate a C++ definition for the structure
> + *   - any type used in a mojom interface definition must have a corresponding
> + *     definition in a mojo file for the Mojo core to accept it

Should we already mention at this point about the exception if the type
is only used as an array/map member?

> + *   - this attribute allows to define a symbol for the Mojo core that

s/to define/defining/

> + *     corresponds to a library-defined type without duplicating its definition

Should we say "libcamera" instead of "library"?

> + *     in the generated C++ headers
>   * - skipSerdes - structs only, and only in core.mojom
> - *   - designate that this struct shall not have a (de)serializer generated
> - *   - all fields need a (de)serializer to be defined, either hand-written
> - *     in ipa_data_serializer.h
> + *   - do not generate a (de)serializer for the structure
> + *   - all types need a (de)serializer to be defined in order to be transported
> + *     over the IPA protocol. The (de)serializer can be:
> + *     - manually implemented in the core library as in example the
> + *       ControlSerializer class that handles libcamera control-related types

The manual implementation of the (de)serializer is the
IPADataSerializer, in ipa_data_serializer.cpp. ControlSerializer doesn't
do this; it's *used* by IPADataSerializer<ControlList> and
IPADataSerializer<ControlInfoMap>.

Which means that this point should be merged into the next one.

> + *     - provided as a template specialization as done in ipa_data_serializer.h
> + *       for POD types and C++ containers

ipa_data_serializer.h only contains the generic (de)serializers for
vectors and maps. The specialized ones go in ipa_data_serializer.cpp.

> + *     - generated at build time for types defined in a mojo interfaces
> + *       definition
> + *   - this attribute instructs the build system that a (de)serializer is
> + *     available for the type and there's no need to generate one

Yeah, this point makes it really clear. Although it has the same intent
as "do not generate a (de)serializer for the structure" above; maybe it
could be moved to a subpoint of that?

>   * - hasFd - struct fields or empty structs only
>   *   - designate that this field or empty struct contains a FileDescriptor
>   *
>   * Rules:
> - * - Any struct that is used in a struct definition in mojom must also be
> - *   defined in mojom
> - *   - If the struct has both a definition in a C++ header and a (de)serializer
> - *     in ipa_data_serializer.h, then the struct shall be declared as empty,
> - *     with both the [skipHeader] and [skipSerdes] attributes
> - *   - If the struct only has a definition in a C++ header, but no
> - *     (de)serializer, then the struct definition should have the [skipHeader]
> - *     attribute
> - * - Nested structures (e.g. FrameBuffer::Plane) cannot be defined in mojom.
> - *   - Avoid them, by defining them in a header in C++ and a (de)serializer in
> - *     ipa_data_serializer.h
> - * - If a struct is in an array/map inside a struct, then the struct that is
> - *   the member of the array/map does not need a mojom definition if it is
> - *   defined in a C++ header.
> - *   - This can be used to embed nested structures. The C++ double colon is
> - *     replaced with a dot (e.g. FrameBuffer::Plane -> FrameBuffer.Plane)
> - *   - The struct must still be defined in a header in C++ and a (de)serializer
> - *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> + * - If the type is defined in a library C++ header and a (de)serializer is

I think something to emphasize that both conditions must be met would be
nice. Like "*and*" or "is both defined"

> + *   available, either manually written in the library or in
> + *   ipa_data_serializer.h, then the struct shall be declared as empty,

Again, "manually written in the library" = IPADataSerializer
specialization defined in ipa_data_serializer.cpp.

> + *   with both the [skipHeader] and [skipSerdes] attributes associated

When both attributes are defined it's written as [skipHeader,
skipSerdes]. I'm not sure if it's better to write the attributes
individually here without the square brackets, or if you want to group
them together like how you'd write it in code, or keep it as-is.

> + * - If the type is defined in the library but no (de)serializer is available
> + *   then the type definition in the core.mojom file should have the
> + *   [skipHeader] attribute only

Would it be better to clarify/remind as a subpoint that in this case a
(de)serializer will be generated?

> + * - If a type definition has [skipHeader], then the header where the type is
> + *   defined must be included in ipa_interface.h
> + * - Nested types (e.g. FrameBuffer::Plane) cannot be directly defined in mojom
> + *   - Avoid them, by defining the nested type in a C++ header and provide a
> + *     (de)serializer
> + *   - The C++ namespace separator :: is replaced with a dot
> + *   - In example, to refer to FrameBuffer::Plane provide a definition of the
> + *     Plane type in a C++ header to be included, provide a deserializer and
> + *     reference it as FrameBuffer.Plane

It's a bit hard to read the points I think...
1 - provide a definition of FrameBuffer::Plane in a C++ header
2 - include this header in ipa_interface.h
3 - provide a (de)serializer (in ipa_data_serializer.cpp)
4 - in mojom, reference the type as FrameBuffer.Plane

Also, FrameBuffer.Plane (and any other nested types) is only usable as
array/map members as it cannot be defined in mojom.


Thanks,

Paul

> + * - Types that are contained in an array/map do not require a mojom definition
> + *   if one exists in the library.
>   * - [skipHeader] and [skipSerdes] only work here in core.mojom.
> - * - If a struct definition has [skipHeader], then the header where the
> - *   struct is defined must be #included in ipa_interface.h
>   * - If a field in a struct has a FileDescriptor, but is not explicitly
>   *   defined so in mojom, then the field must be marked with the [hasFd]
>   *   attribute.
> -- 
> 2.32.0
>