mirror of
https://gitlab.freedesktop.org/gstreamer/gstreamer.git
synced 2024-09-09 05:38:58 +00:00
Original commit message from CVS: Re-arranged the build a bit to try to make it more sane. Added some debug.
182 lines
5.7 KiB
Text
182 lines
5.7 KiB
Text
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.
|