diff --git a/docs/code-reviews/README b/docs/code-reviews/README deleted file mode 100644 index 59f6b88c1f..0000000000 --- a/docs/code-reviews/README +++ /dev/null @@ -1,38 +0,0 @@ -Code reviews: -============= - -Files are to be named by file or subsystem, and CVS revision number or date: - -gstbin.c-1.41 -editor-20001216 - -A file should look something like the following: - ----------------------------------------------------------------------- -Code Review -=========== -File: gst/gstbin.c -Revision: 1.41 -Date: Dec 16, 2000 -Reviewer: Erik Walthinsen - - ------ -Line 20: -#define GST_DEBUG_ENABLED - -Shouldn't be here, DEBUG should be enabled globally. May leave until -scheduling changes are done. - - ------ -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? - ----------------------------------------------------------------------- - -The format will evolve as we do more stuff, such as putting in fields for -recommended actions, comments regarding any later changes made and when, etc. diff --git a/docs/code-reviews/gstbin.c-1.41 b/docs/code-reviews/gstbin.c-1.41 deleted file mode 100644 index e3d69e0e77..0000000000 --- a/docs/code-reviews/gstbin.c-1.41 +++ /dev/null @@ -1,182 +0,0 @@ -Code Review -=========== -File: gst/gstbin.c -Revision: 1.41 -Date: Dec 16, 2000 -Reviewer: Erik Walthinsen - - - ------ -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.