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.