[Bug 691299] API: GstFileMemAllocator - an allocator that uses disk storage to provide memory space

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=691299
  GStreamer | gst-plugins-base | git

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #250246|none                        |needs-work
             status|                            |

--- Comment #10 from Sebastian Dröge (slomo) <slomo at circular-chaos.org> 2013-07-28 15:56:44 UTC ---
Review of attachment 250246:
 --> (https://bugzilla.gnome.org/review?bug=691299&attachment=250246)

::: gst-libs/gst/allocators/gstmappedfile.c
@@ +162,3 @@
+      maxsize);
+
+  if (allocator->f_offset_next + maxsize > allocator->file_size) {

I think you should implement a better block allocation strategy, that allows to
properly deallocate and reallocate blocks

@@ +207,3 @@
+     of blocks available or more advanced if needed. */
+
+#if defined (HAVE_FALLOCATE) && HAVE_DECL_FALLOC_FL_PUNCH_HOLE

So if this is not supported, the memory will slowly run full and then nothing
can be allocated anymore? I think the allocator should not be supported at all
then

@@ +256,3 @@
+  /* Can't really control the alignment
+     Can we do something about the data offset (GstAllocationParams)? Is it
+     the right place to handle it? */

No the alignment and padding and everything should be handled during allocation

@@ +503,3 @@
+  GST_ERROR ("%s allocator is not supported on this platform",
+      GST_ALLOCATOR_MAPPEDFILE);
+#endif // GST_MAPPEDFILEALLOCATOR_SUPPORTED

No C99 comments please

@@ +532,3 @@
+
+  gst_allocator_register (GST_ALLOCATOR_MAPPEDFILE,
+      GST_ALLOCATOR_CAST (allocator));

Not sure if registering the allocator globally with a fixed temp template makes
sense. What if multiple such allocators with different temp templates should be
used?

::: tests/check/libs/allocators.c
@@ +29,3 @@
+  GstAllocator *alloc = NULL;
+
+  gst_mappedfile_allocator_init (size, "/tmp/gstmappedfile-XXXXXX");

Use g_get_tmp_dir() here

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.