[libcamera-devel] [PATCH v3 1/3] utils: ipc: Include instead of forward-declare CameraSensorInfo

[paul.elder at ideasonboard.com]

Hi Kieran,

On Tue, Apr 27, 2021 at 09:30:37PM +0100, Kieran Bingham wrote:
> Hi Paul,
> 
> On 23/04/2021 11:47, Paul Elder wrote:
> > For structs defined in core.mojom that have the skipHeader tag, if
> > they're only used in function parameters (in a mojom file) then a
> > forward-declaration is sufficient. However, if the struct is used in
> > another struct in a mojom file, then the forward-declaration is
> > insufficient, and the definition needs to be included. Do so for
> > CameraSensorInfo, which is the only forward-declared struct in
> > ipa_interface.h, and update the documentation comment.
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain at ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > ---
> > Changes in v3:
> > - update the documentation in core.mojom too
> > ---
> >  include/libcamera/ipa/core.mojom      | 3 +--
> >  include/libcamera/ipa/ipa_interface.h | 6 +++---
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/libcamera/ipa/core.mojom b/include/libcamera/ipa/core.mojom
> > index 5363f1c5..70de71ea 100644
> > --- a/include/libcamera/ipa/core.mojom
> > +++ b/include/libcamera/ipa/core.mojom
> > @@ -38,8 +38,7 @@
> >   *     implemented in ipa_data_serializer.h, as it cannot be defined in mojom
> >   * - [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 (or the struct forward-declared) in
> > - *   ipa_interface.h
> > + *   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.
> > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h
> > index 5d99e2cf..dfe1b40a 100644
> > --- a/include/libcamera/ipa/ipa_interface.h
> > +++ b/include/libcamera/ipa/ipa_interface.h
> > @@ -18,15 +18,15 @@
> >  #include <libcamera/geometry.h>
> >  #include <libcamera/signal.h>
> >  
> > +#include "libcamera/internal/camera_sensor.h"
> 
> I know this is merged already - but I have a question here ;-)
> 
> The libcamera/ipa/ipa_interface is installed as a 'public' header right?
> (So that external IPA's can be built against the interface).
> 
> However you are including an /internal/ file here which is not installed.
> 
> Doesn't this mean that this commit breaks external builds of IPAs?

Oh yeah... it does... maybe that's why it was forward-declared in the
first place.

But also, if somebody defines a struct in mojom what uses
CameraSensorInfo, the code generator needs to generate a struct that
embeds CameraSensorInfo, so the definition is kind of necessary (that's
why the compiler was complaining in the first place). Maybe the struct
definition should be moved here? Or it should be moved to a public
header?

Though, as Umang mentioned, there are a lot of other things that are
broken in external IPA builds. I think it'll get even worse after we
factor in the external IPA authors that are paranoid about importing any
headers at all from us.


Paul

> >  namespace libcamera {
> >  
> >  /*
> >   * Structs that are defined in core.mojom and have the skipHeader tag must be
> > - * forward-declared or #included here.
> > + * #included here.
> >   */
> >  
> > -struct CameraSensorInfo;
> > -
> >  class IPAInterface
> >  {
> >  public:
> >