code review

Original commit message from CVS:
code review

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.
  */

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 */
 };

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);

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
  */

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))

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,                                    \

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));
 

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 */
 };