gstreamer/docs/code-reviews/gstbin.c-1.41

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.