| Message ID | 20200104050947.7673-10-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | b0135a1522ed4217a8deb7929fdb36276e58161b |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-01-04 07:09:42 +0200, Laurent Pinchart wrote: > The bound method arguments pack will need to be accessed by the method > invoker in order to retrieve the method return value when using a > blocking connection type. We thus can't delete the pack unconditionally > in the bound method target thread. We also can't delete it > unconditionally in the invoker's thread, as for queued connections the > pack will be used in the target thread after the invoker completes. > > This shows that ownership of the arguments pack is shared between two > contexts. As a result, manage it using std::shared_ptr<>. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > Documentation/Doxyfile.in | 1 + > include/libcamera/bound_method.h | 54 +++++++++++++++++++------------- > src/libcamera/bound_method.cpp | 5 +-- > src/libcamera/include/message.h | 5 +-- > src/libcamera/message.cpp | 5 +-- > 5 files changed, 43 insertions(+), 27 deletions(-) > > diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > index 9e5efae33919..5ae8773bd3ad 100644 > --- a/Documentation/Doxyfile.in > +++ b/Documentation/Doxyfile.in > @@ -874,6 +874,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMemberMethod \ > libcamera::BoundMethodArgs \ > libcamera::BoundMethodBase \ > libcamera::BoundMethodPack \ > + libcamera::BoundMethodPackBase \ > libcamera::BoundStaticMethod \ > libcamera::SignalBase \ > std::* > diff --git a/include/libcamera/bound_method.h b/include/libcamera/bound_method.h > index b50072ffc098..a74d2c503cdb 100644 > --- a/include/libcamera/bound_method.h > +++ b/include/libcamera/bound_method.h > @@ -7,6 +7,7 @@ > #ifndef __LIBCAMERA_BOUND_METHOD_H__ > #define __LIBCAMERA_BOUND_METHOD_H__ > > +#include <memory> > #include <tuple> > #include <type_traits> > > @@ -21,6 +22,24 @@ enum ConnectionType { > ConnectionTypeBlocking, > }; > > +class BoundMethodPackBase > +{ > +public: > + virtual ~BoundMethodPackBase() {} > +}; > + > +template<typename... Args> > +class BoundMethodPack : public BoundMethodPackBase > +{ > +public: > + BoundMethodPack(const Args &... args) > + : args_(args...) > + { > + } > + > + std::tuple<typename std::remove_reference<Args>::type...> args_; > +}; > + > class BoundMethodBase > { > public: > @@ -36,7 +55,7 @@ public: > > Object *object() const { return object_; } > > - virtual void invokePack(void *pack) = 0; > + virtual void invokePack(BoundMethodPackBase *pack) = 0; > > protected: > #ifndef __DOXYGEN__ > @@ -58,7 +77,8 @@ protected: > }; > #endif > > - void activatePack(void *pack, bool deleteMethod); > + void activatePack(std::shared_ptr<BoundMethodPackBase> pack, > + bool deleteMethod); > > void *obj_; > Object *object_; > @@ -67,18 +87,6 @@ private: > ConnectionType connectionType_; > }; > > -template<typename... Args> > -class BoundMethodPack > -{ > -public: > - BoundMethodPack(const Args &... args) > - : args_(args...) > - { > - } > - > - std::tuple<typename std::remove_reference<Args>::type...> args_; > -}; > - > template<typename R, typename... Args> > class BoundMethodArgs : public BoundMethodBase > { > @@ -87,18 +95,18 @@ public: > > private: > template<int... S> > - void invokePack(void *pack, BoundMethodBase::sequence<S...>) > + void invokePack(BoundMethodPackBase *pack, BoundMethodBase::sequence<S...>) > { > - PackType *args = static_cast<PackType *>(pack); > + /* args is effectively unused when the sequence S is empty. */ > + PackType *args [[gnu::unused]] = static_cast<PackType *>(pack); > invoke(std::get<S>(args->args_)...); > - delete args; > } > > public: > BoundMethodArgs(void *obj, Object *object, ConnectionType type) > : BoundMethodBase(obj, object, type) {} > > - void invokePack(void *pack) override > + void invokePack(BoundMethodPackBase *pack) override > { > invokePack(pack, typename BoundMethodBase::generator<sizeof...(Args)>::type()); > } > @@ -123,10 +131,14 @@ public: > > void activate(Args... args, bool deleteMethod = false) override > { > - if (this->object_) > - BoundMethodBase::activatePack(new PackType{ args... }, deleteMethod); > - else > + if (!this->object_) { > (static_cast<T *>(this->obj_)->*func_)(args...); > + return; > + } > + > + std::shared_ptr<BoundMethodPackBase> pack = > + std::make_shared<typename BoundMemberMethod<T, R, Args...>::PackType>(args...); > + BoundMethodBase::activatePack(pack, deleteMethod); > } > > void invoke(Args... args) override > diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp > index 45c765774801..82339b7e9c95 100644 > --- a/src/libcamera/bound_method.cpp > +++ b/src/libcamera/bound_method.cpp > @@ -48,7 +48,8 @@ namespace libcamera { > * deadlock will occur. > */ > > -void BoundMethodBase::activatePack(void *pack, bool deleteMethod) > +void BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack, > + bool deleteMethod) > { > ConnectionType type = connectionType_; > if (type == ConnectionTypeAuto) { > @@ -61,7 +62,7 @@ void BoundMethodBase::activatePack(void *pack, bool deleteMethod) > switch (type) { > case ConnectionTypeDirect: > default: > - invokePack(pack); > + invokePack(pack.get()); > if (deleteMethod) > delete this; > break; > diff --git a/src/libcamera/include/message.h b/src/libcamera/include/message.h > index 311755cc60fe..8e8b013dcd18 100644 > --- a/src/libcamera/include/message.h > +++ b/src/libcamera/include/message.h > @@ -48,7 +48,8 @@ private: > class InvokeMessage : public Message > { > public: > - InvokeMessage(BoundMethodBase *method, void *pack, > + InvokeMessage(BoundMethodBase *method, > + std::shared_ptr<BoundMethodPackBase> pack, > Semaphore *semaphore = nullptr, > bool deleteMethod = false); > ~InvokeMessage(); > @@ -59,7 +60,7 @@ public: > > private: > BoundMethodBase *method_; > - void *pack_; > + std::shared_ptr<BoundMethodPackBase> pack_; > Semaphore *semaphore_; > bool deleteMethod_; > }; > diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp > index c35bb33dfeda..77f2bdd5fbac 100644 > --- a/src/libcamera/message.cpp > +++ b/src/libcamera/message.cpp > @@ -123,7 +123,8 @@ Message::Type Message::registerMessageType() > * \param[in] deleteMethod True to delete the \a method when the message is > * destroyed > */ > -InvokeMessage::InvokeMessage(BoundMethodBase *method, void *pack, > +InvokeMessage::InvokeMessage(BoundMethodBase *method, > + std::shared_ptr<BoundMethodPackBase> pack, > Semaphore *semaphore, bool deleteMethod) > : Message(Message::InvokeMessage), method_(method), pack_(pack), > semaphore_(semaphore), deleteMethod_(deleteMethod) > @@ -148,7 +149,7 @@ InvokeMessage::~InvokeMessage() > */ > void InvokeMessage::invoke() > { > - method_->invokePack(pack_); > + method_->invokePack(pack_.get()); > } > > /** > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in index 9e5efae33919..5ae8773bd3ad 100644 --- a/Documentation/Doxyfile.in +++ b/Documentation/Doxyfile.in @@ -874,6 +874,7 @@ EXCLUDE_SYMBOLS = libcamera::BoundMemberMethod \ libcamera::BoundMethodArgs \ libcamera::BoundMethodBase \ libcamera::BoundMethodPack \ + libcamera::BoundMethodPackBase \ libcamera::BoundStaticMethod \ libcamera::SignalBase \ std::* diff --git a/include/libcamera/bound_method.h b/include/libcamera/bound_method.h index b50072ffc098..a74d2c503cdb 100644 --- a/include/libcamera/bound_method.h +++ b/include/libcamera/bound_method.h @@ -7,6 +7,7 @@ #ifndef __LIBCAMERA_BOUND_METHOD_H__ #define __LIBCAMERA_BOUND_METHOD_H__ +#include <memory> #include <tuple> #include <type_traits> @@ -21,6 +22,24 @@ enum ConnectionType { ConnectionTypeBlocking, }; +class BoundMethodPackBase +{ +public: + virtual ~BoundMethodPackBase() {} +}; + +template<typename... Args> +class BoundMethodPack : public BoundMethodPackBase +{ +public: + BoundMethodPack(const Args &... args) + : args_(args...) + { + } + + std::tuple<typename std::remove_reference<Args>::type...> args_; +}; + class BoundMethodBase { public: @@ -36,7 +55,7 @@ public: Object *object() const { return object_; } - virtual void invokePack(void *pack) = 0; + virtual void invokePack(BoundMethodPackBase *pack) = 0; protected: #ifndef __DOXYGEN__ @@ -58,7 +77,8 @@ protected: }; #endif - void activatePack(void *pack, bool deleteMethod); + void activatePack(std::shared_ptr<BoundMethodPackBase> pack, + bool deleteMethod); void *obj_; Object *object_; @@ -67,18 +87,6 @@ private: ConnectionType connectionType_; }; -template<typename... Args> -class BoundMethodPack -{ -public: - BoundMethodPack(const Args &... args) - : args_(args...) - { - } - - std::tuple<typename std::remove_reference<Args>::type...> args_; -}; - template<typename R, typename... Args> class BoundMethodArgs : public BoundMethodBase { @@ -87,18 +95,18 @@ public: private: template<int... S> - void invokePack(void *pack, BoundMethodBase::sequence<S...>) + void invokePack(BoundMethodPackBase *pack, BoundMethodBase::sequence<S...>) { - PackType *args = static_cast<PackType *>(pack); + /* args is effectively unused when the sequence S is empty. */ + PackType *args [[gnu::unused]] = static_cast<PackType *>(pack); invoke(std::get<S>(args->args_)...); - delete args; } public: BoundMethodArgs(void *obj, Object *object, ConnectionType type) : BoundMethodBase(obj, object, type) {} - void invokePack(void *pack) override + void invokePack(BoundMethodPackBase *pack) override { invokePack(pack, typename BoundMethodBase::generator<sizeof...(Args)>::type()); } @@ -123,10 +131,14 @@ public: void activate(Args... args, bool deleteMethod = false) override { - if (this->object_) - BoundMethodBase::activatePack(new PackType{ args... }, deleteMethod); - else + if (!this->object_) { (static_cast<T *>(this->obj_)->*func_)(args...); + return; + } + + std::shared_ptr<BoundMethodPackBase> pack = + std::make_shared<typename BoundMemberMethod<T, R, Args...>::PackType>(args...); + BoundMethodBase::activatePack(pack, deleteMethod); } void invoke(Args... args) override diff --git a/src/libcamera/bound_method.cpp b/src/libcamera/bound_method.cpp index 45c765774801..82339b7e9c95 100644 --- a/src/libcamera/bound_method.cpp +++ b/src/libcamera/bound_method.cpp @@ -48,7 +48,8 @@ namespace libcamera { * deadlock will occur. */ -void BoundMethodBase::activatePack(void *pack, bool deleteMethod) +void BoundMethodBase::activatePack(std::shared_ptr<BoundMethodPackBase> pack, + bool deleteMethod) { ConnectionType type = connectionType_; if (type == ConnectionTypeAuto) { @@ -61,7 +62,7 @@ void BoundMethodBase::activatePack(void *pack, bool deleteMethod) switch (type) { case ConnectionTypeDirect: default: - invokePack(pack); + invokePack(pack.get()); if (deleteMethod) delete this; break; diff --git a/src/libcamera/include/message.h b/src/libcamera/include/message.h index 311755cc60fe..8e8b013dcd18 100644 --- a/src/libcamera/include/message.h +++ b/src/libcamera/include/message.h @@ -48,7 +48,8 @@ private: class InvokeMessage : public Message { public: - InvokeMessage(BoundMethodBase *method, void *pack, + InvokeMessage(BoundMethodBase *method, + std::shared_ptr<BoundMethodPackBase> pack, Semaphore *semaphore = nullptr, bool deleteMethod = false); ~InvokeMessage(); @@ -59,7 +60,7 @@ public: private: BoundMethodBase *method_; - void *pack_; + std::shared_ptr<BoundMethodPackBase> pack_; Semaphore *semaphore_; bool deleteMethod_; }; diff --git a/src/libcamera/message.cpp b/src/libcamera/message.cpp index c35bb33dfeda..77f2bdd5fbac 100644 --- a/src/libcamera/message.cpp +++ b/src/libcamera/message.cpp @@ -123,7 +123,8 @@ Message::Type Message::registerMessageType() * \param[in] deleteMethod True to delete the \a method when the message is * destroyed */ -InvokeMessage::InvokeMessage(BoundMethodBase *method, void *pack, +InvokeMessage::InvokeMessage(BoundMethodBase *method, + std::shared_ptr<BoundMethodPackBase> pack, Semaphore *semaphore, bool deleteMethod) : Message(Message::InvokeMessage), method_(method), pack_(pack), semaphore_(semaphore), deleteMethod_(deleteMethod) @@ -148,7 +149,7 @@ InvokeMessage::~InvokeMessage() */ void InvokeMessage::invoke() { - method_->invokePack(pack_); + method_->invokePack(pack_.get()); } /**
The bound method arguments pack will need to be accessed by the method invoker in order to retrieve the method return value when using a blocking connection type. We thus can't delete the pack unconditionally in the bound method target thread. We also can't delete it unconditionally in the invoker's thread, as for queued connections the pack will be used in the target thread after the invoker completes. This shows that ownership of the arguments pack is shared between two contexts. As a result, manage it using std::shared_ptr<>. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Documentation/Doxyfile.in | 1 + include/libcamera/bound_method.h | 54 +++++++++++++++++++------------- src/libcamera/bound_method.cpp | 5 +-- src/libcamera/include/message.h | 5 +-- src/libcamera/message.cpp | 5 +-- 5 files changed, 43 insertions(+), 27 deletions(-)