| Message ID | 20190812124642.24287-16-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Aug 12, 2019 at 03:46:39PM +0300, Laurent Pinchart wrote: > Add a parent Object to Object instances, and track the parent-children > relationships. Children are bound to the same thread as their parent, > and moving an Object to a thread automatically moves all its children. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/object.h | 8 ++++- > src/libcamera/include/thread.h | 2 ++ > src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ > src/libcamera/thread.cpp | 12 ++++++- > 4 files changed, 74 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h > index 14b939a9bd3d..4a0a272b875a 100644 > --- a/include/libcamera/object.h > +++ b/include/libcamera/object.h > @@ -9,6 +9,7 @@ > > #include <list> > #include <memory> > +#include <vector> > > #include <libcamera/bound_method.h> > > @@ -23,7 +24,7 @@ class Thread; > class Object > { > public: > - Object(); > + Object(Object *parent = nullptr); > virtual ~Object(); > > void postMessage(std::unique_ptr<Message> msg); > @@ -42,6 +43,8 @@ public: > Thread *thread() const { return thread_; } > void moveToThread(Thread *thread); > > + Object *parent() const { return parent_; } maybe const Object * ? > + > protected: > virtual void message(Message *msg); > > @@ -58,6 +61,9 @@ private: > void connect(SignalBase *signal); > void disconnect(SignalBase *signal); > > + Object *parent_; > + std::vector<Object *> children_; > + > Thread *thread_; > std::list<SignalBase *> signals_; > unsigned int pendingMessages_; > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > index 630abb49534f..37edd4f5138b 100644 > --- a/src/libcamera/include/thread.h > +++ b/src/libcamera/include/thread.h > @@ -61,6 +61,8 @@ private: > friend class ThreadMain; > > void moveObject(Object *object); > + void moveObject(Object *object, ThreadData *currentData, > + ThreadData *targetData); > > std::thread thread_; > ThreadData *data_; > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > index 7c68ec01f78c..57cd6fefe20f 100644 > --- a/src/libcamera/object.cpp > +++ b/src/libcamera/object.cpp > @@ -7,6 +7,8 @@ > > #include <libcamera/object.h> > > +#include <algorithm> > + > #include <libcamera/signal.h> > > #include "log.h" > @@ -21,6 +23,8 @@ > > namespace libcamera { > > +LOG_DEFINE_CATEGORY(Object) > + > /** > * \class Object > * \brief Base object to support automatic signal disconnection > @@ -29,10 +33,11 @@ namespace libcamera { > * slots. By inheriting from Object, an object is automatically disconnected > * from all connected signals when it gets destroyed. > * > - * Object instances are bound to the thread in which they're created. When a > - * message is posted to an object, its handler will run in the object's thread. > - * This allows implementing easy message passing between threads by inheriting > - * from the Object class. > + * Object instances are bound to the thread of their parent, or the thread in > + * which they're created when they have no parent. When a message is posted to > + * an object, its handler will run in the object's thread. This allows > + * implementing easy message passing between threads by inheriting from the > + * Object class. > * > * Object slots connected to signals will also run in the context of the > * object's thread, regardless of whether the signal is emitted in the same or > @@ -41,10 +46,20 @@ namespace libcamera { > * \sa Message, Signal, Thread > */ > > -Object::Object() > - : pendingMessages_(0) > +/** > + * \brief Construct an Object instance > + * \param[in] parent The object parent > + * > + * The new Object instance is bound to the thread of its \a parent, or to the > + * current thread if the \a parent is nullptr. > + */ > +Object::Object(Object *parent) > + : parent_(parent), pendingMessages_(0) > { > - thread_ = Thread::current(); > + thread_ = parent ? parent->thread() : Thread::current(); > + > + if (parent) > + parent->children_.push_back(this); > } > > Object::~Object() > @@ -54,6 +69,16 @@ Object::~Object() > > if (pendingMessages_) > thread()->removeMessages(this); > + > + if (parent_) { > + auto it = std::find(parent_->children_.begin(), > + parent_->children_.end(), this); > + ASSERT(it != parent_->children_.end()); > + parent_->children_.erase(it); > + } > + > + for (auto child : children_) > + child->parent_ = nullptr; > } > > /** > @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) > */ > > /** > - * \brief Move the object to a different thread > + * \brief Move the object and all its children to a different thread > * \param[in] thread The target thread > * > - * This method moves the object from the current thread to the new \a thread. > - * It shall be called from the thread in which the object currently lives, > - * otherwise the behaviour is undefined. > + * This method moves the object and all its children from the current thread to > + * the new \a thread. It shall be called from the thread in which the object > + * currently lives, otherwise the behaviour is undefined. > * > * Before the object is moved, a Message::ThreadMoveMessage message is sent to > * it. The message() method can be reimplement in derived classes to be notified > * of the upcoming thread move and perform any required processing. > + * > + * Moving an object that has a parent is not allowed, and causes undefined > + * behaviour. > */ > void Object::moveToThread(Thread *thread) > { > @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) > if (thread_ == thread) > return; > > + if (parent_) { > + LOG(Object, Error) > + << "Moving object to thread with a parent is not permitted"; > + return; > + } > + > notifyThreadMove(); > > thread->moveObject(this); > @@ -170,8 +204,17 @@ void Object::notifyThreadMove() > { > Message msg(Message::ThreadMoveMessage); > sendMessage(&msg); > + > + for (auto child : children_) > + child->notifyThreadMove(); > } > > +/** > + * \fn Object::parent() > + * \brief Retrieve the object's parent > + * \return The object's parent > + */ > + > void Object::connect(SignalBase *signal) > { > signals_.push_back(signal); > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > index 24422f7b7bd0..872ad1bd9d69 100644 > --- a/src/libcamera/thread.cpp > +++ b/src/libcamera/thread.cpp > @@ -448,7 +448,7 @@ void Thread::dispatchMessages() > } > > /** > - * \brief Move an \a object to the thread > + * \brief Move an \a object and all its children to the thread > * \param[in] object The object > */ > void Thread::moveObject(Object *object) > @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) > MutexLocker lockerTo(targetData->mutex_, std::defer_lock); > std::lock(lockerFrom, lockerTo); > > + moveObject(object, currentData, targetData); > +} > + > +void Thread::moveObject(Object *object, ThreadData *currentData, > + ThreadData *targetData) > +{ > /* Move pending messages to the message queue of the new thread. */ > if (object->pendingMessages_) { > unsigned int movedMessages = 0; > @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) > } > > object->thread_ = this; > + > + /* Move all children. */ > + for (auto child : object->children_) > + moveObject(child, currentData, targetData); > } > > }; /* namespace libcamera */ > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, Aug 15, 2019 at 11:56:47AM +0200, Jacopo Mondi wrote: > On Mon, Aug 12, 2019 at 03:46:39PM +0300, Laurent Pinchart wrote: > > Add a parent Object to Object instances, and track the parent-children > > relationships. Children are bound to the same thread as their parent, > > and moving an Object to a thread automatically moves all its children. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/object.h | 8 ++++- > > src/libcamera/include/thread.h | 2 ++ > > src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ > > src/libcamera/thread.cpp | 12 ++++++- > > 4 files changed, 74 insertions(+), 13 deletions(-) > > > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h > > index 14b939a9bd3d..4a0a272b875a 100644 > > --- a/include/libcamera/object.h > > +++ b/include/libcamera/object.h > > @@ -9,6 +9,7 @@ > > > > #include <list> > > #include <memory> > > +#include <vector> > > > > #include <libcamera/bound_method.h> > > > > @@ -23,7 +24,7 @@ class Thread; > > class Object > > { > > public: > > - Object(); > > + Object(Object *parent = nullptr); > > virtual ~Object(); > > > > void postMessage(std::unique_ptr<Message> msg); > > @@ -42,6 +43,8 @@ public: > > Thread *thread() const { return thread_; } > > void moveToThread(Thread *thread); > > > > + Object *parent() const { return parent_; } > > maybe const Object * ? There's a need to call non-const function of the parent, so we need a non-const pointer. I could add two versions of this method, const Object *parent() const and Object *parent(), but I don't think that's needed. The parent isn't part of the child, so modifying it doesn't modify the state of the child. > > + > > protected: > > virtual void message(Message *msg); > > > > @@ -58,6 +61,9 @@ private: > > void connect(SignalBase *signal); > > void disconnect(SignalBase *signal); > > > > + Object *parent_; > > + std::vector<Object *> children_; > > + > > Thread *thread_; > > std::list<SignalBase *> signals_; > > unsigned int pendingMessages_; > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > > index 630abb49534f..37edd4f5138b 100644 > > --- a/src/libcamera/include/thread.h > > +++ b/src/libcamera/include/thread.h > > @@ -61,6 +61,8 @@ private: > > friend class ThreadMain; > > > > void moveObject(Object *object); > > + void moveObject(Object *object, ThreadData *currentData, > > + ThreadData *targetData); > > > > std::thread thread_; > > ThreadData *data_; > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > > index 7c68ec01f78c..57cd6fefe20f 100644 > > --- a/src/libcamera/object.cpp > > +++ b/src/libcamera/object.cpp > > @@ -7,6 +7,8 @@ > > > > #include <libcamera/object.h> > > > > +#include <algorithm> > > + > > #include <libcamera/signal.h> > > > > #include "log.h" > > @@ -21,6 +23,8 @@ > > > > namespace libcamera { > > > > +LOG_DEFINE_CATEGORY(Object) > > + > > /** > > * \class Object > > * \brief Base object to support automatic signal disconnection > > @@ -29,10 +33,11 @@ namespace libcamera { > > * slots. By inheriting from Object, an object is automatically disconnected > > * from all connected signals when it gets destroyed. > > * > > - * Object instances are bound to the thread in which they're created. When a > > - * message is posted to an object, its handler will run in the object's thread. > > - * This allows implementing easy message passing between threads by inheriting > > - * from the Object class. > > + * Object instances are bound to the thread of their parent, or the thread in > > + * which they're created when they have no parent. When a message is posted to > > + * an object, its handler will run in the object's thread. This allows > > + * implementing easy message passing between threads by inheriting from the > > + * Object class. > > * > > * Object slots connected to signals will also run in the context of the > > * object's thread, regardless of whether the signal is emitted in the same or > > @@ -41,10 +46,20 @@ namespace libcamera { > > * \sa Message, Signal, Thread > > */ > > > > -Object::Object() > > - : pendingMessages_(0) > > +/** > > + * \brief Construct an Object instance > > + * \param[in] parent The object parent > > + * > > + * The new Object instance is bound to the thread of its \a parent, or to the > > + * current thread if the \a parent is nullptr. > > + */ > > +Object::Object(Object *parent) > > + : parent_(parent), pendingMessages_(0) > > { > > - thread_ = Thread::current(); > > + thread_ = parent ? parent->thread() : Thread::current(); > > + > > + if (parent) > > + parent->children_.push_back(this); > > } > > > > Object::~Object() > > @@ -54,6 +69,16 @@ Object::~Object() > > > > if (pendingMessages_) > > thread()->removeMessages(this); > > + > > + if (parent_) { > > + auto it = std::find(parent_->children_.begin(), > > + parent_->children_.end(), this); > > + ASSERT(it != parent_->children_.end()); > > + parent_->children_.erase(it); > > + } > > + > > + for (auto child : children_) > > + child->parent_ = nullptr; > > } > > > > /** > > @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) > > */ > > > > /** > > - * \brief Move the object to a different thread > > + * \brief Move the object and all its children to a different thread > > * \param[in] thread The target thread > > * > > - * This method moves the object from the current thread to the new \a thread. > > - * It shall be called from the thread in which the object currently lives, > > - * otherwise the behaviour is undefined. > > + * This method moves the object and all its children from the current thread to > > + * the new \a thread. It shall be called from the thread in which the object > > + * currently lives, otherwise the behaviour is undefined. > > * > > * Before the object is moved, a Message::ThreadMoveMessage message is sent to > > * it. The message() method can be reimplement in derived classes to be notified > > * of the upcoming thread move and perform any required processing. > > + * > > + * Moving an object that has a parent is not allowed, and causes undefined > > + * behaviour. > > */ > > void Object::moveToThread(Thread *thread) > > { > > @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) > > if (thread_ == thread) > > return; > > > > + if (parent_) { > > + LOG(Object, Error) > > + << "Moving object to thread with a parent is not permitted"; > > + return; > > + } > > + > > notifyThreadMove(); > > > > thread->moveObject(this); > > @@ -170,8 +204,17 @@ void Object::notifyThreadMove() > > { > > Message msg(Message::ThreadMoveMessage); > > sendMessage(&msg); > > + > > + for (auto child : children_) > > + child->notifyThreadMove(); > > } > > > > +/** > > + * \fn Object::parent() > > + * \brief Retrieve the object's parent > > + * \return The object's parent > > + */ > > + > > void Object::connect(SignalBase *signal) > > { > > signals_.push_back(signal); > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > > index 24422f7b7bd0..872ad1bd9d69 100644 > > --- a/src/libcamera/thread.cpp > > +++ b/src/libcamera/thread.cpp > > @@ -448,7 +448,7 @@ void Thread::dispatchMessages() > > } > > > > /** > > - * \brief Move an \a object to the thread > > + * \brief Move an \a object and all its children to the thread > > * \param[in] object The object > > */ > > void Thread::moveObject(Object *object) > > @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) > > MutexLocker lockerTo(targetData->mutex_, std::defer_lock); > > std::lock(lockerFrom, lockerTo); > > > > + moveObject(object, currentData, targetData); > > +} > > + > > +void Thread::moveObject(Object *object, ThreadData *currentData, > > + ThreadData *targetData) > > +{ > > /* Move pending messages to the message queue of the new thread. */ > > if (object->pendingMessages_) { > > unsigned int movedMessages = 0; > > @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) > > } > > > > object->thread_ = this; > > + > > + /* Move all children. */ > > + for (auto child : object->children_) > > + moveObject(child, currentData, targetData); > > } > > > > }; /* namespace libcamera */ > > -- > > Regards, > > > > Laurent Pinchart > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your work. On 2019-08-12 15:46:39 +0300, Laurent Pinchart wrote: > Add a parent Object to Object instances, and track the parent-children > relationships. Children are bound to the same thread as their parent, > and moving an Object to a thread automatically moves all its children. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > include/libcamera/object.h | 8 ++++- > src/libcamera/include/thread.h | 2 ++ > src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ > src/libcamera/thread.cpp | 12 ++++++- > 4 files changed, 74 insertions(+), 13 deletions(-) > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h > index 14b939a9bd3d..4a0a272b875a 100644 > --- a/include/libcamera/object.h > +++ b/include/libcamera/object.h > @@ -9,6 +9,7 @@ > > #include <list> > #include <memory> > +#include <vector> > > #include <libcamera/bound_method.h> > > @@ -23,7 +24,7 @@ class Thread; > class Object > { > public: > - Object(); > + Object(Object *parent = nullptr); > virtual ~Object(); > > void postMessage(std::unique_ptr<Message> msg); > @@ -42,6 +43,8 @@ public: > Thread *thread() const { return thread_; } > void moveToThread(Thread *thread); > > + Object *parent() const { return parent_; } > + > protected: > virtual void message(Message *msg); > > @@ -58,6 +61,9 @@ private: > void connect(SignalBase *signal); > void disconnect(SignalBase *signal); > > + Object *parent_; > + std::vector<Object *> children_; > + > Thread *thread_; > std::list<SignalBase *> signals_; > unsigned int pendingMessages_; > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > index 630abb49534f..37edd4f5138b 100644 > --- a/src/libcamera/include/thread.h > +++ b/src/libcamera/include/thread.h > @@ -61,6 +61,8 @@ private: > friend class ThreadMain; > > void moveObject(Object *object); > + void moveObject(Object *object, ThreadData *currentData, > + ThreadData *targetData); > > std::thread thread_; > ThreadData *data_; > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > index 7c68ec01f78c..57cd6fefe20f 100644 > --- a/src/libcamera/object.cpp > +++ b/src/libcamera/object.cpp > @@ -7,6 +7,8 @@ > > #include <libcamera/object.h> > > +#include <algorithm> > + > #include <libcamera/signal.h> > > #include "log.h" > @@ -21,6 +23,8 @@ > > namespace libcamera { > > +LOG_DEFINE_CATEGORY(Object) > + > /** > * \class Object > * \brief Base object to support automatic signal disconnection > @@ -29,10 +33,11 @@ namespace libcamera { > * slots. By inheriting from Object, an object is automatically disconnected > * from all connected signals when it gets destroyed. > * > - * Object instances are bound to the thread in which they're created. When a > - * message is posted to an object, its handler will run in the object's thread. > - * This allows implementing easy message passing between threads by inheriting > - * from the Object class. > + * Object instances are bound to the thread of their parent, or the thread in > + * which they're created when they have no parent. When a message is posted to > + * an object, its handler will run in the object's thread. This allows > + * implementing easy message passing between threads by inheriting from the > + * Object class. > * > * Object slots connected to signals will also run in the context of the > * object's thread, regardless of whether the signal is emitted in the same or > @@ -41,10 +46,20 @@ namespace libcamera { > * \sa Message, Signal, Thread > */ > > -Object::Object() > - : pendingMessages_(0) > +/** > + * \brief Construct an Object instance > + * \param[in] parent The object parent > + * > + * The new Object instance is bound to the thread of its \a parent, or to the > + * current thread if the \a parent is nullptr. > + */ > +Object::Object(Object *parent) > + : parent_(parent), pendingMessages_(0) > { > - thread_ = Thread::current(); > + thread_ = parent ? parent->thread() : Thread::current(); > + > + if (parent) > + parent->children_.push_back(this); > } > > Object::~Object() > @@ -54,6 +69,16 @@ Object::~Object() > > if (pendingMessages_) > thread()->removeMessages(this); > + > + if (parent_) { > + auto it = std::find(parent_->children_.begin(), > + parent_->children_.end(), this); > + ASSERT(it != parent_->children_.end()); > + parent_->children_.erase(it); > + } > + > + for (auto child : children_) > + child->parent_ = nullptr; Is this the behavior that is most logical? I'm not saying this is wrong but want to discuss it a bit. If a object with a parent and children is delete is it not more logical to move the children to the parent of the object being deleted? I fear this could lead to subtle bugs if objects who are part of a hierarchy become disjointed from the hierarchy when its parent is deleted. Maybe my gut feeling is wrong and my fear is unfounded. > } > > /** > @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) > */ > > /** > - * \brief Move the object to a different thread > + * \brief Move the object and all its children to a different thread > * \param[in] thread The target thread > * > - * This method moves the object from the current thread to the new \a thread. > - * It shall be called from the thread in which the object currently lives, > - * otherwise the behaviour is undefined. > + * This method moves the object and all its children from the current thread to > + * the new \a thread. It shall be called from the thread in which the object > + * currently lives, otherwise the behaviour is undefined. > * > * Before the object is moved, a Message::ThreadMoveMessage message is sent to > * it. The message() method can be reimplement in derived classes to be notified > * of the upcoming thread move and perform any required processing. > + * > + * Moving an object that has a parent is not allowed, and causes undefined > + * behaviour. > */ > void Object::moveToThread(Thread *thread) > { > @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) > if (thread_ == thread) > return; > > + if (parent_) { > + LOG(Object, Error) > + << "Moving object to thread with a parent is not permitted"; > + return; > + } > + > notifyThreadMove(); > > thread->moveObject(this); > @@ -170,8 +204,17 @@ void Object::notifyThreadMove() > { > Message msg(Message::ThreadMoveMessage); > sendMessage(&msg); > + > + for (auto child : children_) > + child->notifyThreadMove(); > } > > +/** > + * \fn Object::parent() > + * \brief Retrieve the object's parent > + * \return The object's parent > + */ > + > void Object::connect(SignalBase *signal) > { > signals_.push_back(signal); > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > index 24422f7b7bd0..872ad1bd9d69 100644 > --- a/src/libcamera/thread.cpp > +++ b/src/libcamera/thread.cpp > @@ -448,7 +448,7 @@ void Thread::dispatchMessages() > } > > /** > - * \brief Move an \a object to the thread > + * \brief Move an \a object and all its children to the thread > * \param[in] object The object > */ > void Thread::moveObject(Object *object) > @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) > MutexLocker lockerTo(targetData->mutex_, std::defer_lock); > std::lock(lockerFrom, lockerTo); > > + moveObject(object, currentData, targetData); > +} > + > +void Thread::moveObject(Object *object, ThreadData *currentData, > + ThreadData *targetData) > +{ > /* Move pending messages to the message queue of the new thread. */ > if (object->pendingMessages_) { > unsigned int movedMessages = 0; > @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) > } > > object->thread_ = this; > + > + /* Move all children. */ > + for (auto child : object->children_) > + moveObject(child, currentData, targetData); > } > > }; /* namespace libcamera */ > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sat, Aug 17, 2019 at 05:10:22PM +0200, Niklas Söderlund wrote: > On 2019-08-12 15:46:39 +0300, Laurent Pinchart wrote: > > Add a parent Object to Object instances, and track the parent-children > > relationships. Children are bound to the same thread as their parent, > > and moving an Object to a thread automatically moves all its children. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > include/libcamera/object.h | 8 ++++- > > src/libcamera/include/thread.h | 2 ++ > > src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ > > src/libcamera/thread.cpp | 12 ++++++- > > 4 files changed, 74 insertions(+), 13 deletions(-) > > > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h > > index 14b939a9bd3d..4a0a272b875a 100644 > > --- a/include/libcamera/object.h > > +++ b/include/libcamera/object.h > > @@ -9,6 +9,7 @@ > > > > #include <list> > > #include <memory> > > +#include <vector> > > > > #include <libcamera/bound_method.h> > > > > @@ -23,7 +24,7 @@ class Thread; > > class Object > > { > > public: > > - Object(); > > + Object(Object *parent = nullptr); > > virtual ~Object(); > > > > void postMessage(std::unique_ptr<Message> msg); > > @@ -42,6 +43,8 @@ public: > > Thread *thread() const { return thread_; } > > void moveToThread(Thread *thread); > > > > + Object *parent() const { return parent_; } > > + > > protected: > > virtual void message(Message *msg); > > > > @@ -58,6 +61,9 @@ private: > > void connect(SignalBase *signal); > > void disconnect(SignalBase *signal); > > > > + Object *parent_; > > + std::vector<Object *> children_; > > + > > Thread *thread_; > > std::list<SignalBase *> signals_; > > unsigned int pendingMessages_; > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > > index 630abb49534f..37edd4f5138b 100644 > > --- a/src/libcamera/include/thread.h > > +++ b/src/libcamera/include/thread.h > > @@ -61,6 +61,8 @@ private: > > friend class ThreadMain; > > > > void moveObject(Object *object); > > + void moveObject(Object *object, ThreadData *currentData, > > + ThreadData *targetData); > > > > std::thread thread_; > > ThreadData *data_; > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > > index 7c68ec01f78c..57cd6fefe20f 100644 > > --- a/src/libcamera/object.cpp > > +++ b/src/libcamera/object.cpp > > @@ -7,6 +7,8 @@ > > > > #include <libcamera/object.h> > > > > +#include <algorithm> > > + > > #include <libcamera/signal.h> > > > > #include "log.h" > > @@ -21,6 +23,8 @@ > > > > namespace libcamera { > > > > +LOG_DEFINE_CATEGORY(Object) > > + > > /** > > * \class Object > > * \brief Base object to support automatic signal disconnection > > @@ -29,10 +33,11 @@ namespace libcamera { > > * slots. By inheriting from Object, an object is automatically disconnected > > * from all connected signals when it gets destroyed. > > * > > - * Object instances are bound to the thread in which they're created. When a > > - * message is posted to an object, its handler will run in the object's thread. > > - * This allows implementing easy message passing between threads by inheriting > > - * from the Object class. > > + * Object instances are bound to the thread of their parent, or the thread in > > + * which they're created when they have no parent. When a message is posted to > > + * an object, its handler will run in the object's thread. This allows > > + * implementing easy message passing between threads by inheriting from the > > + * Object class. > > * > > * Object slots connected to signals will also run in the context of the > > * object's thread, regardless of whether the signal is emitted in the same or > > @@ -41,10 +46,20 @@ namespace libcamera { > > * \sa Message, Signal, Thread > > */ > > > > -Object::Object() > > - : pendingMessages_(0) > > +/** > > + * \brief Construct an Object instance > > + * \param[in] parent The object parent > > + * > > + * The new Object instance is bound to the thread of its \a parent, or to the > > + * current thread if the \a parent is nullptr. > > + */ > > +Object::Object(Object *parent) > > + : parent_(parent), pendingMessages_(0) > > { > > - thread_ = Thread::current(); > > + thread_ = parent ? parent->thread() : Thread::current(); > > + > > + if (parent) > > + parent->children_.push_back(this); > > } > > > > Object::~Object() > > @@ -54,6 +69,16 @@ Object::~Object() > > > > if (pendingMessages_) > > thread()->removeMessages(this); > > + > > + if (parent_) { > > + auto it = std::find(parent_->children_.begin(), > > + parent_->children_.end(), this); > > + ASSERT(it != parent_->children_.end()); > > + parent_->children_.erase(it); > > + } > > + > > + for (auto child : children_) > > + child->parent_ = nullptr; > > Is this the behavior that is most logical? I'm not saying this is wrong > but want to discuss it a bit. > > If a object with a parent and children is delete is it not more logical > to move the children to the parent of the object being deleted? I fear > this could lead to subtle bugs if objects who are part of a hierarchy > become disjointed from the hierarchy when its parent is deleted. It's a good question. This means that the children would need to be moved to the thread of their grand-parent, which may be an unwanted side effect. Another option would be to delete the children :-) I've been toying with this idea, I think it could simplify code, but I didn't want to include it in this series. In any case I'll already update the documentation to explain that children of a delete object become orphan. > Maybe my gut feeling is wrong and my fear is unfounded. > > > } > > > > /** > > @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) > > */ > > > > /** > > - * \brief Move the object to a different thread > > + * \brief Move the object and all its children to a different thread > > * \param[in] thread The target thread > > * > > - * This method moves the object from the current thread to the new \a thread. > > - * It shall be called from the thread in which the object currently lives, > > - * otherwise the behaviour is undefined. > > + * This method moves the object and all its children from the current thread to > > + * the new \a thread. It shall be called from the thread in which the object > > + * currently lives, otherwise the behaviour is undefined. > > * > > * Before the object is moved, a Message::ThreadMoveMessage message is sent to > > * it. The message() method can be reimplement in derived classes to be notified > > * of the upcoming thread move and perform any required processing. > > + * > > + * Moving an object that has a parent is not allowed, and causes undefined > > + * behaviour. > > */ > > void Object::moveToThread(Thread *thread) > > { > > @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) > > if (thread_ == thread) > > return; > > > > + if (parent_) { > > + LOG(Object, Error) > > + << "Moving object to thread with a parent is not permitted"; > > + return; > > + } > > + > > notifyThreadMove(); > > > > thread->moveObject(this); > > @@ -170,8 +204,17 @@ void Object::notifyThreadMove() > > { > > Message msg(Message::ThreadMoveMessage); > > sendMessage(&msg); > > + > > + for (auto child : children_) > > + child->notifyThreadMove(); > > } > > > > +/** > > + * \fn Object::parent() > > + * \brief Retrieve the object's parent > > + * \return The object's parent > > + */ > > + > > void Object::connect(SignalBase *signal) > > { > > signals_.push_back(signal); > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > > index 24422f7b7bd0..872ad1bd9d69 100644 > > --- a/src/libcamera/thread.cpp > > +++ b/src/libcamera/thread.cpp > > @@ -448,7 +448,7 @@ void Thread::dispatchMessages() > > } > > > > /** > > - * \brief Move an \a object to the thread > > + * \brief Move an \a object and all its children to the thread > > * \param[in] object The object > > */ > > void Thread::moveObject(Object *object) > > @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) > > MutexLocker lockerTo(targetData->mutex_, std::defer_lock); > > std::lock(lockerFrom, lockerTo); > > > > + moveObject(object, currentData, targetData); > > +} > > + > > +void Thread::moveObject(Object *object, ThreadData *currentData, > > + ThreadData *targetData) > > +{ > > /* Move pending messages to the message queue of the new thread. */ > > if (object->pendingMessages_) { > > unsigned int movedMessages = 0; > > @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) > > } > > > > object->thread_ = this; > > + > > + /* Move all children. */ > > + for (auto child : object->children_) > > + moveObject(child, currentData, targetData); > > } > > > > }; /* namespace libcamera */
Hi Laurent, On 2019-08-17 18:13:06 +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Sat, Aug 17, 2019 at 05:10:22PM +0200, Niklas Söderlund wrote: > > On 2019-08-12 15:46:39 +0300, Laurent Pinchart wrote: > > > Add a parent Object to Object instances, and track the parent-children > > > relationships. Children are bound to the same thread as their parent, > > > and moving an Object to a thread automatically moves all its children. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > include/libcamera/object.h | 8 ++++- > > > src/libcamera/include/thread.h | 2 ++ > > > src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ > > > src/libcamera/thread.cpp | 12 ++++++- > > > 4 files changed, 74 insertions(+), 13 deletions(-) > > > > > > diff --git a/include/libcamera/object.h b/include/libcamera/object.h > > > index 14b939a9bd3d..4a0a272b875a 100644 > > > --- a/include/libcamera/object.h > > > +++ b/include/libcamera/object.h > > > @@ -9,6 +9,7 @@ > > > > > > #include <list> > > > #include <memory> > > > +#include <vector> > > > > > > #include <libcamera/bound_method.h> > > > > > > @@ -23,7 +24,7 @@ class Thread; > > > class Object > > > { > > > public: > > > - Object(); > > > + Object(Object *parent = nullptr); > > > virtual ~Object(); > > > > > > void postMessage(std::unique_ptr<Message> msg); > > > @@ -42,6 +43,8 @@ public: > > > Thread *thread() const { return thread_; } > > > void moveToThread(Thread *thread); > > > > > > + Object *parent() const { return parent_; } > > > + > > > protected: > > > virtual void message(Message *msg); > > > > > > @@ -58,6 +61,9 @@ private: > > > void connect(SignalBase *signal); > > > void disconnect(SignalBase *signal); > > > > > > + Object *parent_; > > > + std::vector<Object *> children_; > > > + > > > Thread *thread_; > > > std::list<SignalBase *> signals_; > > > unsigned int pendingMessages_; > > > diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h > > > index 630abb49534f..37edd4f5138b 100644 > > > --- a/src/libcamera/include/thread.h > > > +++ b/src/libcamera/include/thread.h > > > @@ -61,6 +61,8 @@ private: > > > friend class ThreadMain; > > > > > > void moveObject(Object *object); > > > + void moveObject(Object *object, ThreadData *currentData, > > > + ThreadData *targetData); > > > > > > std::thread thread_; > > > ThreadData *data_; > > > diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp > > > index 7c68ec01f78c..57cd6fefe20f 100644 > > > --- a/src/libcamera/object.cpp > > > +++ b/src/libcamera/object.cpp > > > @@ -7,6 +7,8 @@ > > > > > > #include <libcamera/object.h> > > > > > > +#include <algorithm> > > > + > > > #include <libcamera/signal.h> > > > > > > #include "log.h" > > > @@ -21,6 +23,8 @@ > > > > > > namespace libcamera { > > > > > > +LOG_DEFINE_CATEGORY(Object) > > > + > > > /** > > > * \class Object > > > * \brief Base object to support automatic signal disconnection > > > @@ -29,10 +33,11 @@ namespace libcamera { > > > * slots. By inheriting from Object, an object is automatically disconnected > > > * from all connected signals when it gets destroyed. > > > * > > > - * Object instances are bound to the thread in which they're created. When a > > > - * message is posted to an object, its handler will run in the object's thread. > > > - * This allows implementing easy message passing between threads by inheriting > > > - * from the Object class. > > > + * Object instances are bound to the thread of their parent, or the thread in > > > + * which they're created when they have no parent. When a message is posted to > > > + * an object, its handler will run in the object's thread. This allows > > > + * implementing easy message passing between threads by inheriting from the > > > + * Object class. > > > * > > > * Object slots connected to signals will also run in the context of the > > > * object's thread, regardless of whether the signal is emitted in the same or > > > @@ -41,10 +46,20 @@ namespace libcamera { > > > * \sa Message, Signal, Thread > > > */ > > > > > > -Object::Object() > > > - : pendingMessages_(0) > > > +/** > > > + * \brief Construct an Object instance > > > + * \param[in] parent The object parent > > > + * > > > + * The new Object instance is bound to the thread of its \a parent, or to the > > > + * current thread if the \a parent is nullptr. > > > + */ > > > +Object::Object(Object *parent) > > > + : parent_(parent), pendingMessages_(0) > > > { > > > - thread_ = Thread::current(); > > > + thread_ = parent ? parent->thread() : Thread::current(); > > > + > > > + if (parent) > > > + parent->children_.push_back(this); > > > } > > > > > > Object::~Object() > > > @@ -54,6 +69,16 @@ Object::~Object() > > > > > > if (pendingMessages_) > > > thread()->removeMessages(this); > > > + > > > + if (parent_) { > > > + auto it = std::find(parent_->children_.begin(), > > > + parent_->children_.end(), this); > > > + ASSERT(it != parent_->children_.end()); > > > + parent_->children_.erase(it); > > > + } > > > + > > > + for (auto child : children_) > > > + child->parent_ = nullptr; > > > > Is this the behavior that is most logical? I'm not saying this is wrong > > but want to discuss it a bit. > > > > If a object with a parent and children is delete is it not more logical > > to move the children to the parent of the object being deleted? I fear > > this could lead to subtle bugs if objects who are part of a hierarchy > > become disjointed from the hierarchy when its parent is deleted. > > It's a good question. This means that the children would need to be > moved to the thread of their grand-parent, which may be an unwanted side > effect. Another option would be to delete the children :-) I've been > toying with this idea, I think it could simplify code, but I didn't want > to include it in this series. I toyed with the idea of deleting children too, but it seemed like such a large change it might be best left for later once this is settled a bit. > > In any case I'll already update the documentation to explain that > children of a delete object become orphan. Documenting the behavior solves the immediate issue, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > Maybe my gut feeling is wrong and my fear is unfounded. > > > > > } > > > > > > /** > > > @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) > > > */ > > > > > > /** > > > - * \brief Move the object to a different thread > > > + * \brief Move the object and all its children to a different thread > > > * \param[in] thread The target thread > > > * > > > - * This method moves the object from the current thread to the new \a thread. > > > - * It shall be called from the thread in which the object currently lives, > > > - * otherwise the behaviour is undefined. > > > + * This method moves the object and all its children from the current thread to > > > + * the new \a thread. It shall be called from the thread in which the object > > > + * currently lives, otherwise the behaviour is undefined. > > > * > > > * Before the object is moved, a Message::ThreadMoveMessage message is sent to > > > * it. The message() method can be reimplement in derived classes to be notified > > > * of the upcoming thread move and perform any required processing. > > > + * > > > + * Moving an object that has a parent is not allowed, and causes undefined > > > + * behaviour. > > > */ > > > void Object::moveToThread(Thread *thread) > > > { > > > @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) > > > if (thread_ == thread) > > > return; > > > > > > + if (parent_) { > > > + LOG(Object, Error) > > > + << "Moving object to thread with a parent is not permitted"; > > > + return; > > > + } > > > + > > > notifyThreadMove(); > > > > > > thread->moveObject(this); > > > @@ -170,8 +204,17 @@ void Object::notifyThreadMove() > > > { > > > Message msg(Message::ThreadMoveMessage); > > > sendMessage(&msg); > > > + > > > + for (auto child : children_) > > > + child->notifyThreadMove(); > > > } > > > > > > +/** > > > + * \fn Object::parent() > > > + * \brief Retrieve the object's parent > > > + * \return The object's parent > > > + */ > > > + > > > void Object::connect(SignalBase *signal) > > > { > > > signals_.push_back(signal); > > > diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp > > > index 24422f7b7bd0..872ad1bd9d69 100644 > > > --- a/src/libcamera/thread.cpp > > > +++ b/src/libcamera/thread.cpp > > > @@ -448,7 +448,7 @@ void Thread::dispatchMessages() > > > } > > > > > > /** > > > - * \brief Move an \a object to the thread > > > + * \brief Move an \a object and all its children to the thread > > > * \param[in] object The object > > > */ > > > void Thread::moveObject(Object *object) > > > @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) > > > MutexLocker lockerTo(targetData->mutex_, std::defer_lock); > > > std::lock(lockerFrom, lockerTo); > > > > > > + moveObject(object, currentData, targetData); > > > +} > > > + > > > +void Thread::moveObject(Object *object, ThreadData *currentData, > > > + ThreadData *targetData) > > > +{ > > > /* Move pending messages to the message queue of the new thread. */ > > > if (object->pendingMessages_) { > > > unsigned int movedMessages = 0; > > > @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) > > > } > > > > > > object->thread_ = this; > > > + > > > + /* Move all children. */ > > > + for (auto child : object->children_) > > > + moveObject(child, currentData, targetData); > > > } > > > > > > }; /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/include/libcamera/object.h b/include/libcamera/object.h index 14b939a9bd3d..4a0a272b875a 100644 --- a/include/libcamera/object.h +++ b/include/libcamera/object.h @@ -9,6 +9,7 @@ #include <list> #include <memory> +#include <vector> #include <libcamera/bound_method.h> @@ -23,7 +24,7 @@ class Thread; class Object { public: - Object(); + Object(Object *parent = nullptr); virtual ~Object(); void postMessage(std::unique_ptr<Message> msg); @@ -42,6 +43,8 @@ public: Thread *thread() const { return thread_; } void moveToThread(Thread *thread); + Object *parent() const { return parent_; } + protected: virtual void message(Message *msg); @@ -58,6 +61,9 @@ private: void connect(SignalBase *signal); void disconnect(SignalBase *signal); + Object *parent_; + std::vector<Object *> children_; + Thread *thread_; std::list<SignalBase *> signals_; unsigned int pendingMessages_; diff --git a/src/libcamera/include/thread.h b/src/libcamera/include/thread.h index 630abb49534f..37edd4f5138b 100644 --- a/src/libcamera/include/thread.h +++ b/src/libcamera/include/thread.h @@ -61,6 +61,8 @@ private: friend class ThreadMain; void moveObject(Object *object); + void moveObject(Object *object, ThreadData *currentData, + ThreadData *targetData); std::thread thread_; ThreadData *data_; diff --git a/src/libcamera/object.cpp b/src/libcamera/object.cpp index 7c68ec01f78c..57cd6fefe20f 100644 --- a/src/libcamera/object.cpp +++ b/src/libcamera/object.cpp @@ -7,6 +7,8 @@ #include <libcamera/object.h> +#include <algorithm> + #include <libcamera/signal.h> #include "log.h" @@ -21,6 +23,8 @@ namespace libcamera { +LOG_DEFINE_CATEGORY(Object) + /** * \class Object * \brief Base object to support automatic signal disconnection @@ -29,10 +33,11 @@ namespace libcamera { * slots. By inheriting from Object, an object is automatically disconnected * from all connected signals when it gets destroyed. * - * Object instances are bound to the thread in which they're created. When a - * message is posted to an object, its handler will run in the object's thread. - * This allows implementing easy message passing between threads by inheriting - * from the Object class. + * Object instances are bound to the thread of their parent, or the thread in + * which they're created when they have no parent. When a message is posted to + * an object, its handler will run in the object's thread. This allows + * implementing easy message passing between threads by inheriting from the + * Object class. * * Object slots connected to signals will also run in the context of the * object's thread, regardless of whether the signal is emitted in the same or @@ -41,10 +46,20 @@ namespace libcamera { * \sa Message, Signal, Thread */ -Object::Object() - : pendingMessages_(0) +/** + * \brief Construct an Object instance + * \param[in] parent The object parent + * + * The new Object instance is bound to the thread of its \a parent, or to the + * current thread if the \a parent is nullptr. + */ +Object::Object(Object *parent) + : parent_(parent), pendingMessages_(0) { - thread_ = Thread::current(); + thread_ = parent ? parent->thread() : Thread::current(); + + if (parent) + parent->children_.push_back(this); } Object::~Object() @@ -54,6 +69,16 @@ Object::~Object() if (pendingMessages_) thread()->removeMessages(this); + + if (parent_) { + auto it = std::find(parent_->children_.begin(), + parent_->children_.end(), this); + ASSERT(it != parent_->children_.end()); + parent_->children_.erase(it); + } + + for (auto child : children_) + child->parent_ = nullptr; } /** @@ -143,16 +168,19 @@ void Object::invokeMethod(BoundMethodBase *method, void *args) */ /** - * \brief Move the object to a different thread + * \brief Move the object and all its children to a different thread * \param[in] thread The target thread * - * This method moves the object from the current thread to the new \a thread. - * It shall be called from the thread in which the object currently lives, - * otherwise the behaviour is undefined. + * This method moves the object and all its children from the current thread to + * the new \a thread. It shall be called from the thread in which the object + * currently lives, otherwise the behaviour is undefined. * * Before the object is moved, a Message::ThreadMoveMessage message is sent to * it. The message() method can be reimplement in derived classes to be notified * of the upcoming thread move and perform any required processing. + * + * Moving an object that has a parent is not allowed, and causes undefined + * behaviour. */ void Object::moveToThread(Thread *thread) { @@ -161,6 +189,12 @@ void Object::moveToThread(Thread *thread) if (thread_ == thread) return; + if (parent_) { + LOG(Object, Error) + << "Moving object to thread with a parent is not permitted"; + return; + } + notifyThreadMove(); thread->moveObject(this); @@ -170,8 +204,17 @@ void Object::notifyThreadMove() { Message msg(Message::ThreadMoveMessage); sendMessage(&msg); + + for (auto child : children_) + child->notifyThreadMove(); } +/** + * \fn Object::parent() + * \brief Retrieve the object's parent + * \return The object's parent + */ + void Object::connect(SignalBase *signal) { signals_.push_back(signal); diff --git a/src/libcamera/thread.cpp b/src/libcamera/thread.cpp index 24422f7b7bd0..872ad1bd9d69 100644 --- a/src/libcamera/thread.cpp +++ b/src/libcamera/thread.cpp @@ -448,7 +448,7 @@ void Thread::dispatchMessages() } /** - * \brief Move an \a object to the thread + * \brief Move an \a object and all its children to the thread * \param[in] object The object */ void Thread::moveObject(Object *object) @@ -460,6 +460,12 @@ void Thread::moveObject(Object *object) MutexLocker lockerTo(targetData->mutex_, std::defer_lock); std::lock(lockerFrom, lockerTo); + moveObject(object, currentData, targetData); +} + +void Thread::moveObject(Object *object, ThreadData *currentData, + ThreadData *targetData) +{ /* Move pending messages to the message queue of the new thread. */ if (object->pendingMessages_) { unsigned int movedMessages = 0; @@ -483,6 +489,10 @@ void Thread::moveObject(Object *object) } object->thread_ = this; + + /* Move all children. */ + for (auto child : object->children_) + moveObject(child, currentData, targetData); } }; /* namespace libcamera */
Add a parent Object to Object instances, and track the parent-children relationships. Children are bound to the same thread as their parent, and moving an Object to a thread automatically moves all its children. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/object.h | 8 ++++- src/libcamera/include/thread.h | 2 ++ src/libcamera/object.cpp | 65 ++++++++++++++++++++++++++++------ src/libcamera/thread.cpp | 12 ++++++- 4 files changed, 74 insertions(+), 13 deletions(-)