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

[Laurent Pinchart]

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 at 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 at ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart