After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 782556 - decklinkvideosrc: Implement a custom memory allocator
decklinkvideosrc: Implement a custom memory allocator
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-05-12 13:06 UTC by Georg Lippitsch
Modified: 2017-07-11 11:27 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Custom memory allocator for decklinkvideosrc (5.03 KB, patch)
2017-05-12 13:06 UTC, Georg Lippitsch
none Details | Review
Custom memory allocator for decklinkvideosrc (4.58 KB, patch)
2017-06-30 14:07 UTC, Georg Lippitsch
none Details | Review
Custom memory allocator for decklinkvideosrc (4.46 KB, patch)
2017-07-07 13:41 UTC, Georg Lippitsch
none Details | Review
Custom memory allocator for decklinkvideosrc (4.41 KB, patch)
2017-07-10 08:07 UTC, Georg Lippitsch
committed Details | Review

Description Georg Lippitsch 2017-05-12 13:06:15 UTC
Created attachment 351714 [details] [review]
Custom memory allocator for decklinkvideosrc

The default memory allocator of the decklink library allocates
a fixed pool of buffers, and the number of buffers is unknown.
This makes it impossible do useful queuing downstream. The new
memory allocator can create an unlimited number of buffers,
giving all queuing features one would expect from a live source.
Comment 1 Sebastian Dröge (slomo) 2017-05-16 12:37:51 UTC
Review of attachment 351714 [details] [review]:

::: sys/decklink/gstdecklink.cpp
@@ +943,3 @@
+      pool->bufferSize = bufferSize;
+      pool->nonEmptyCalls = 0;
+      pool->buffers = gst_queue_array_new (5);

You probably want to move all this into a proper constructor for DecklinkBufferPool

@@ +985,3 @@
+    /* Put the buffer back to the pool */
+    DecklinkBufferPool * pool = *(DecklinkBufferPool **) ((uint8_t*)buffer - 128);
+    gst_queue_array_push_tail (pool->buffers, buffer);

If allocation sizes are changing, you will never release old buffers until Decommit() and also not the DecklinkBufferPool. There is no code that checks other sizes, only code that releases too many buffers in the current size.
Comment 2 Sebastian Dröge (slomo) 2017-05-17 08:59:48 UTC
Review of attachment 351714 [details] [review]:

::: sys/decklink/gstdecklink.cpp
@@ +860,3 @@
+private:
+  GMutex m_mutex;
+  GSList * m_sizesList;

If you use C++, use C++ :) std::list is a better choice here (only one allocation per node instead of two), and it will clean itself up automatically

@@ +866,3 @@
+    uint32_t bufferSize;
+    uint32_t nonEmptyCalls;
+    GstQueueArray * buffers;

Constructor here, and destructor for cleaning up

@@ +944,3 @@
+      pool->nonEmptyCalls = 0;
+      pool->buffers = gst_queue_array_new (5);
+      m_sizesList = g_slist_prepend (m_sizesList, pool);

Mutex needed here at least

@@ +985,3 @@
+    /* Put the buffer back to the pool */
+    DecklinkBufferPool * pool = *(DecklinkBufferPool **) ((uint8_t*)buffer - 128);
+    gst_queue_array_push_tail (pool->buffers, buffer);

What you could do for cleaning up is to count the number of outstanding buffers, and once that reaches 0 you get rid of everything.

Also this function needs locking
Comment 3 Georg Lippitsch 2017-06-30 14:07:46 UTC
Created attachment 354734 [details] [review]
Custom memory allocator for decklinkvideosrc

Only use one memory pool instead of separate pools for every size.

The sizes changes rarely (only when decklink mode changes), so there is really no point of making things that complicated.
Comment 4 Sebastian Dröge (slomo) 2017-07-04 06:49:25 UTC
Review of attachment 354734 [details] [review]:

Generally looks good

::: sys/decklink/gstdecklink.cpp
@@ +872,3 @@
+
+    while (gst_queue_array_get_length (m_buffers))
+    {

Code style: opening { for if/for/while on the same line, only for functions/structs in a separate line. Same for other places too in this patch

@@ +889,3 @@
+    g_mutex_init (&m_mutex);
+
+    m_buffers = gst_queue_array_new (60);

Why not a C++ vector anymore? But this also works of course

@@ +952,3 @@
+    if (gst_queue_array_get_length (m_buffers) > 0)
+    {
+      buf = (uint8_t*) gst_queue_array_pop_head (m_buffers);

Just pop() here without checking if there is an actual buffer. It will return NULL if there isn't
Comment 5 Georg Lippitsch 2017-07-07 13:41:12 UTC
Created attachment 355093 [details] [review]
Custom memory allocator for decklinkvideosrc
Comment 6 Sebastian Dröge (slomo) 2017-07-10 07:20:02 UTC
Review of attachment 355093 [details] [review]:

One final comment, then good for merging

::: sys/decklink/gstdecklink.cpp
@@ +875,3 @@
+        (uint8_t*) gst_queue_array_pop_head (m_buffers) - 128;
+      g_free (buf);
+    }

while ((buf = gst_queue_array_pop_head (m_buffers))
  g_free(buf - 128);

Is a bit shorter :)
Comment 7 Georg Lippitsch 2017-07-10 08:07:59 UTC
Created attachment 355239 [details] [review]
Custom memory allocator for decklinkvideosrc
Comment 8 Sebastian Dröge (slomo) 2017-07-11 11:27:22 UTC
commit f0c7fbb371494bbe0721ef2abdff01b0fe7c7474 (HEAD -> master, origin/master, origin/HEAD)
Author: Georg Lippitsch <glippitsch@toolsonair.com>
Date:   Fri May 12 14:39:54 2017 +0200

    decklinkvideosrc: Add custom memory allocator
    
    The default memory allocator of the decklink library allocates
    a fixed pool of buffers, and the number of buffers is unknown.
    This makes it impossible do useful queuing downstream. The new
    memory allocator can create an unlimited number of buffers,
    giving all queuing features one would expect from a live source.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=782556