memory: check semantics of nested mappings

Count how many mappings are currently active and also with what access pattern.
Update the design doc with restrictions on the access patterns for nested
mappings.
Check if nested mappings obey the access mode restrictions of the design doc.
Add various unit tests to check the desired behaviour.

diff --git a/docs/design/part-memory.txt b/docs/design/part-memory.txt
index af12090..981616c 100644 (file)
--- a/docs/design/part-memory.txt
+++ b/docs/design/part-memory.txt
@@ -115,7 +115,9 @@ Data Access
  performed. The call will update the new memory size with the specified size.
 
  It is allowed to map multiple times with different access modes. for each of
- the map calls, an corresponding unmap call needs to be made.
+ the map calls, an corresponding unmap call needs to be made. WRITE-only memory
+ cannot be mapped in READ mode and READ-only memory cannot be mapped in WRITE
+ mode.
  
  The memory pointer returned from the map call is guaranteed to remain valid in
  the requested mapping mode until the corresponding unmap call is performed on
@@ -127,7 +129,8 @@ Data Access
  When the final reference on a memory object is dropped, all outstanding
  mappings are automatically unmapped.
 
- Resizing a GstMemory does not influence any current mappings an any way.
+ Resizing a GstMemory does not influence any current mappings an any way. Note
+ however that the unmap call can resize the buffer again.
 
 
 Copy

diff --git a/gst/gstmemory.c b/gst/gstmemory.c
index a44da54..c63588d 100644 (file)
--- a/gst/gstmemory.c
+++ b/gst/gstmemory.c
@@ -123,6 +123,7 @@ _default_mem_init (GstMemoryDefault * mem, GstMemoryFlags flags,
   mem->mem.flags = flags;
   mem->mem.refcount = 1;
   mem->mem.parent = parent ? gst_memory_ref (parent) : NULL;
+  mem->mem.state = 0;
   mem->slice_size = slice_size;
   mem->data = data;
   mem->free_func = free_func;
@@ -220,6 +221,10 @@ _default_mem_map (GstMemoryDefault * mem, gsize * size, gsize * maxsize,
 static gboolean
 _default_mem_unmap (GstMemoryDefault * mem, gpointer data, gsize size)
 {
+  GST_DEBUG ("mem: %p, data %p, size %u", mem, data, size);
+  GST_DEBUG ("mem: %p, data %p,  offset %u, size %u, maxsize %u", mem,
+      mem->data, mem->offset, mem->size, mem->maxsize);
+
   g_return_val_if_fail ((guint8 *) data >= mem->data
       && (guint8 *) data < mem->data + mem->maxsize, FALSE);
 
@@ -477,11 +482,43 @@ gpointer
 gst_memory_map (GstMemory * mem, gsize * size, gsize * maxsize,
     GstMapFlags flags)
 {
-  g_return_val_if_fail (mem != NULL, NULL);
-  g_return_val_if_fail (!(flags & GST_MAP_WRITE) ||
-      GST_MEMORY_IS_WRITABLE (mem), NULL);
+  gpointer res;
+  gint access_mode, state, newstate;
 
-  return mem->allocator->info.map (mem, size, maxsize, flags);
+  g_return_val_if_fail (mem != NULL, NULL);
+  access_mode = flags & 3;
+  g_return_val_if_fail (!(access_mode & GST_MAP_WRITE)
+      || GST_MEMORY_IS_WRITABLE (mem), NULL);
+
+  do {
+    state = g_atomic_int_get (&mem->state);
+    if (state == 0) {
+      /* nothing mapped, set access_mode and refcount */
+      newstate = 4 | access_mode;
+    } else {
+      /* access_mode must match */
+      g_return_val_if_fail ((state & access_mode) == access_mode, NULL);
+      /* increase refcount */
+      newstate = state + 4;
+    }
+  } while (!g_atomic_int_compare_and_exchange (&mem->state, state, newstate));
+
+  res = mem->allocator->info.map (mem, size, maxsize, flags);
+
+  if (G_UNLIKELY (res == NULL)) {
+    /* something went wrong, restore the orginal state again */
+    do {
+      state = g_atomic_int_get (&mem->state);
+      /* there must be a ref */
+      g_return_val_if_fail (state >= 4, NULL);
+      /* decrease the refcount */
+      newstate = state - 4;
+      /* last refcount, unset access_mode */
+      if (newstate < 4)
+        newstate = 0;
+    } while (!g_atomic_int_compare_and_exchange (&mem->state, state, newstate));
+  }
+  return res;
 }
 
 /**
@@ -502,9 +539,31 @@ gst_memory_map (GstMemory * mem, gsize * size, gsize * maxsize,
 gboolean
 gst_memory_unmap (GstMemory * mem, gpointer data, gssize size)
 {
+  gboolean need_unmap = TRUE;
+  gint state, newstate;
+
   g_return_val_if_fail (mem != NULL, FALSE);
 
-  return mem->allocator->info.unmap (mem, data, size);
+  do {
+    state = g_atomic_int_get (&mem->state);
+
+    /* there must be a ref */
+    g_return_val_if_fail (state >= 4, FALSE);
+
+    if (need_unmap) {
+      /* try to unmap, only do this once */
+      if (!mem->allocator->info.unmap (mem, data, size))
+        return FALSE;
+      need_unmap = FALSE;
+    }
+    /* success, try to decrease the refcount */
+    newstate = state - 4;
+    /* last refcount, unset access_mode */
+    if (newstate < 4)
+      newstate = 0;
+  } while (!g_atomic_int_compare_and_exchange (&mem->state, state, newstate));
+
+  return TRUE;
 }
 
 /**

diff --git a/gst/gstmemory.h b/gst/gstmemory.h
index eb54f2c..0268da1 100644 (file)
--- a/gst/gstmemory.h
+++ b/gst/gstmemory.h
@@ -71,6 +71,7 @@ typedef enum {
  * @flags: memory flags
  * @refcount: refcount
  * @parent: parent memory block
+ * @state: private state
  *
  * Base structure for memory implementations. Custom memory will put this structure
  * as the first member of their structure.
@@ -81,6 +82,7 @@ struct _GstMemory {
   GstMemoryFlags  flags;
   gint            refcount;
   GstMemory      *parent;
+  volatile gint   state;
 };
 
 /**

diff --git a/tests/check/gst/gstmemory.c b/tests/check/gst/gstmemory.c
index d6861a3..1f9363f 100644 (file)
--- a/tests/check/gst/gstmemory.c
+++ b/tests/check/gst/gstmemory.c
@@ -463,7 +463,7 @@ GST_START_TEST (test_map_nested)
   fail_unless (data1 != NULL);
   fail_unless (size1 == 100);
 
-  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE);
+  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ);
   fail_unless (data2 == data1);
   fail_unless (size2 == 100);
 
@@ -471,6 +471,36 @@ GST_START_TEST (test_map_nested)
   gst_memory_unmap (mem, data1, size1);
   gst_memory_unmap (mem, data2, size2);
 
+  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));
+  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ);
+  gst_memory_unmap (mem, data1, size1);
+  gst_memory_unmap (mem, data2, size2);
+  fail_unless (mem->state == 0);
+
+  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));
+  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE);
+  gst_memory_unmap (mem, data1, size1);
+  gst_memory_unmap (mem, data2, size2);
+  /* nothing was mapped */
+  ASSERT_CRITICAL (gst_memory_unmap (mem, data2, size2));
+
+  data1 = gst_memory_map (mem, &size1, &maxsize1, GST_MAP_READWRITE);
+  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_READ);
+  gst_memory_unmap (mem, data2, size2);
+  data2 = gst_memory_map (mem, &size2, &maxsize2, GST_MAP_WRITE);
+  gst_memory_unmap (mem, data2, size2);
+  /* ut of range */
+  ASSERT_CRITICAL (gst_memory_unmap (mem, (guint8 *) data1 - 1, size1));
+  gst_memory_unmap (mem, data1, size1);
+  /* nothing was mapped */
+  ASSERT_CRITICAL (gst_memory_unmap (mem, data1, size1));
+
   gst_memory_unref (mem);
 }