[libcamera-devel] utils: ipc: mojo: Error if ControlInfoMap/List doesn't prefix libcamera

12435 diff mbox series

The mojo parser is fine if there are types that are used in array/map
members that it does not know about. These are usually caught by the C++
compiler, because the generated code refers to unknown types. This
feature is necessary for us for supporting FrameBuffer::Plane as an
array/map member, since as long as the type has an IPADataSerializer and
the struct defined in C++, the generated code will run fine
(FrameBuffer::Plane is not defined anywhere in mojom but is used as an
array member in IPABuffer).

The types that are defined in controls.h (or any header included in
ipa_interface.h) will all be compiled by the C++ compiler fine, since
the generated files all include controls.h. The types that are there
that are not ControlInfoMap or ControlList (like ControlValue) will
still fail at the linker stage. For example:

struct A {
	array<ControlValue> a;
};

will compile fine, but will fail to link, since
IPADataSerializer<ControlValue> doesn't exist. This behavior, although
not the best, is acceptable.

The issue is that if ControlInfoMap or ControlList are used as array/map
members without the libcamera prefix, the compiler will not complain, as
the types are valid, and the linker will also not complain, as
IPADataSerializer<ControlList> and IPADataSerializer<ControlInfoMap>
both exist. However, the code generator will not recognize them as
types that require a ControlSerializer (since mojo doesn't recognize
them, so they are different from the ones that it does recognize with
the libcamera namespace), and so the ControlSerializer will not be
passed to the serializer in the generated code. This is the cause of the
FATAL breakage:

 FATAL IPADataSerializer ipa_data_serializer.cpp:437 ControlSerializer
 not provided for serialization of ControlInfoMap

Since ControlInfoMap and ControlList are the only types that will run
into this issue, we solve this by simply detecting if they are used
without the prefix, and produce an error at that point in the code
generator. As the code generator stage no longer has information on the
source code file and line, we output the struct name in which the error
was found (ninja will output the file name).

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 utils/ipc/generators/mojom_libcamera_generator.py | 4 ++++
 1 file changed, 4 insertions(+)

Hi Paul,

Thank you for the patch.

On Thu, May 27, 2021 at 02:32:30PM +0900, Paul Elder wrote:
> The mojo parser is fine if there are types that are used in array/map
> members that it does not know about. These are usually caught by the C++
> compiler, because the generated code refers to unknown types. This
> feature is necessary for us for supporting FrameBuffer::Plane as an
> array/map member, since as long as the type has an IPADataSerializer and
> the struct defined in C++, the generated code will run fine
> (FrameBuffer::Plane is not defined anywhere in mojom but is used as an
> array member in IPABuffer).
> 
> The types that are defined in controls.h (or any header included in
> ipa_interface.h) will all be compiled by the C++ compiler fine, since
> the generated files all include controls.h. The types that are there
> that are not ControlInfoMap or ControlList (like ControlValue) will
> still fail at the linker stage. For example:
> 
> struct A {
> 	array<ControlValue> a;
> };
> 
> will compile fine, but will fail to link, since
> IPADataSerializer<ControlValue> doesn't exist. This behavior, although
> not the best, is acceptable.
> 
> The issue is that if ControlInfoMap or ControlList are used as array/map
> members without the libcamera prefix, the compiler will not complain, as
> the types are valid, and the linker will also not complain, as
> IPADataSerializer<ControlList> and IPADataSerializer<ControlInfoMap>
> both exist. However, the code generator will not recognize them as
> types that require a ControlSerializer (since mojo doesn't recognize
> them, so they are different from the ones that it does recognize with
> the libcamera namespace), and so the ControlSerializer will not be
> passed to the serializer in the generated code. This is the cause of the
> FATAL breakage:
> 
>  FATAL IPADataSerializer ipa_data_serializer.cpp:437 ControlSerializer
>  not provided for serialization of ControlInfoMap
> 
> Since ControlInfoMap and ControlList are the only types that will run
> into this issue, we solve this by simply detecting if they are used
> without the prefix, and produce an error at that point in the code
> generator. As the code generator stage no longer has information on the
> source code file and line, we output the struct name in which the error
> was found (ninja will output the file name).

Very clear explanation, thank you.

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  utils/ipc/generators/mojom_libcamera_generator.py | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/utils/ipc/generators/mojom_libcamera_generator.py b/utils/ipc/generators/mojom_libcamera_generator.py
> index effdfed6..25cacf9a 100644
> --- a/utils/ipc/generators/mojom_libcamera_generator.py
> +++ b/utils/ipc/generators/mojom_libcamera_generator.py
> @@ -129,6 +129,10 @@ def GetAllAttrs(element):
>  
>  def NeedsControlSerializer(element):
>      types = GetAllTypes(element)
> +    if 'x:ControlList' in types:
> +        raise Exception(f'Unknown type "ControlList" in {element.mojom_name}, did you mean "libcamera.ControlList"?')
> +    if 'x:ControlInfoMap' in types:
> +        raise Exception(f'Unknown type "ControlInfoMap" in {element.mojom_name}, did you mean "libcamera.ControlInfoMap"?')

Following the 0, 1, N rule,

       for type in ['ControlList', 'ControlInfoMap']:
           if f'x:{type}' in types:
               raise Exception(f'Unknown type "{type}" in {element.mojom_name}, did you mean "libcamera.{type}"?')

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>      return "ControlList" in types or "ControlInfoMap" in types
>  
>  def HasFd(element):