libs/base/idlesrc: cleanup & add support for shared thread pool#45
Open
camilo-celis wants to merge 62 commits into
Open
libs/base/idlesrc: cleanup & add support for shared thread pool#45camilo-celis wants to merge 62 commits into
camilo-celis wants to merge 62 commits into
Conversation
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
5 times, most recently
from
af6d75d to
d72b517
Compare
camilo-celis
changed the title
camilo-celis
commented
Oct 30, 2025
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
bb611d6 to
e98b8a4
Compare
…caps object, as we only push events or buffers to it
Similar to the basesrc, buffer allocation could be a featured that is implemented on derived classes
Buffer allocation, and pushing is driven by derived classes Remove up until we have an use for it
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
96f62d8 to
56fda68
Compare
…op for internal queue
GQueue is just { GList *head; GList *tail; guint length; }, so copying it out and re-initializing the source detaches the whole list in O(1) without freeing/reallocating any GList nodes.
** Why this is safe? **
* GQueue has no pointer-to-self or heap-owned envelope; it's a value type whose contents are entirely in the head/tail GList chain. A struct copy moves ownership of the chain wholesale.
* g_queue_init() resets head/tail to NULL and length to 0 — equivalent to a freshly declared GQueue drained = G_QUEUE_INIT;. After the swap, the source queue is genuinely empty; any subsequent g_queue_push_tail() call from a late producer behaves correctly (it starts a new list).
* The drained GQueue is now a stack-local, owned entirely by this thread. The pop_head loop that follows still walks the list — but we have to walk it anyway to unref each GstMiniObject. The win is that we don't also free + re-allocate every GList node along the way (which the old pop_head + push_tail pair did via g_slice_*).
** Does it matter? **
Per stop(), with N queued objects the old code did 2·N g_slice_free/g_slice_alloc calls just to move the chain from one queue to another.
The new version does zero! yes, ZERO! For a typical stop with a small queue it's noise; for stress tests that submit thousands of buffers and then tear down (or a pipeline with hundreds of idlesrc instances stopping concurrently) the difference is measurable and — more importantly — there's no longer a transient pressure spike on the slice allocator while holding OBJECT_LOCK.
The producer-side running check in submit_buffer*() is TOCTOU vs. stop(). Re-check after acquiring the pool ref, before pushing — joining any leftover prev_handle still happens unconditionally so we don't leak a worker
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
subprojects/gstreamer/libs/gst/base/gstbaseidlesrc.c:1537
- gst_base_idle_src_check_pending_segment() reads and mutates segment/segment_pending/segment_seqnum without holding GST_OBJECT_LOCK, even though these fields are documented as MT-protected. submit_buffer*() can be called from arbitrary threads, so this creates data races and could emit a segment event with partially-updated state. Snapshot the segment+seqnum under GST_OBJECT_LOCK, clear the pending flag under the same lock, then create/queue the segment event after unlocking.
static void
gst_base_idle_src_check_pending_segment (GstBaseIdleSrc * src)
{
GST_DEBUG_OBJECT (src, "Checking pending segment");
/* push events to close/start our segment before we push the buffer. */
95f6fcd to
49893db
Compare
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
6f5b4b6 to
040b254
Compare
finalize() drains priv->obj_queue without taking GST_OBJECT_LOCK(), but the worker/producers mutate the same queue under that lock (e.g. via gst_base_idle_src_queue_object()). If the element is finalized while a task is still running, this can race and corrupt the queue / crash. Join/drain via gst_base_idle_src_drain_and_join() before freeing the queue. After this small refactor: * We drop the old "If we still hold a handle, drop it on the originating pool" block because drain_and_join() already handles that under the lock. * The g_assert makes the invariant explicit. * The pop-loop now runs after drain_and_join() and is purely defensive. * g_atomic_int_set (&src->running, FALSE) before drain_and_join() ensures any racing producer's submit_buffer*() will bail in its initial running check rather than queueing into a soon-to-be-freed queue.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.