diff --git a/src/common/singletonpolicies.hpp b/src/common/singletonpolicies.hpp index 5f5624990..3093d275d 100644 --- a/src/common/singletonpolicies.hpp +++ b/src/common/singletonpolicies.hpp @@ -47,7 +47,7 @@ namespace cinelerra /* == several Policies usable in conjunction with cinelerra::Singleton == */ /** - * Policy for creating the Singleton instance statically + * Policy placing the Singleton instance into a statically allocated buffer */ template struct StaticCreate diff --git a/src/nobugcfg.h b/src/nobugcfg.h index c16a569c0..7907d50a1 100644 --- a/src/nobugcfg.h +++ b/src/nobugcfg.h @@ -49,6 +49,7 @@ /* declare flags used throughout the code base... */ NOBUG_DECLARE_FLAG(config); + NOBUG_DECLARE_FLAG(oper); NOBUG_DECLARE_FLAG(test); NOBUG_DECLARE_FLAG(singleton); NOBUG_DECLARE_FLAG(assetmem); @@ -76,6 +77,7 @@ /* flags used throughout the code base... */ NOBUG_CPP_DEFINE_FLAG(config); + NOBUG_CPP_DEFINE_FLAG(oper); NOBUG_CPP_DEFINE_FLAG(test); NOBUG_CPP_DEFINE_FLAG_LIMIT(singleton, LOG_WARNING); NOBUG_CPP_DEFINE_FLAG_LIMIT(assetmem, LOG_WARNING); diff --git a/src/proc/asset.cpp b/src/proc/asset.cpp index ec75d84b3..901f21d4e 100644 --- a/src/proc/asset.cpp +++ b/src/proc/asset.cpp @@ -108,16 +108,21 @@ namespace asset other->unlink (this->id); } - /** release all links to other Asset objects held internally. */ + /** release all links to other dependant + * asset objects held internally and advise all parent + * assets to do so with the link to this asset. + * @note we don't release upward links to parent assets, + * thus effectively keeping the parents alive, because + * frequently these accessible parent assets are part + * of our own contract. (e.g. media for clip assets) + */ void Asset::unlink () { - function unregister_me = bind(&Asset::unregister, this,_1); + function forget_me = bind(&Asset::unregister, this,_1); - for_each (parents, unregister_me); - for_each (dependants, unregister_me); + for_each (parents, forget_me); dependants.clear(); - parents.clear(); } /** variant dropping only the links to the given Asset */ diff --git a/src/proc/asset.hpp b/src/proc/asset.hpp index 0c304a5de..11f18c85a 100644 --- a/src/proc/asset.hpp +++ b/src/proc/asset.hpp @@ -101,6 +101,7 @@ namespace asset operator size_t() const { return hash; } }; + class DB; class Asset; class AssetManager; typedef const ID& IDA; @@ -246,6 +247,7 @@ namespace asset virtual void defineDependency (PAsset parent); friend class AssetManager; + friend class DB; private: void unregister (PAsset& other); diff --git a/src/proc/asset/db.hpp b/src/proc/asset/db.hpp index d2fd0d02f..78352680a 100644 --- a/src/proc/asset/db.hpp +++ b/src/proc/asset/db.hpp @@ -26,6 +26,7 @@ #include "proc/asset.hpp" +#include "common/error.hpp" #include #include @@ -83,8 +84,8 @@ namespace asset { IdHashtable table; - DB () : table() {} - ~DB () {} + DB () : table() { } + ~DB () {clear();} friend class cinelerra::singleton::StaticCreate; @@ -92,7 +93,8 @@ namespace asset public: template void put (ID hash, shared_ptr& ptr) { table[hash] = static_pointer_cast (ptr); } - void put (ID hash, PAsset& ptr) { table[hash] = ptr; } + void put (ID hash, PAsset& ptr) { table[hash] = ptr; } + bool del (ID hash) { return table.erase (hash); } template shared_ptr @@ -101,6 +103,30 @@ namespace asset return dynamic_pointer_cast (find (hash)); } + /** removes all registered assets and invokes + * Asset::unlink() on each to break cyclic dependencies + */ + void + clear () throw() + { + try + { + IdHashtable::const_iterator i = table.begin(); + IdHashtable::const_iterator e = table.end(); + for ( ; i!=e ; ++i ) + i->second->unlink(); + + table.clear(); + } + catch (cinelerra::Error& EX) + { + WARN (oper, "Problems while clearing Asset registry: %s", EX.what()); + } + catch (...) + { + ERROR (oper, "Serious trouble while clearing Asset registry."); + } } + /** intended for diagnostics */ void diff --git a/src/proc/assetmanager.cpp b/src/proc/assetmanager.cpp index 5fa4183f2..617d45f46 100644 --- a/src/proc/assetmanager.cpp +++ b/src/proc/assetmanager.cpp @@ -34,6 +34,7 @@ #include using std::tr1::static_pointer_cast; +using boost::function; using boost::format; using boost::bind; //using boost::lambda::_1; @@ -186,25 +187,28 @@ namespace asset void - AssetManager::detach_child (PAsset& pA, IDA id) + recursive_call (AssetManager* instance, PAsset& pA) + { + instance->remove (pA->getID()); + } + + function + detach_child_recursively () ///< @return a functor recursively invoking remove(child) { - pA->unlink(id); + return bind( &recursive_call, &AssetManager::instance(), _1 ); } /** - * remove the given asset together with all its dependants from the internal DB + * remove the given asset from the internal DB + * together with all its dependants */ void AssetManager::remove (IDA id) - throw(cinelerra::error::Invalid, - cinelerra::error::State) { - UNIMPLEMENTED ("remove Asset with all dependecies"); - - PAsset pA = getAsset (id); - vector par = pA->getParents(); - boost::function func = bind(&detach_child, _1,id ); - for_each (par, func); // ,boost::lambda::var(id))); + PAsset asset = getAsset (id); + for_each (asset->dependants, detach_child_recursively()); + asset->unlink(); + registry.del(id); } diff --git a/src/proc/assetmanager.hpp b/src/proc/assetmanager.hpp index a8500ea0b..01e21447c 100644 --- a/src/proc/assetmanager.hpp +++ b/src/proc/assetmanager.hpp @@ -88,8 +88,7 @@ namespace asset /**remove the given asset from the internal DB. * together with all its dependants */ - void remove (IDA id) throw(cinelerra::error::Invalid, - cinelerra::error::State); + void remove (IDA id) ; /** extract a sorted list of all registered Assets */ list listContent() const; @@ -104,7 +103,7 @@ namespace asset static ID reg (KIND* obj, const Asset::Ident& idi) throw(cinelerra::error::Invalid); - /** deleter function used by the Asset smart pointers to delet Asset objects */ + /** deleter function used by the Asset smart pointers to delete Asset objects */ static void destroy (Asset* aa) { delete aa; } friend Asset::Asset (const Asset::Ident& idi); @@ -113,9 +112,6 @@ namespace asset friend class cinelerra::singleton::StaticCreate; - - private: - static void detach_child (PAsset&, IDA); }; diff --git a/tests/components/proc/asset/dependantassetstest.cpp b/tests/components/proc/asset/dependantassetstest.cpp index 82265a168..b76a209e8 100644 --- a/tests/components/proc/asset/dependantassetstest.cpp +++ b/tests/components/proc/asset/dependantassetstest.cpp @@ -92,25 +92,29 @@ namespace asset PAsset a1 (a1_); PTestA a2_ = TA::create(a1); PAsset a2 (a2_); + PAsset a3 = TA::create(a2); ASSERT (a1 == a2->getParents()[0]); ASSERT (a2 == a1->getDependant()[0]); + ASSERT (a2 == a3->getParents()[0]); + ASSERT (a3 == a2->getDependant()[0]); a2_->call_unlink(); - ASSERT (isnil (a2->getParents())); ASSERT (isnil (a2->getDependant())); - ASSERT (!contains (a1->getDependant(), a2)); // has been propagated + ASSERT (!contains (a1->getDependant(), a2)); // has been propagated up + ASSERT (!isnil (a2->getParents())); + ASSERT (contains (a3->getParents(), a2)); // but up-links remain intact a2_->set_depend(a1); - PAsset a3 = TA::create(a1); + PAsset a4 = TA::create(a1); ASSERT (a1 == a2->getParents()[0]); - ASSERT (a1 == a3->getParents()[0]); + ASSERT (a1 == a4->getParents()[0]); ASSERT (a2 == a1->getDependant()[0]); - ASSERT (a3 == a1->getDependant()[1]); + ASSERT (a4 == a1->getDependant()[1]); - a1_->call_unlink(a3->getID()); - ASSERT (!contains (a1->getDependant(), a3)); // selectively removed + a1_->call_unlink(a4->getID()); + ASSERT (!contains (a1->getDependant(), a4)); // selectively removed ASSERT ( contains (a1->getDependant(), a2)); - ASSERT (a1 == a3->getParents()[0]); // no propagation + ASSERT (a1 == a4->getParents()[0]); // no propagation } diff --git a/wiki/renderengine.html b/wiki/renderengine.html index 83be72225..3ffb052d6 100644 --- a/wiki/renderengine.html +++ b/wiki/renderengine.html @@ -1004,7 +1004,7 @@ For this Cinelerra3 design, we could consider making GOP just another raw media &rarr;see in [[Wikipedia|http://en.wikipedia.org/wiki/Group_of_pictures]] -
+
This wiki page is the entry point to detail notes covering some technical decisions, details and problems encountered in the course of the implementation of the Cinelerra Renderengine, the Builder and the related parts.
 
 * [[Packages, Interfaces and Namespaces|InterfaceNamespaces]]
@@ -1014,7 +1014,19 @@ For this Cinelerra3 design, we could consider making GOP just another raw media
 * [[Multichannel Media|MultichannelMedia]]
 * [[Editing Operations|EditingOperations]]
 * [[Handling of the current Session|CurrentSession]]
+* [[collecting Ideas for Implementation Guidelines|ImplementationGuidelines]]
 
+
+
+
+
!Observations, Ideas, Proposals
+''this page is a scrapbook for collecting ideas'' &mdash; please don't take anything noted here too literal. While writing code, I observe that I (ichthyo) follow certain informal guidelines, some of which I'd like to note down because they could evolve into general style guidelines for the proc layer code.
+* avoid doing anything non-local during the startup phase or shutdown phase of the application. consequently, allways prefer using an explicit cinelerra::Singleton<T> over using an static instance directly, thus yielding better control when the ctor and dtor will be invoked.
+* write error handling code only if the error situation can be actually //handled// at this place. Otherwise, be prepared for exceptions just passing by and thus handle any resources by "resource acquisition is initialisation". Remember: error handling defeats decoupling and encapsulation
+* (almost) never {{{delete}}} an object directly, use {{{new}}} only when some smart pointer is at hand.
+* when user/client code is inteded to create objects, make the ctor protected and provide a factory member called {{{create}}} instead, returning a smart pointer
+* avoid asuming anything that can't be enforced by types, interfaces or signatures; this means: be prepared for open possibilities
+* prefer {{{const}}} and initialisation code over assignment and active changes (inspired by functional programming)
@@ -1411,21 +1423,23 @@ This Design strives to achieve a StrongSeparation between the low-level Structur [[Admin]] <<fullscreen>>
-
+
Problem is: when removing an Asset, all corresponding MObjects need to disappear. This means, besides the obvious ~Ref-Link (MObject referring to an asset) we need backlinks or a sort of registry. And still worse: we need to remove the affected MObject from the object network in the EDL and rebuild the Fixture...
 
 &rarr; for a general design discussion see [[Relation of Clip and Asset|RelationClipAsset]]
 
-As a //first shot// Ichthyo considers the following approach:
+//Currently// Ichthyo considers the following approach:
 * all references between MObjects and Assets are implemented as __refcounting__ boost::shared_ptr
 * the opposite direction is also a __strong reference__, effectively keeping the clip-MO alive even if it is no longer in use in the session (note this is a cyclic dependency that needs to be actively broken on deletion). This design decision is based on  logical considerations (&rarr; see "deletions, Model-2" [[here|RelationClipAsset]]). This back-link is implemented by a Placement which is stored internally within the asset::Clip, it is needed for clean deletion of Assets, for GUI search functions and for adding the Clip to the EDL (again after been removed, or multiple times as if cloned).
-* MObjects and Assets implement an {{{unlink()}}} function releasing the internal links to other entities.
-* Instead of a delete, we call this unlink() function and let the shared_ptr handle the actual deletion.
+* MObjects and Assets implement an {{{unlink()}}} function releasing any internal links causing circular dependencies. It is always implemented such as to drop //optional// associations while retaining those associations mandatory for fulfilling the objects contract.
+* Instead of a delete, we call this unlink() function and let the shared_ptr handle the actual deletion. Thus, even if the object is already unlinked, it is still valid and usable as long as some other entity holds a smart-ptr. An ongoing render process for example can still use a clip asset and the corresponding media asset linked as parent, but this media asset's link to the dependant clip has already been cleared (and the media is no longer registered with the AssetManager of course).
+* so the back-link from dependant asset up to the parent asset is mandatory for the child asset to be functional, but preferably it should be {{{const}}} (only used for information retrieval)
+* the whole hierarchy has to be crafted accordingly, but this isn't much of a limitation
 * we don't use a registry, rather we model the real dependencies by individual dependency links. So a MediaAsset gets links to all Clips created from this Asset and by traversing this tree, we can handle the deletion
 * after the deletion, the Fixture needs to be rebuilt.
 * but any render processes still can have pointers to the Asset to be removed, and the shared_ptr will ensure, that the referred objects stay alive as long as needed.
 
-{{red{to be considered in more detail later}}}
+{{red{let's see if this approach works...}}}