From e40c2845726697d397e33086d3073e8293332ec3 Mon Sep 17 00:00:00 2001 From: Erik Walthinsen Date: Sun, 17 Dec 2000 06:26:30 +0000 Subject: [PATCH] README and the beginning of the first code-review. Here's the README: Original commit message from CVS: README and the beginning of the first code-review. Here's the README: 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. --- docs/code-reviews/README | 38 +++++++++++ docs/code-reviews/gstbin.c-1.41 | 117 ++++++++++++++++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 docs/code-reviews/README create mode 100644 docs/code-reviews/gstbin.c-1.41 diff --git a/docs/code-reviews/README b/docs/code-reviews/README new file mode 100644 index 0000000000..59f6b88c1f --- /dev/null +++ b/docs/code-reviews/README @@ -0,0 +1,38 @@ +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 new file mode 100644 index 0000000000..4757c32859 --- /dev/null +++ b/docs/code-reviews/gstbin.c-1.41 @@ -0,0 +1,117 @@ +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.