| Message ID | 20210310092951.665759-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On 2021-03-10 09:29:51 +0000, Kieran Bingham wrote: > The struct CameraSensorInfo forward declaration is not used by > the ipa_interface. > > Remove it. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/ipa/ipa_interface.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > index 47f81d1d2e8f..8554613e0217 100644 > --- a/include/libcamera/ipa/ipa_interface.h > +++ b/include/libcamera/ipa/ipa_interface.h > @@ -20,8 +20,6 @@ > > namespace libcamera { > > -struct CameraSensorInfo; > - > class IPAInterface > { > public: > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On Wed, Mar 10, 2021 at 09:29:51AM +0000, Kieran Bingham wrote: > The struct CameraSensorInfo forward declaration is not used by > the ipa_interface. > CameraSensorInfo is in the parameters of configure() for the raspberrypi and rkisp1 IPA interfaces. rkisp1 doesn't seem to actually use it, but raspberrypi does. Now, CameraSensorInfo is defined in core.mojom, but it has the skipHeader tag because CameraSensorInfo is also defined in camera_sensor.h, and it has lots of member functions. This means that core_ipa_interface.h will not have CameraSensorInfo in it. Since we don't have a way to tell the generated files (proxy cpp+h, proxy worker cpp, interface header, serializer) what specific files need to be included, they all include ipa_interface.h, and any custom data structures need to be defined in the respective mojom file. This means that any struct defined in core.mojom that has skipHeader needs to be defined in ipa_interface.h, or it has to be included by ipa_interface.h. So if you are to remove struct CameraSensorInfo here, then you need to #include "libcamera/internal/camera_sensor.h" Paul > Remove it. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/ipa/ipa_interface.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > index 47f81d1d2e8f..8554613e0217 100644 > --- a/include/libcamera/ipa/ipa_interface.h > +++ b/include/libcamera/ipa/ipa_interface.h > @@ -20,8 +20,6 @@ > > namespace libcamera { > > -struct CameraSensorInfo; > - > class IPAInterface > { > public: > -- > 2.25.1
Hi Paul, On 10/03/2021 09:54, paul.elder@ideasonboard.com wrote: > Hi Kieran, > > On Wed, Mar 10, 2021 at 09:29:51AM +0000, Kieran Bingham wrote: >> The struct CameraSensorInfo forward declaration is not used by >> the ipa_interface. >> > > CameraSensorInfo is in the parameters of configure() for the raspberrypi > and rkisp1 IPA interfaces. rkisp1 doesn't seem to actually use it, but > raspberrypi does. > Aha, I must have been compiling IPU3 only when I looked at this. > Now, CameraSensorInfo is defined in core.mojom, but it has the > skipHeader tag because CameraSensorInfo is also defined in > camera_sensor.h, and it has lots of member functions. This means that > core_ipa_interface.h will not have CameraSensorInfo in it. > > Since we don't have a way to tell the generated files (proxy cpp+h, > proxy worker cpp, interface header, serializer) what specific files need > to be included, they all include ipa_interface.h, and any custom data > structures need to be defined in the respective mojom file. > > This means that any struct defined in core.mojom that has skipHeader > needs to be defined in ipa_interface.h, or it has to be included by > ipa_interface.h. Hrm ... Ok - so it's a bit opaque for the reasoning in the file itself. Is there anyway that we could extend the pipeline specific 'include' statements in the mojom files? > So if you are to remove struct CameraSensorInfo here, then you need to > #include "libcamera/internal/camera_sensor.h" Perhaps if we end up leaving this here we should comment the includes (or forward declarations) that they are needed because they are used by specific pipeline handlers. > > > Paul > >> Remove it. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> include/libcamera/ipa/ipa_interface.h | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h >> index 47f81d1d2e8f..8554613e0217 100644 >> --- a/include/libcamera/ipa/ipa_interface.h >> +++ b/include/libcamera/ipa/ipa_interface.h >> @@ -20,8 +20,6 @@ >> >> namespace libcamera { >> >> -struct CameraSensorInfo; >> - >> class IPAInterface >> { >> public: >> -- >> 2.25.1
Hi Kieran, On Fri, Mar 12, 2021 at 06:33:59AM +0000, Kieran Bingham wrote: > Hi Paul, > > On 10/03/2021 09:54, paul.elder@ideasonboard.com wrote: > > Hi Kieran, > > > > On Wed, Mar 10, 2021 at 09:29:51AM +0000, Kieran Bingham wrote: > >> The struct CameraSensorInfo forward declaration is not used by > >> the ipa_interface. > >> > > > > CameraSensorInfo is in the parameters of configure() for the raspberrypi > > and rkisp1 IPA interfaces. rkisp1 doesn't seem to actually use it, but > > raspberrypi does. > > > > Aha, I must have been compiling IPU3 only when I looked at this. > > > > Now, CameraSensorInfo is defined in core.mojom, but it has the > > skipHeader tag because CameraSensorInfo is also defined in > > camera_sensor.h, and it has lots of member functions. This means that > > core_ipa_interface.h will not have CameraSensorInfo in it. > > > > Since we don't have a way to tell the generated files (proxy cpp+h, > > proxy worker cpp, interface header, serializer) what specific files need > > to be included, they all include ipa_interface.h, and any custom data > > structures need to be defined in the respective mojom file. > > > > This means that any struct defined in core.mojom that has skipHeader > > needs to be defined in ipa_interface.h, or it has to be included by > > ipa_interface.h. > > Hrm ... Ok - so it's a bit opaque for the reasoning in the file itself. > Is there anyway that we could extend the pipeline specific 'include' > statements in the mojom files? mojo has an import statement, but it looks like the parser actually uses it, so no, there's no way in mojo that we can do that. > > > So if you are to remove struct CameraSensorInfo here, then you need to > > #include "libcamera/internal/camera_sensor.h" > > Perhaps if we end up leaving this here we should comment the includes > (or forward declarations) that they are needed because they are used by > specific pipeline handlers. I was thinking ipa_interface.h as the header file that contains all the libcamera-specific structs that we alow IPA interfaces to use. iirc that was the original intention too. Paul > > > >> Remove it. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> include/libcamera/ipa/ipa_interface.h | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > >> index 47f81d1d2e8f..8554613e0217 100644 > >> --- a/include/libcamera/ipa/ipa_interface.h > >> +++ b/include/libcamera/ipa/ipa_interface.h > >> @@ -20,8 +20,6 @@ > >> > >> namespace libcamera { > >> > >> -struct CameraSensorInfo; > >> - > >> class IPAInterface > >> { > >> public: > >> -- > >> 2.25.1
On Fri, Mar 12, 2021 at 04:50:33PM +0900, paul.elder@ideasonboard.com wrote: > On Fri, Mar 12, 2021 at 06:33:59AM +0000, Kieran Bingham wrote: > > On 10/03/2021 09:54, paul.elder@ideasonboard.com wrote: > > > On Wed, Mar 10, 2021 at 09:29:51AM +0000, Kieran Bingham wrote: > > >> The struct CameraSensorInfo forward declaration is not used by > > >> the ipa_interface. > > >> > > > > > > CameraSensorInfo is in the parameters of configure() for the raspberrypi > > > and rkisp1 IPA interfaces. rkisp1 doesn't seem to actually use it, but > > > raspberrypi does. > > > > Aha, I must have been compiling IPU3 only when I looked at this. > > > > > Now, CameraSensorInfo is defined in core.mojom, but it has the > > > skipHeader tag because CameraSensorInfo is also defined in > > > camera_sensor.h, and it has lots of member functions. This means that > > > core_ipa_interface.h will not have CameraSensorInfo in it. > > > > > > Since we don't have a way to tell the generated files (proxy cpp+h, > > > proxy worker cpp, interface header, serializer) what specific files need > > > to be included, they all include ipa_interface.h, and any custom data > > > structures need to be defined in the respective mojom file. > > > > > > This means that any struct defined in core.mojom that has skipHeader > > > needs to be defined in ipa_interface.h, or it has to be included by > > > ipa_interface.h. > > > > Hrm ... Ok - so it's a bit opaque for the reasoning in the file itself. > > Is there anyway that we could extend the pipeline specific 'include' > > statements in the mojom files? > > mojo has an import statement, but it looks like the parser actually > uses it, so no, there's no way in mojo that we can do that. > > > > So if you are to remove struct CameraSensorInfo here, then you need to > > > #include "libcamera/internal/camera_sensor.h" > > > > Perhaps if we end up leaving this here we should comment the includes > > (or forward declarations) that they are needed because they are used by > > specific pipeline handlers. > > I was thinking ipa_interface.h as the header file that contains all the > libcamera-specific structs that we alow IPA interfaces to use. iirc > that was the original intention too. Maybe adding a comment in the file to explain why the forward declarations shouldn't be dropped would avoid the same patch being submitted in 6 months by someone else (or even again by Kieran ;-)) ? > > >> Remove it. > > >> > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> --- > > >> include/libcamera/ipa/ipa_interface.h | 2 -- > > >> 1 file changed, 2 deletions(-) > > >> > > >> diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h > > >> index 47f81d1d2e8f..8554613e0217 100644 > > >> --- a/include/libcamera/ipa/ipa_interface.h > > >> +++ b/include/libcamera/ipa/ipa_interface.h > > >> @@ -20,8 +20,6 @@ > > >> > > >> namespace libcamera { > > >> > > >> -struct CameraSensorInfo; > > >> - > > >> class IPAInterface > > >> { > > >> public:
diff --git a/include/libcamera/ipa/ipa_interface.h b/include/libcamera/ipa/ipa_interface.h index 47f81d1d2e8f..8554613e0217 100644 --- a/include/libcamera/ipa/ipa_interface.h +++ b/include/libcamera/ipa/ipa_interface.h @@ -20,8 +20,6 @@ namespace libcamera { -struct CameraSensorInfo; - class IPAInterface { public:
The struct CameraSensorInfo forward declaration is not used by the ipa_interface. Remove it. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/ipa/ipa_interface.h | 2 -- 1 file changed, 2 deletions(-)