code review

Original commit message from CVS:
code review

diff --git a/gst/gstbuffer.c b/gst/gstbuffer.c
index 0f5a836..ffbc59f 100644 (file)
--- a/gst/gstbuffer.c
+++ b/gst/gstbuffer.c
@@ -439,7 +439,8 @@ gst_buffer_copy (GstBuffer *buffer)
  * @buf1: first source buffer
  * @buf2: second source buffer
  *
- * Determines whether a gst_buffer_span is free, or requires a memcpy. 
+ * Determines whether a gst_buffer_span is free (as in free beer), 
+ * or requires a memcpy. 
  *
  * Returns: TRUE if the buffers are contiguous, FALSE if a copy would be required.
  */

diff --git a/gst/gstbuffer.h b/gst/gstbuffer.h
index 4e63557..10e7b6e 100644 (file)
--- a/gst/gstbuffer.h
+++ b/gst/gstbuffer.h
@@ -113,7 +113,7 @@ struct _GstBuffer {
 #endif
 
   /* flags */
-  guint16              flags;
+  guint16              flags; /* boolean properties of buffer */
 
   /* pointer to data, its size, and offset in original source if known */
   guchar               *data;
@@ -122,8 +122,8 @@ struct _GstBuffer {
   guint32              offset;
 
   /* timestamp */
-  gint64               timestamp;
-  gint64               maxage;
+  gint64               timestamp; /* nanoseconds since zero */
+  gint64               maxage;    /* FIXME: not used yet */
 
   /* subbuffer support, who's my parent? */
   GstBuffer            *parent;
@@ -132,7 +132,7 @@ struct _GstBuffer {
   GstBufferPool        *pool;
   gpointer             pool_private;
 
-  /* utility function pointers */
+  /* utility function pointers, can override default */
   GstBufferFreeFunc    free;           /* free the data associated with the buffer */
   GstBufferCopyFunc    copy;           /* copy the data from one buffer to another */
 };

diff --git a/gst/gstcaps.h b/gst/gstcaps.h
index 673741c..9cd98cd 100644 (file)
--- a/gst/gstcaps.h
+++ b/gst/gstcaps.h
@@ -45,18 +45,23 @@ extern GType _gst_caps_type;
 #define GST_CAPS_IS_FIXED(caps)                ((caps)->fixed)
 #define GST_CAPS_IS_CHAINED(caps)      ((caps)->next)
 
+/* CR1: id is an int corresponding to the quark for the mime type because
+ * it's really fast when doing a first-pass check for caps compatibility */
 struct _GstCaps {
   gchar        *name;                  /* the name of this caps */
-  guint16      id;                     /* type id (major type) */
+  guint16      id;                     /* type id (major type) representing 
+                                          the mime type */
 
   guint        refcount;               
   gboolean     fixed;                  /* this caps doesn't contain variable properties */
 
   GstProps     *properties;            /* properties for this capability */
 
-  GstCaps      *next;
+  GstCaps      *next;                  /* not with a GList for efficiency */
 };
 
+/* factory macros which make it easier for plugins to instantiate */
+
 #define GST_CAPS_NEW(name, type, a...)          \
 gst_caps_new (                                  \
   name,                                         \
@@ -92,7 +97,7 @@ void          gst_caps_destroy                        (GstCaps *caps);
 void           gst_caps_debug                          (GstCaps *caps, const gchar *label);
 
 GstCaps*       gst_caps_copy                           (GstCaps *caps);
-GstCaps*       gst_caps_copy_1                         (GstCaps *caps);
+GstCaps*       gst_caps_copy_first                     (GstCaps *caps);
 GstCaps*       gst_caps_copy_on_write                  (GstCaps *caps);
 
 const gchar*   gst_caps_get_name                       (GstCaps *caps);

diff --git a/gst/gstobject.c b/gst/gstobject.c
index 30ae7b9..8681914 100644 (file)
--- a/gst/gstobject.c
+++ b/gst/gstobject.c
@@ -109,6 +109,9 @@ gst_object_class_init (GstObjectClass *klass)
 
   gobject_class->set_property = GST_DEBUG_FUNCPTR (gst_object_set_property);
   gobject_class->get_property = GST_DEBUG_FUNCPTR (gst_object_get_property);
+
+  /* CR1: we override the signal property so that an object can propagate the
+   * signal to the parent object */
   gobject_class->dispatch_properties_changed = GST_DEBUG_FUNCPTR (gst_object_dispatch_properties_changed);
 
   g_object_class_install_property (G_OBJECT_CLASS (klass), ARG_NAME,
@@ -202,6 +205,7 @@ gst_object_unref (GstObject *object)
  * Removes floating reference on an object.  Any newly created object has
  * a refcount of 1 and is FLOATING.  This function should be used when
  * creating a new object to symbolically 'take ownership of' the object.
+ * Use #gst_object_set_parent to have this done for you.
  */
 void
 gst_object_sink (GstObject *object)
@@ -222,6 +226,7 @@ gst_object_sink (GstObject *object)
  * @object: GstObject to destroy
  *
  * Destroy the object.
+ * 
  */
 void
 gst_object_destroy (GstObject *object)
@@ -330,7 +335,8 @@ gst_object_set_name (GstObject *object, const gchar *name)
 const gchar*
 gst_object_get_name (GstObject *object)
 {
-  g_return_val_if_fail (object != NULL, NULL);
+  /* CR1: GLib checks for NULL */
+  //FIXME: _REDUNDANT g_return_val_if_fail (object != NULL, NULL);
   g_return_val_if_fail (GST_IS_OBJECT (object), NULL);
 
   return object->name;
@@ -503,9 +509,9 @@ gst_object_check_uniqueness (GList *list, const gchar *name)
   while (list) {
     GstObject *child = GST_OBJECT (list->data);
 
-    list = g_list_next(list);
+    list = g_list_next (list);
       
-    if (strcmp(GST_OBJECT_NAME(child), name) == 0) 
+    if (strcmp (GST_OBJECT_NAME (child), name) == 0) 
       return FALSE;
   }
 
@@ -615,6 +621,10 @@ gst_object_get_property (GObject* object, guint prop_id,
       break;
   }
 }
+
+/* CR1: the GObject changing a property emits signals to it's parents
+ * so that the app can connect a listener to the top-level bin */
+
 static void
 gst_object_dispatch_properties_changed (GObject     *object,
                                      guint        n_pspecs,
@@ -644,7 +654,7 @@ gst_object_dispatch_properties_changed (GObject     *object,
  * @object: GstObject to get the path from
  *
  * Generates a string describing the path of the object in
- * the object hierarchy. Usefull for debugging
+ * the object hierarchy. Only useful (or used) for debugging
  *
  * Returns: a string describing the path of the object
  */

diff --git a/gst/gstobject.h b/gst/gstobject.h
index 1e168c4..9dbf9b8 100644 (file)
--- a/gst/gstobject.h
+++ b/gst/gstobject.h
@@ -90,6 +90,7 @@ struct _GstObject {
   guint32      flags;
 };
 
+/* signal_object is used to signal to the whole class */
 struct _GstObjectClass {
   GObjectClass parent_class;
 
@@ -123,7 +124,7 @@ struct _GstObjectClass {
 #define GST_OBJECT_DESTROYED(obj)      (GST_FLAG_IS_SET (obj, GST_DESTROYED))
 #define GST_OBJECT_FLOATING(obj)       (GST_FLAG_IS_SET (obj, GST_FLOATING))
 
-/* object locking */
+/* CR1: object locking - GObject 2.0 doesn't have threadsafe locking */
 #define GST_LOCK(obj)          (g_mutex_lock(GST_OBJECT_CAST(obj)->lock))
 #define GST_TRYLOCK(obj)       (g_mutex_trylock(GST_OBJECT_CAST(obj)->lock))
 #define GST_UNLOCK(obj)                (g_mutex_unlock(GST_OBJECT_CAST(obj)->lock))

diff --git a/gst/gstpad.h b/gst/gstpad.h
index 136cb8b..c1331f0 100644 (file)
--- a/gst/gstpad.h
+++ b/gst/gstpad.h
@@ -178,6 +178,7 @@ struct _GstRealPad {
   GstRealPad                   *peer;
 
   GstBuffer                    *bufpen;
+  //CR1: FIXME: regiontype should go away
   GstRegionType                regiontype;
   guint64                      offset;
   guint64                      len;
@@ -311,6 +312,7 @@ struct _GstPadTemplateClass {
   void (*pad_created)  (GstPadTemplate *templ, GstPad *pad);
 };
 
+/* CR1: the space after 'a' is necessary because of preprocessing in gcc */
 #define GST_PAD_TEMPLATE_NEW(padname, dir, pres, a...) \
   gst_pad_template_new (                         \
     padname,                                    \

diff --git a/gst/gstthread.c b/gst/gstthread.c
index 465552b..2fc7039 100644 (file)
--- a/gst/gstthread.c
+++ b/gst/gstthread.c
@@ -139,6 +139,7 @@ gst_thread_init (GstThread *thread)
   GST_DEBUG (GST_CAT_THREAD, "initializing thread");
 
   /* we're a manager by default */
+  /* CR1: the GstBin code checks these flags */
   GST_FLAG_SET (thread, GST_BIN_FLAG_MANAGER);
   GST_FLAG_SET (thread, GST_BIN_SELF_SCHEDULABLE);
 
@@ -434,7 +435,7 @@ gst_thread_change_state (GstElement * element)
  * @arg: the thread to start
  *
  * The main loop of the thread. The thread will iterate
- * while the state is GST_THREAD_STATE_SPINNING
+ * while the state is GST_THREAD_STATE_SPINNING.
  */
 static void *
 gst_thread_main_loop (void *arg)
@@ -467,6 +468,8 @@ gst_thread_main_loop (void *arg)
 
   /***** THREAD IS NOW IN READY STATE *****/
 
+  /* CR1: most of this code is handshaking */
+  /* do this while the thread lives */
   while (!GST_FLAG_IS_SET (thread, GST_THREAD_STATE_REAPING)) {
     /* NOTE we hold the thread lock at this point */
     /* what we do depends on what state we're in */
@@ -503,7 +506,7 @@ gst_thread_main_loop (void *arg)
                        gst_element_statename (GST_STATE_PAUSED),
                        gst_element_statename (GST_STATE_READY),
                        gst_element_statename (GST_STATE_PLAYING));
-        g_cond_wait (thread->cond,thread->lock);
+        g_cond_wait (thread->cond, thread->lock);
 
        /* this must have happened by a state change in the thread context */
        if (GST_STATE_PENDING (thread) != GST_STATE_READY &&
@@ -568,7 +571,7 @@ gst_thread_main_loop (void *arg)
         break;
     }
   }
-  /* we need to destroy the scheduler here bacause it has mapped it's
+  /* we need to destroy the scheduler here because it has mapped it's
    * stack into the threads stack space */
   gst_scheduler_reset (GST_ELEMENT_SCHED (thread));
 

diff --git a/gst/gstthread.h b/gst/gstthread.h
index def68e3..1e9da8e 100644 (file)
--- a/gst/gstthread.h
+++ b/gst/gstthread.h
@@ -67,8 +67,8 @@ struct _GstThread {
   pthread_t thread_id;         /* id of the thread, if any */
   gint pid;                    /* the pid of the thread */
   gint ppid;                   /* the pid of the thread's parent process */
-  GMutex *lock;                        /* thread lock/condititon pair... */
-  GCond *cond;                 /* used to control the thread */
+  GMutex *lock;                        /* thread lock/condititon pair ... */
+  GCond *cond;                 /* .... used to control the thread */
 
   gint transition;             /* the current state transition */
 };