Code Review =========== File: gst/gstbin.c Revision: 1.41 Date: Dec 16, 2000 Reviewer: Erik Walthinsen <omega@cse.ogi.edu> ----- 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. ----- Line 226: (gst_bin_change_state) // g_return_val_if_fail(bin->numchildren != 0, GST_STATE_FAILURE); Should this be uncommented? It makes sense that a bin can't change state unless it's got children, though we probably should check to see what state change is actually occuring before blindly failing. ----- Lines 241-243 (gst_bin_change_state) case GST_STATE_ASYNC: g_print("gstbin: child '%s' is changing state asynchronously\n", gst_element_get_name (child)); break; Obviously some work needs to be done on async state management. Probably need to have the Bin attach to one of the element's signals. This assumes that the element will get to run at some point in the future, or is capable of doing things in a separate context. ----- Lines 250-257: (gst_bin_change_state) if (GST_STATE_PENDING (element) == GST_STATE_READY) { GstObject *parent; parent = gst_object_get_parent (GST_OBJECT (element)); if (!parent || !GST_IS_BIN (parent)) gst_bin_create_plan (bin); } First of all, this code is run even if there is a failed or async reply from one of the child elements. Second problem is that it will call create_plan only in the NULL->READY and PLAYING->READY cases. If PAUSED is going to be a time for allowing changes to the pipeline, we need to update the plan somehow on PAUSED->PLAYING state. And calling create_plan in the PLAYING->READY case isn't actually that useful... ----- Line 263-271: (gst_bin_change_state_norecurse) . . . Is this really necessary? Are any users going to want to call this function? If not, then we can move the body of this function into gst_bin_change_state. ----- Line 273-305: (gst_bin_set_state_type) . . . Is this function ever going to be used by anything? I tend to think not... ----- Line 356-393: (gst_bin_get_by_name) . . . Should this be renamed to gst_bin_get_child_by_name() ? ----- Line 464-471: (gst_bin_use_cothreads) . . . This should be removed in favor of an argument or two? I think so.