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

[Umang Jain]

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