mirror of
https://gitlab.freedesktop.org/gstreamer/gstreamer.git
synced 2025-04-02 14:20:06 +00:00
code-reviews: remove obsolete code reviews
This obsolete folder hasn't been touched since 2001 and has no purpose. It confuses new developers.
This commit is contained in:
Luis de Bethencourt
parent
8bc0a6c562
commit
8115dd07ae
2 changed files with 0 additions and 220 deletions
|
|
@ -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 <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.
|
||||
|
||||
|
||||
-----
|
||||
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.
|
||||
|
|
@ -1,182 +0,0 @@
|
|||
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.
|
||||
Loading…
Reference in a new issue