| Message ID | 20200706110847.11324-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, just a small nit On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote: > To make error handling easier in callers, allow the stop() function to > be called when the proxy is already stopped, or not started yet. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/internal/ipa_proxy.h | 2 ++ > src/libcamera/ipa_proxy.cpp | 10 ++++++++++ > src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++ > 3 files changed, 15 insertions(+) > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > index aec8f04ffc15..b429ce5a68a3 100644 > --- a/include/libcamera/internal/ipa_proxy.h > +++ b/include/libcamera/internal/ipa_proxy.h > @@ -27,6 +27,8 @@ public: > > std::string configurationFile(const std::string &file) const; > > + void stop() override = 0; > + > protected: > std::string resolvePath(const std::string &file) const; > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > index 23be24ad9bf1..01740a6e39ec 100644 > --- a/src/libcamera/ipa_proxy.cpp > +++ b/src/libcamera/ipa_proxy.cpp > @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const > return std::string(); > } > > +/** > + * \fn IPAProxy::stop() > + * \brief Stop the IPA proxy > + * > + * This method informs the IPA module that the camera is stopped. The IPA module > + * shall release resources prepared in start(). Calling stop() when the IPA s/prepared/acquired ? > + * hasn't been started or has already been stopped is valid, and the IPA shall > + * treat this as a no-op. > + */ > + > /** > * \brief Find a valid full path for a proxy worker for a given executable name > * \param[in] file File name of proxy worker executable > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp > index aa403e22f297..eead2883708d 100644 > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp > @@ -121,6 +121,9 @@ int IPAProxyThread::start() > > void IPAProxyThread::stop() > { > + if (!running_) > + return; > + In case the IPA is then stopped already, IPA won't receive the call, making the above "hasn't been started or has already been stopped is valid" possibly confusing. With out without these addressed Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > running_ = false; > > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Mon, Jul 06, 2020 at 01:49:13PM +0200, Jacopo Mondi wrote: > On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote: > > To make error handling easier in callers, allow the stop() function to > > be called when the proxy is already stopped, or not started yet. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/internal/ipa_proxy.h | 2 ++ > > src/libcamera/ipa_proxy.cpp | 10 ++++++++++ > > src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++ > > 3 files changed, 15 insertions(+) > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > index aec8f04ffc15..b429ce5a68a3 100644 > > --- a/include/libcamera/internal/ipa_proxy.h > > +++ b/include/libcamera/internal/ipa_proxy.h > > @@ -27,6 +27,8 @@ public: > > > > std::string configurationFile(const std::string &file) const; > > > > + void stop() override = 0; > > + > > protected: > > std::string resolvePath(const std::string &file) const; > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > index 23be24ad9bf1..01740a6e39ec 100644 > > --- a/src/libcamera/ipa_proxy.cpp > > +++ b/src/libcamera/ipa_proxy.cpp > > @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > return std::string(); > > } > > > > +/** > > + * \fn IPAProxy::stop() > > + * \brief Stop the IPA proxy > > + * > > + * This method informs the IPA module that the camera is stopped. The IPA module > > + * shall release resources prepared in start(). Calling stop() when the IPA > > s/prepared/acquired ? > > > + * hasn't been started or has already been stopped is valid, and the IPA shall > > + * treat this as a no-op. > > + */ > > + > > /** > > * \brief Find a valid full path for a proxy worker for a given executable name > > * \param[in] file File name of proxy worker executable > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp > > index aa403e22f297..eead2883708d 100644 > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp > > @@ -121,6 +121,9 @@ int IPAProxyThread::start() > > > > void IPAProxyThread::stop() > > { > > + if (!running_) > > + return; > > + > > In case the IPA is then stopped already, IPA won't receive the call, > making the above "hasn't been started or has already been stopped is > valid" possibly confusing. I agree with you. How about the following ? * This function stops the IPA and releases all the resources acquired by the * proxy in start(). Calling stop() when the IPA proxy hasn't been started or * has already been stopped is valid, the proxy shall treat this as a no-op and * shall not forward the call to the IPA. > With out without these addressed > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > running_ = false; > > > > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
Hi Laurent, On Mon, Jul 06, 2020 at 02:51:29PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Jul 06, 2020 at 01:49:13PM +0200, Jacopo Mondi wrote: > > On Mon, Jul 06, 2020 at 02:08:47PM +0300, Laurent Pinchart wrote: > > > To make error handling easier in callers, allow the stop() function to > > > be called when the proxy is already stopped, or not started yet. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/internal/ipa_proxy.h | 2 ++ > > > src/libcamera/ipa_proxy.cpp | 10 ++++++++++ > > > src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++ > > > 3 files changed, 15 insertions(+) > > > > > > diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h > > > index aec8f04ffc15..b429ce5a68a3 100644 > > > --- a/include/libcamera/internal/ipa_proxy.h > > > +++ b/include/libcamera/internal/ipa_proxy.h > > > @@ -27,6 +27,8 @@ public: > > > > > > std::string configurationFile(const std::string &file) const; > > > > > > + void stop() override = 0; > > > + > > > protected: > > > std::string resolvePath(const std::string &file) const; > > > > > > diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp > > > index 23be24ad9bf1..01740a6e39ec 100644 > > > --- a/src/libcamera/ipa_proxy.cpp > > > +++ b/src/libcamera/ipa_proxy.cpp > > > @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const > > > return std::string(); > > > } > > > > > > +/** > > > + * \fn IPAProxy::stop() > > > + * \brief Stop the IPA proxy > > > + * > > > + * This method informs the IPA module that the camera is stopped. The IPA module > > > + * shall release resources prepared in start(). Calling stop() when the IPA > > > > s/prepared/acquired ? > > > > > + * hasn't been started or has already been stopped is valid, and the IPA shall > > > + * treat this as a no-op. > > > + */ > > > + > > > /** > > > * \brief Find a valid full path for a proxy worker for a given executable name > > > * \param[in] file File name of proxy worker executable > > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp > > > index aa403e22f297..eead2883708d 100644 > > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp > > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp > > > @@ -121,6 +121,9 @@ int IPAProxyThread::start() > > > > > > void IPAProxyThread::stop() > > > { > > > + if (!running_) > > > + return; > > > + > > > > In case the IPA is then stopped already, IPA won't receive the call, > > making the above "hasn't been started or has already been stopped is > > valid" possibly confusing. > > I agree with you. How about the following ? > > * This function stops the IPA and releases all the resources acquired by the > * proxy in start(). Calling stop() when the IPA proxy hasn't been started or > * has already been stopped is valid, the proxy shall treat this as a no-op and > * shall not forward the call to the IPA. > Looks good, thanks! > > With out without these addressed > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > running_ = false; > > > > > > proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking); > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/internal/ipa_proxy.h b/include/libcamera/internal/ipa_proxy.h index aec8f04ffc15..b429ce5a68a3 100644 --- a/include/libcamera/internal/ipa_proxy.h +++ b/include/libcamera/internal/ipa_proxy.h @@ -27,6 +27,8 @@ public: std::string configurationFile(const std::string &file) const; + void stop() override = 0; + protected: std::string resolvePath(const std::string &file) const; diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp index 23be24ad9bf1..01740a6e39ec 100644 --- a/src/libcamera/ipa_proxy.cpp +++ b/src/libcamera/ipa_proxy.cpp @@ -145,6 +145,16 @@ std::string IPAProxy::configurationFile(const std::string &name) const return std::string(); } +/** + * \fn IPAProxy::stop() + * \brief Stop the IPA proxy + * + * This method informs the IPA module that the camera is stopped. The IPA module + * shall release resources prepared in start(). Calling stop() when the IPA + * hasn't been started or has already been stopped is valid, and the IPA shall + * treat this as a no-op. + */ + /** * \brief Find a valid full path for a proxy worker for a given executable name * \param[in] file File name of proxy worker executable diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp index aa403e22f297..eead2883708d 100644 --- a/src/libcamera/proxy/ipa_proxy_thread.cpp +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp @@ -121,6 +121,9 @@ int IPAProxyThread::start() void IPAProxyThread::stop() { + if (!running_) + return; + running_ = false; proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);
To make error handling easier in callers, allow the stop() function to be called when the proxy is already stopped, or not started yet. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/ipa_proxy.h | 2 ++ src/libcamera/ipa_proxy.cpp | 10 ++++++++++ src/libcamera/proxy/ipa_proxy_thread.cpp | 3 +++ 3 files changed, 15 insertions(+)