WIP second attemptmemory-improvements2

Attempt to merge and atomically remember the merged memory so that we can use it
in unmap. Does not quite work because we can't safely unref the memory in unmap.

4 files changed, 119 insertions, 73 deletions

diff --git a/gst/gstbuffer.c b/gst/gstbuffer.c
index e0b64cf8e..6ca0e6cef 100644
--- a/gst/gstbuffer.c
+++ b/gst/gstbuffer.c
@@ -136,6 +136,8 @@ struct _GstMetaItem
 #define GST_BUFFER_MEM_ARRAY(b)    (((GstBufferImpl *)(b))->mem)
 #define GST_BUFFER_MEM_PTR(b,i)    (((GstBufferImpl *)(b))->mem[i])
 #define GST_BUFFER_BUFMEM(b)       (((GstBufferImpl *)(b))->bufmem)
+#define GST_BUFFER_MERGED(b)       (((GstBufferImpl *)(b))->merged)
+#define GST_BUFFER_MAPREF(b)       (((GstBufferImpl *)(b))->mapref)
 #define GST_BUFFER_META(b)         (((GstBufferImpl *)(b))->item)
 
 typedef struct
@@ -149,6 +151,10 @@ typedef struct
   /* memory of the buffer when allocated from 1 chunk */
   GstMemory *bufmem;
 
+  /* cache for last merged memory */
+  GstMemory *merged;
+  gint mapref;
+
   /* FIXME, make metadata allocation more efficient by using part of the
    * GstBufferImpl */
   GstMetaItem *item;
@@ -399,6 +405,9 @@ _gst_buffer_free (GstBuffer * buffer)
    * itself */
   msize = GST_MINI_OBJECT_SIZE (buffer);
 
+  /* unmap any pending mappings */
+  gst_buffer_unmap (buffer, NULL, 0);
+
   /* free our memory */
   len = GST_BUFFER_MEM_LEN (buffer);
   for (i = 0; i < len; i++)
@@ -431,6 +440,7 @@ gst_buffer_init (GstBufferImpl * buffer, gsize size)
   GST_BUFFER_OFFSET_END (buffer) = GST_BUFFER_OFFSET_NONE;
 
   GST_BUFFER_MEM_LEN (buffer) = 0;
+  GST_BUFFER_MERGED (buffer) = NULL;
   GST_BUFFER_META (buffer) = NULL;
 }
 
@@ -746,6 +756,42 @@ gst_buffer_remove_memory_range (GstBuffer * buffer, guint idx, guint length)
 }
 
 /**
+ * gst_buffer_get_merged_memory:
+ * @buffer: a #GstBuffer.
+ *
+ * Return a #GstMemory object that contains all the memory in @buffer. If there
+ * was only one memory in @buffer, it will be returned directly, otherwise all
+ * memory objects will be merged into one object that will be returned.
+ *
+ * Returns: a #GstMemory with the merged memory in @buffer. This function can
+ * return %NULL if there is no memory in @buffer. Use gst_memory_unref() after
+ * usage.
+ */
+GstMemory *
+gst_buffer_get_merged_memory (GstBuffer * buffer)
+{
+  guint len;
+  GstMemory *mem;
+
+  g_return_val_if_fail (GST_IS_BUFFER (buffer), NULL);
+
+  len = GST_BUFFER_MEM_LEN (buffer);
+
+  if (G_UNLIKELY (len == 0)) {
+    /* no memory */
+    mem = NULL;
+  } else if (G_LIKELY (len == 1)) {
+    /* we can take the first one */
+    mem = GST_BUFFER_MEM_PTR (buffer, 0);
+    gst_memory_ref (mem);
+  } else {
+    /* we need to span memory */
+    mem = _span_memory (buffer, 0, -1, FALSE);
+  }
+  return mem;
+}
+
+/**
  * gst_buffer_get_sizes:
  * @buffer: a #GstBuffer.
  * @offset: a pointer to the offset
@@ -917,8 +963,7 @@ gpointer
 gst_buffer_map (GstBuffer * buffer, gsize * size, gsize * maxsize,
     GstMapFlags flags)
 {
-  guint len;
-  gpointer data;
+  gpointer data = NULL;
   GstMemory *mem;
   gboolean write, writable;
 
@@ -927,64 +972,74 @@ gst_buffer_map (GstBuffer * buffer, gsize * size, gsize * maxsize,
   write = (flags & GST_MAP_WRITE) != 0;
   writable = gst_buffer_is_writable (buffer);
 
-  /* check if we can write when asked for write access */
-  if (G_UNLIKELY (write && !writable))
+  if (write && !writable)
     goto not_writable;
 
-  len = GST_BUFFER_MEM_LEN (buffer);
+  g_atomic_int_inc (&GST_BUFFER_MAPREF (buffer));
 
-  if (G_UNLIKELY (len == 0)) {
-    /* no memory, return immediately */
-    if (size)
-      *size = 0;
-    if (maxsize)
-      *maxsize = 0;
-    return NULL;
-  }
+  do {
+    /* try to use last merged copy */
+    mem = g_atomic_pointer_get (&GST_BUFFER_MERGED (buffer));
 
-  if (G_LIKELY (len == 1)) {
-    /* we can take the first one */
-    mem = GST_BUFFER_MEM_PTR (buffer, 0);
-  } else {
-    /* we need to span memory */
-    if (writable) {
-      /* if we can write, we can change the memory with the spanned
-       * memory */
-      mem = _span_memory (buffer, 0, -1, write);
-      _replace_memory (buffer, mem);
-    } else {
-      gsize bsize;
-
-      /* extract all data in new memory, FIXME slow!! */
-      bsize = gst_buffer_get_size (buffer);
-
-      data = g_malloc (bsize);
-      gst_buffer_extract (buffer, 0, data, bsize);
-      if (size)
-        *size = bsize;
-      if (maxsize)
-        *maxsize = bsize;
-      return data;
+    /* if we have a merged copy, use that */
+    if (mem != NULL)
+      break;
+
+    /* else make a new merged copy */
+    mem = gst_buffer_get_merged_memory (buffer);
+    if (G_UNLIKELY (mem == NULL))
+      goto no_memory;
+
+    /* now try to map */
+    data = gst_memory_map (mem, size, maxsize, flags);
+    if (data == NULL && write) {
+      GstMemory *tmp;
+      /* make a (writable) copy */
+      tmp = gst_memory_copy (mem, 0, -1);
+      gst_memory_unref (mem);
+      mem = tmp;
+      if (G_UNLIKELY (mem == NULL))
+        goto cannot_map;
     }
-  }
+    /* try to swap in the new memory */
+    if (g_atomic_pointer_compare_and_exchange (&GST_BUFFER_MERGED (buffer),
+            NULL, mem))
+      break;
 
-  if (G_UNLIKELY (write && !gst_memory_is_writable (mem))) {
-    GstMemory *copy;
-    /* replace with a writable copy */
-    copy = gst_memory_copy (mem, 0, -1);
-    GST_BUFFER_MEM_PTR (buffer, 0) = copy;
+    /* somebody added some merged memory, try again */
     gst_memory_unref (mem);
-    mem = copy;
-  }
+    data = NULL;
+  } while (1);
 
-  data = gst_memory_map (mem, size, maxsize, flags);
+  GST_DEBUG_OBJECT (buffer, "map %p", mem);
+  /* map if not already mapped */
+  if (data == NULL)
+    data = gst_memory_map (mem, size, maxsize, flags);
 
   return data;
 
-  /* ERROR */
+  /* ERRORS */
 not_writable:
   {
-    g_return_val_if_fail (gst_buffer_is_writable (buffer), NULL);
+    g_cirital ("write map requested on non-writable buffer");
+    goto error_exit;
+  }
+no_memory:
+  {
+    GST_DEBUG_OBJECT (buffer, "no memory");
+    goto error_exit;
+  }
+cannot_map:
+  {
+    GST_DEBUG_OBJECT (buffer, "cannot map memory");
+    goto error_exit;
+  }
+error_exit:
+  {
+    if (size)
+      *size = 0;
+    if (maxsize)
+      *maxsize = 0;
     return NULL;
   }
 }
@@ -1004,23 +1059,17 @@ not_writable:
 gboolean
 gst_buffer_unmap (GstBuffer * buffer, gpointer data, gssize size)
 {
-  guint len;
+  GstMemory *mem;
 
   g_return_val_if_fail (GST_IS_BUFFER (buffer), FALSE);
   g_return_val_if_fail (size >= -1, FALSE);
+  g_return_val_if_fail (GST_BUFFER_MAPREF (buffer) > 0, FALSE);
 
-  len = GST_BUFFER_MEM_LEN (buffer);
-
-  if (G_LIKELY (len == 1)) {
-    GstMemory *mem = GST_BUFFER_MEM_PTR (buffer, 0);
+  mem = g_atomic_pointer_get (&GST_BUFFER_MERGED (buffer));
+  g_return_val_if_fail (mem != NULL, FALSE);
+  GST_DEBUG_OBJECT (buffer, "unmap %p", mem);
+  gst_memory_unmap (mem);
 
-    gst_memory_unmap (mem);
-  } else {
-    /* this must have been from read-only access. After _map, the buffer either
-     * only contains 1 memory block or it allocated memory to join memory
-     * blocks. It's not allowed to add buffers between _map and _unmap. */
-    g_free (data);
-  }
   return TRUE;
 }
 
diff --git a/gst/gstbuffer.h b/gst/gstbuffer.h
index d52753292..a433cae8b 100644
--- a/gst/gstbuffer.h
+++ b/gst/gstbuffer.h
@@ -276,6 +276,8 @@ void        gst_buffer_take_memory         (GstBuffer *buffer, gint idx, GstMemo
 GstMemory * gst_buffer_peek_memory         (GstBuffer *buffer, guint idx, GstMapFlags flags);
 void        gst_buffer_remove_memory_range (GstBuffer *buffer, guint idx, guint length);
 
+GstMemory * gst_buffer_get_merged_memory   (GstBuffer *buffer);
+
 /**
  * gst_buffer_remove_memory:
  * @b: a #GstBuffer.
diff --git a/gst/gstmemory.c b/gst/gstmemory.c
index 5172e978a..7c22f8335 100644
--- a/gst/gstmemory.c
+++ b/gst/gstmemory.c
@@ -527,7 +527,7 @@ gst_memory_map (GstMemory * mem, gsize * size, gsize * maxsize,
   g_return_val_if_fail (mem != NULL, NULL);
 
   if (!gst_memory_lock (mem, flags))
-    goto lock_failed;
+    return NULL;
 
   res = mem->allocator->info.map (mem, mem->maxsize, flags);
 
@@ -542,11 +542,6 @@ gst_memory_map (GstMemory * mem, gsize * size, gsize * maxsize,
   return res + mem->offset;
 
   /* ERRORS */
-lock_failed:
-  {
-    g_critical ("memory %p: failed to lock memory", mem);
-    return NULL;
-  }
 error:
   {
     /* something went wrong, restore the orginal state again */
diff --git a/tests/check/gst/gstmemory.c b/tests/check/gst/gstmemory.c
index 5439c8966..283f79f33 100644
--- a/tests/check/gst/gstmemory.c
+++ b/tests/check/gst/gstmemory.c
@@ -148,7 +148,7 @@ GST_START_TEST (test_writable)
   /* create read-only memory and try to write */
   mem = create_read_only_memory ();
 
-  ASSERT_CRITICAL (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
+  fail_if (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
   fail_if (gst_memory_is_writable (mem));
 
   mem2 = gst_memory_copy (mem, 0, -1);
@@ -160,7 +160,7 @@ GST_START_TEST (test_writable)
   gst_memory_unmap (mem2);
 
   gst_memory_ref (mem2);
-  ASSERT_CRITICAL (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
+  fail_if (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
   gst_memory_unref (mem2);
 
   data = gst_memory_map (mem2, &size, NULL, GST_MAP_WRITE);
@@ -184,8 +184,8 @@ GST_START_TEST (test_submemory_writable)
   sub_mem = gst_memory_share (mem, 0, 8);
   fail_if (gst_memory_is_writable (sub_mem));
 
-  ASSERT_CRITICAL (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
-  ASSERT_CRITICAL (gst_memory_map (sub_mem, &size, NULL, GST_MAP_WRITE));
+  fail_if (gst_memory_map (mem, &size, NULL, GST_MAP_WRITE));
+  fail_if (gst_memory_map (sub_mem, &size, NULL, GST_MAP_WRITE));
 
   gst_memory_unref (sub_mem);
   gst_memory_unref (mem);
@@ -425,8 +425,8 @@ GST_START_TEST (test_map_nested)
 
   data1 = gst_memory_map (mem, &size1, &maxsize1, GST_MAP_READ);
   /* not allowed */
-  ASSERT_CRITICAL (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE));
-  ASSERT_CRITICAL (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READWRITE));
+  fail_if (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE));
+  fail_if (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READWRITE));
   data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ);
   gst_memory_unmap (mem);
   gst_memory_unmap (mem);
@@ -434,8 +434,8 @@ GST_START_TEST (test_map_nested)
 
   data1 = gst_memory_map (mem, &size1, &maxsize1, GST_MAP_WRITE);
   /* not allowed */
-  ASSERT_CRITICAL (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ));
-  ASSERT_CRITICAL (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READWRITE));
+  fail_if (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ));
+  fail_if (gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READWRITE));
   data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE);
   gst_memory_unmap (mem);
   gst_memory_unmap (mem);