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

12099 diff mbox series

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@ideasonboard.com>
Tested-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@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(-)

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@ideasonboard.com>
> Tested-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@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?

--
Kieran



> +
>  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:
>

Hi Kieran,

On 4/28/21 2:00 AM, 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@ideasonboard.com>
>> Tested-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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?
Probably it's broken already as I don't think we thought out the design 
constraints while writing the IPA skeleton as well. For e.g. I am 
reading the internal headers used for writing the IPA IPU3 skeleton and 
it already uses "libcamera/internal/buffer.h" for MappedFrameBuffer.

There are others too, in our closed source IPA, like 
libcamera/internal/log.h & libcamera/internal/file.h, that we probably 
need to write/copy some kind of drop-in replacement.
>
> --
> Kieran
>
>
>
>> +
>>   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:
>>

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@ideasonboard.com>
> > Tested-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@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:
> >

Hi Paul,

On 29/04/2021 04:15, paul.elder@ideasonboard.com wrote:
> 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@ideasonboard.com>
>>> Tested-by: Umang Jain <umang.jain@ideasonboard.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@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.


I noticed because it also breaks my ABI-Compliance checker, which parses
all the public headers, and has no visibility of the private headers ...
so it broke... :D


> 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?

Yes, but ... not 'public' headers, as these are not libcamera public API
headers - but they are needed by an internal interface which is
available only to IPAs. (of course external IPA's mean a separate sort
of public interface that we need to now handle).

So we need to be able to break these out from include/internal/ and have
them available to the IPA without being available to applications.


> 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.

Indeed, we may have to consider specific licensing issues on those
interface headers, and/or ensure that we can facilitate the entire
reconstruction of those binary protocols from scratch ...


> 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:
>>>