+++ /dev/null
-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.
projects / platform / upstream / gstreamer.git / commitdiff