[libcamera-devel] [PATCH 09/14] libcamera: bound_method: Manage BoundMethodPack through std::shared_ptr
Niklas Söderlund
niklas.soderlund at ragnatech.se
Tue Jan 7 20:16:58 CET 2020
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 at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list