[libcamera-devel] [PATCH] include: ipa: Remove unused declaration

[paul.elder at ideasonboard.com]

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