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

[Laurent Pinchart]

On Fri, Mar 12, 2021 at 04:50:33PM +0900, paul.elder at ideasonboard.com wrote:
> On Fri, Mar 12, 2021 at 06:33:59AM +0000, Kieran Bingham wrote:
> > On 10/03/2021 09:54, paul.elder at 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 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:

-- 
Regards,

Laurent Pinchart