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

[Kieran Bingham]

Hi Paul,

On 29/04/2021 04:15, paul.elder at 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 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.


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

-- 
Regards
--
Kieran