[libcamera-devel] [PATCH v5 9/9] [RFC] test: generated_serializer: Test skipHeader enums and flags

[paul.elder at ideasonboard.com]

Hi Laurent,

On Tue, Oct 11, 2022 at 05:01:53PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Tue, Oct 11, 2022 at 07:58:59PM +0900, Paul Elder via libcamera-devel wrote:
> > Add fields to the test struct to test serialization/deserialization of
> > scoped enums and flags of said scoped enums that are defined in a C++
> > header, which are thus designated by [skipHeader] in mojom.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > 
> > ---
> > New in v5
> > 
> > RFC because this touches core stuff. This can be detached from the rest
> > of the series.
> > ---
> >  include/libcamera/ipa/core.mojom                      |  2 ++
> >  include/libcamera/ipa/ipa_interface.h                 |  7 +++++++
> >  .../generated_serializer_test.cpp                     | 11 +++++++++++
> >  .../include/libcamera/ipa/test.mojom                  |  4 ++++
> >  4 files changed, 24 insertions(+)
> > 
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > index 1ff674b0..3438af93 100644
> > --- a/include/libcamera/ipa/core.mojom
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -84,6 +84,8 @@ module libcamera;
> >  [skipSerdes, skipHeader] struct ControlList {};
> >  [skipSerdes, skipHeader] struct SharedFD {};
> >  
> > +[skipHeader, scopedEnum] enum TestEnum {};
> > +
> 
> This particular part isn't very nice. Same for the change to
> ipa_interface.h below.

Yeah that's what I thought :/

> We should either find a suitable enum that IPA modules (will) really need,

Any ideas? :)

> or try to test this feature using test.mojom and/or vimc.mojom.

That won't work (that's why this part of the series was delayed). The
enums that are skipHeader-ed need to be accessbile from
{pipeline}_ipa_interface.h, but those are generated from
{pipeline}.mojom. Which means that either 1) the headers that defined
those enums need to be #included in the global ipa_interface.h (so *all*
IPAs have access to them) or 2) we add a new header that will
automatically be imcluded in the generated {pipeline}_ipa_interface.h,
which imo defeats the whole purpose of {pipeline}.mojom.

So we need a suitable enum that IPA modules will really need.


Paul

> 
> >  [skipHeader] struct Point {
> >  	int32 x;
> >  	int32 y;
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index 8884f0ed..96fb8344 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -27,6 +27,13 @@ namespace libcamera {
> >   * tag must be #included here.
> >   */
> >  
> > +enum class TestEnum {
> > +	TestEnumValueA,
> > +	TestEnumValueB,
> > +	TestEnumValueC,
> > +	TestEnumValueD,
> > +};
> > +
> >  class IPAInterface
> >  {
> >  public:
> > diff --git a/test/serialization/generated_serializer/generated_serializer_test.cpp b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > index 4670fe46..6f01c3d4 100644
> > --- a/test/serialization/generated_serializer/generated_serializer_test.cpp
> > +++ b/test/serialization/generated_serializer/generated_serializer_test.cpp
> > @@ -11,6 +11,8 @@
> >  
> >  #include "test.h"
> >  
> > +#include <libcamera/ipa/ipa_interface.h>
> > +
> >  #include "test_ipa_interface.h"
> >  #include "test_ipa_serializer.h"
> >  
> > @@ -59,6 +61,9 @@ if (struct1.field != struct2.field) {				\
> >  		t.s3 = "lorem ipsum";
> >  		t.i  = 58527;
> >  		t.c = ipa::test::IPAOperationInit;
> > +		t.ex = TestEnum::TestEnumValueA;
> > +		t.exf |= TestEnum::TestEnumValueB;
> > +
> >  		t.e = ipa::test::ErrorFlags::Error1;
> >  
> >  		Flags<ipa::test::ErrorFlags> flags;
> > @@ -87,6 +92,8 @@ if (struct1.field != struct2.field) {				\
> >  
> >  		TEST_SCOPED_ENUM_EQUALITY(t, u, e);
> >  		TEST_SCOPED_ENUM_EQUALITY(t, u, f);
> > +		TEST_SCOPED_ENUM_EQUALITY(t, u, ex);
> > +		TEST_SCOPED_ENUM_EQUALITY(t, u, exf);
> >  
> >  		/* Test vector of generated structs */
> >  		std::vector<ipa::test::TestStruct> v = { t, u };
> > @@ -113,6 +120,8 @@ if (struct1.field != struct2.field) {				\
> >  
> >  		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], e);
> >  		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], f);
> > +		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], ex);
> > +		TEST_SCOPED_ENUM_EQUALITY(v[0], w[0], exf);
> >  
> >  		TEST_FIELD_EQUALITY(v[1], w[1], s1);
> >  		TEST_FIELD_EQUALITY(v[1], w[1], s2);
> > @@ -122,6 +131,8 @@ if (struct1.field != struct2.field) {				\
> >  
> >  		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], e);
> >  		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], f);
> > +		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], ex);
> > +		TEST_SCOPED_ENUM_EQUALITY(v[1], w[1], exf);
> >  
> >  		return TestPass;
> >  	}
> > diff --git a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > index 91c31642..1f1ba8dc 100644
> > --- a/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > +++ b/test/serialization/generated_serializer/include/libcamera/ipa/test.mojom
> > @@ -2,6 +2,8 @@
> >  
> >  module ipa.test;
> >  
> > +import "include/libcamera/ipa/core.mojom";
> > +
> >  enum IPAOperationCode {
> >  	IPAOperationNone,
> >  	IPAOperationInit,
> > @@ -28,6 +30,8 @@ struct TestStruct {
> >  	IPAOperationCode c;
> >  	ErrorFlags e;
> >  	[flags] ErrorFlags f;
> > +	libcamera.TestEnum ex;
> > +	[flags] libcamera.TestEnum exf;
> >  };
> >  
> >  interface IPATestInterface {