GNOME Bugzilla – Bug 782556
decklinkvideosrc: Implement a custom memory allocator
Last modified: 2017-07-11 11:27:42 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.
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.
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
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.
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
Created attachment 355093 [details] [review] Custom memory allocator for decklinkvideosrc
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 :)
Created attachment 355239 [details] [review] Custom memory allocator for decklinkvideosrc
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