| Message ID | 20191108205409.18845-22-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Fri, Nov 08, 2019 at 10:54:06PM +0200, Laurent Pinchart wrote: > IPA modules have to implement a public ipaCreate() function, but its > prototype isn't declared in any header file. This allows for modules to > get the prototype wrong without being warned by the compiler. Fix it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/ipa/ipa_interface.h | 2 ++ > src/libcamera/ipa_interface.cpp | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > index 3a423e37671f..92f1aac50b85 100644 > --- a/include/ipa/ipa_interface.h > +++ b/include/ipa/ipa_interface.h > @@ -80,6 +80,8 @@ struct ipa_context_ops { > const struct ipa_operation_data *data); > }; > > +struct ipa_context *ipaCreate(); > + > #ifdef __cplusplus > } > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > index 89fce0e8347f..cb2767a04711 100644 > --- a/src/libcamera/ipa_interface.cpp > +++ b/src/libcamera/ipa_interface.cpp > @@ -261,6 +261,16 @@ > * \sa libcamera::IPAInterface::processEvent() > */ > > +/** > + * \fn ipaCreate() > + * \brief Entry point to the IPA modules > + * > + * This function is the entry point to the IPA modules. It is implemented by > + * every IPA module, and called by libcamera to create a new IPA context. > + * > + * \return A newly created IPA context > + */ > + Yout might want to remove the prototype defined in the \file comment block in ipa_interface.cpp and let Doxygen link ipaCreate() mentions with this Apart from that Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > namespace libcamera { > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Fri, Nov 15, 2019 at 06:05:21PM +0100, Jacopo Mondi wrote: > On Fri, Nov 08, 2019 at 10:54:06PM +0200, Laurent Pinchart wrote: > > IPA modules have to implement a public ipaCreate() function, but its > > prototype isn't declared in any header file. This allows for modules to > > get the prototype wrong without being warned by the compiler. Fix it. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/ipa/ipa_interface.h | 2 ++ > > src/libcamera/ipa_interface.cpp | 10 ++++++++++ > > 2 files changed, 12 insertions(+) > > > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > > index 3a423e37671f..92f1aac50b85 100644 > > --- a/include/ipa/ipa_interface.h > > +++ b/include/ipa/ipa_interface.h > > @@ -80,6 +80,8 @@ struct ipa_context_ops { > > const struct ipa_operation_data *data); > > }; > > > > +struct ipa_context *ipaCreate(); > > + > > #ifdef __cplusplus > > } > > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > > index 89fce0e8347f..cb2767a04711 100644 > > --- a/src/libcamera/ipa_interface.cpp > > +++ b/src/libcamera/ipa_interface.cpp > > @@ -261,6 +261,16 @@ > > * \sa libcamera::IPAInterface::processEvent() > > */ > > > > +/** > > + * \fn ipaCreate() > > + * \brief Entry point to the IPA modules > > + * > > + * This function is the entry point to the IPA modules. It is implemented by > > + * every IPA module, and called by libcamera to create a new IPA context. > > + * > > + * \return A newly created IPA context > > + */ > > + > > Yout might want to remove the prototype defined in the \file comment > block in ipa_interface.cpp and let Doxygen link ipaCreate() mentions > with this I like having it there, as it gives a nice overview without a need to jump to the ipaCreate() documentation, but if you think it should be removed, I can replace the two paragraphs with * IPA modules shall expose a public function named ipaCreate(). The function * creates an instance of an IPA context, which models a context of execution * for the IPA. IPA modules shall support creating one context per camera, as * required by their associated pipeline handler. Please let me know what you think is best. > Apart from that > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > namespace libcamera { > > > > /**
Hi Laurent, On Mon, Nov 18, 2019 at 03:09:19AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > On Fri, Nov 15, 2019 at 06:05:21PM +0100, Jacopo Mondi wrote: > > On Fri, Nov 08, 2019 at 10:54:06PM +0200, Laurent Pinchart wrote: > > > IPA modules have to implement a public ipaCreate() function, but its > > > prototype isn't declared in any header file. This allows for modules to > > > get the prototype wrong without being warned by the compiler. Fix it. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/ipa/ipa_interface.h | 2 ++ > > > src/libcamera/ipa_interface.cpp | 10 ++++++++++ > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > > > index 3a423e37671f..92f1aac50b85 100644 > > > --- a/include/ipa/ipa_interface.h > > > +++ b/include/ipa/ipa_interface.h > > > @@ -80,6 +80,8 @@ struct ipa_context_ops { > > > const struct ipa_operation_data *data); > > > }; > > > > > > +struct ipa_context *ipaCreate(); > > > + > > > #ifdef __cplusplus > > > } > > > > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > > > index 89fce0e8347f..cb2767a04711 100644 > > > --- a/src/libcamera/ipa_interface.cpp > > > +++ b/src/libcamera/ipa_interface.cpp > > > @@ -261,6 +261,16 @@ > > > * \sa libcamera::IPAInterface::processEvent() > > > */ > > > > > > +/** > > > + * \fn ipaCreate() > > > + * \brief Entry point to the IPA modules > > > + * > > > + * This function is the entry point to the IPA modules. It is implemented by > > > + * every IPA module, and called by libcamera to create a new IPA context. > > > + * > > > + * \return A newly created IPA context > > > + */ > > > + > > > > Yout might want to remove the prototype defined in the \file comment > > block in ipa_interface.cpp and let Doxygen link ipaCreate() mentions > > with this > > I like having it there, as it gives a nice overview without a need to > jump to the ipaCreate() documentation, but if you think it should be > removed, I can replace the two paragraphs with No no, more documentation it's fine, please take my tag in. > > * IPA modules shall expose a public function named ipaCreate(). The function > * creates an instance of an IPA context, which models a context of execution > * for the IPA. IPA modules shall support creating one context per camera, as > * required by their associated pipeline handler. > > Please let me know what you think is best. > > > Apart from that > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > namespace libcamera { > > > > > > /** > > -- > Regards, > > Laurent Pinchart
Hi Laurent, Thanks for your patch. On 2019-11-08 22:54:06 +0200, Laurent Pinchart wrote: > IPA modules have to implement a public ipaCreate() function, but its > prototype isn't declared in any header file. This allows for modules to > get the prototype wrong without being warned by the compiler. Fix it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/ipa/ipa_interface.h | 2 ++ > src/libcamera/ipa_interface.cpp | 10 ++++++++++ > 2 files changed, 12 insertions(+) > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > index 3a423e37671f..92f1aac50b85 100644 > --- a/include/ipa/ipa_interface.h > +++ b/include/ipa/ipa_interface.h > @@ -80,6 +80,8 @@ struct ipa_context_ops { > const struct ipa_operation_data *data); > }; > > +struct ipa_context *ipaCreate(); > + > #ifdef __cplusplus > } > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > index 89fce0e8347f..cb2767a04711 100644 > --- a/src/libcamera/ipa_interface.cpp > +++ b/src/libcamera/ipa_interface.cpp > @@ -261,6 +261,16 @@ > * \sa libcamera::IPAInterface::processEvent() > */ > > +/** > + * \fn ipaCreate() > + * \brief Entry point to the IPA modules > + * > + * This function is the entry point to the IPA modules. It is implemented by > + * every IPA module, and called by libcamera to create a new IPA context. > + * > + * \return A newly created IPA context > + */ > + > namespace libcamera { > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h index 3a423e37671f..92f1aac50b85 100644 --- a/include/ipa/ipa_interface.h +++ b/include/ipa/ipa_interface.h @@ -80,6 +80,8 @@ struct ipa_context_ops { const struct ipa_operation_data *data); }; +struct ipa_context *ipaCreate(); + #ifdef __cplusplus } diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp index 89fce0e8347f..cb2767a04711 100644 --- a/src/libcamera/ipa_interface.cpp +++ b/src/libcamera/ipa_interface.cpp @@ -261,6 +261,16 @@ * \sa libcamera::IPAInterface::processEvent() */ +/** + * \fn ipaCreate() + * \brief Entry point to the IPA modules + * + * This function is the entry point to the IPA modules. It is implemented by + * every IPA module, and called by libcamera to create a new IPA context. + * + * \return A newly created IPA context + */ + namespace libcamera { /**
IPA modules have to implement a public ipaCreate() function, but its prototype isn't declared in any header file. This allows for modules to get the prototype wrong without being warned by the compiler. Fix it. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/ipa/ipa_interface.h | 2 ++ src/libcamera/ipa_interface.cpp | 10 ++++++++++ 2 files changed, 12 insertions(+)