Code Review =========== File: gst/gstbin.c Revision: 1.41 Date: Dec 16, 2000 Reviewer: Erik Walthinsen ----- Line 20: #define GST_DEBUG_ENABLED Shouldn't be here, DEBUG should be enabled globally. May leave until scheduling changes are done. ----- Lines 24-25: #include "gstsrc.h" #include "gstconnection.h" These may go away entirely with scheduling changes complete. Differentiation is based on element's pads. ----- Lines 52-54: /* Bin signals and args */ enum { OBJECT_ADDED, Bin needs a lot more signals, there are a lot of events that may need to be trapped. ----- Line 117: (gst_bin_class_init) gstelement_class->elementfactory = gst_elementfactory_find("bin"); Not sure this is such a great idea. I thought the GstElement code did this kind of stuff? ----- Lines 127-128: (gst_bin_init) // FIXME temporary testing measure // bin->use_cothreads = TRUE; Need to work out how the cothreads force stuff works. need_cothreads is the private state that create_plan figures out, use_cothreads is the argument that the user sets. Since create_plan works out if cothreads are needed, and scheduling can't be done without them if they are really needed, perhaps we should rename use_cothreads to force_cothreads. If FALSE, create_plan decides. If TRUE, cothreads are forced on. Then again, there may be some random case where create_plan says to use cothreads when they're not strictly required. Not sure if we want to enable the user to force cothreads *off*. I suppose the user may know better, given specific plugins. Maybe a second argument force_cothreads_disable. ----- Lines 131-145: (gst_bin_new) . . . It seems to make sense to keep this function, since it's part of the main API and not a plugin, but if we come up with some better way of creating plugins and such, this may go away. It stays for now. ----- Lines 164-166: (gst_bin_add) // must be NULL or PAUSED state in order to modify bin g_return_if_fail ((GST_STATE (bin) == GST_STATE_NULL) || (GST_STATE (bin) == GST_STATE_PAUSED)); Live pipeline modification needs some serious thought, along with the rest of the potential state-transition cases. This check looks sane in the sense that you really shouldn't be in running state and change the bin contents, since create_plan won't be run. But we don't actually re-run create_plan (or maybe it should be update_plan, and we keep a log of the changes since last plan to optimize the update?), so if someone messes with the bin during PAUSED state, we're looking forward to major disaster. ----- Line 168: (gst_bin_add) bin->children = g_list_append (bin->children, element); Should we be appending or prepending? Append is slow, but it's a matter of whether you want to spend the time at setup or later, with setup being preferable. We'll walk the child list quite often during runtime, so the time spent putting the element in the right place up front is done only once. Then again, does the order of the elements in the child list really matter? ----- Lines 172-174: (gst_bin_add) /* we know we have at least one child, we just added one... */ // if (GST_STATE(element) < GST_STATE_READY) // gst_bin_change_state_norecurse(bin,GST_STATE_READY); This is another part of the state management issue. The behavior of Bins seems to be leaning away from this style, which is why it's commented out, but it's something to keep in mind when working on the state system. ----- Lines 204-206: (gst_bin_remove) /* if we're down to zero children, force state to NULL */ if (bin->numchildren == 0) gst_element_set_state (GST_ELEMENT (bin), GST_STATE_NULL); Also suspect from the state management perspective, except this one isn't commented out.