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
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
ad20784 to
1728e8c
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates GstBaseIdleSrc (gst-base) to support using a shared GstTaskPool for scheduling queued buffer/event pushes, and expands the accompanying unit tests to cover buffer lists, event emission, and thread-pool behavior.
Changes:
- Add
GstTaskPoolintegration toGstBaseIdleSrc, including newset/get_thread_pool()APIs and using the pool for submission processing. - Refactor/expand
GstBaseIdleSrcdocumentation and allocation vfunc wiring. - Extend
baseidlesrccheck tests with new cases for buffer lists, events, and thread-pool usage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| subprojects/gstreamer/libs/gst/base/gstbaseidlesrc.c | Introduces task-pool based scheduling, adds thread-pool APIs, refactors allocation/defaults, and updates documentation. |
| subprojects/gstreamer/libs/gst/base/gstbaseidlesrc.h | Updates class vfuncs and public API declarations (alloc vfunc docs + thread-pool API). |
| subprojects/gstreamer/tests/check/libs/baseidlesrc.c | Adds new unit tests for buffer list submission, event handling, and thread-pool set/get + stress submission. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
2 times, most recently
from
46e77c8 to
e7978a1
Compare
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
96f62d8 to
56fda68
Compare
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
priv->thread_handle may be NULL (e.g. if _stop runs right after _start's wait=TRUE join). Most pool implementations treat NULL defensively but it's documented as "the same value returned by gst_task_pool_push", so passing NULL is technically out-of-contract and GstSharedTaskPool does deref it.
… and set gst_base_idle_src_start_task reads and writes priv->thread_pool and priv->thread_handle without holding OBJECT_LOCK, but set_thread_pool and finalize mutate both under the lock (and stop also reads them lock-free). Since producers call submit_buffer → start_task from arbitrary threads while the application can call set_thread_pool concurrently, there's a real data race + UAF window: * Thread A is in start_task, has just read priv->thread_pool into a local register, but hasn't called gst_task_pool_push yet. * Thread B calls set_thread_pool, swaps the pointer, unrefs the previous pool — the last reference goes to zero and the pool is finalized. * Thread A now calls gst_task_pool_push on a freed object.
The stop is the only code path touching priv fields without the object lock.
The if (!src->running) early-return in submit_buffer is a TOCTOU check that does
not save us from races/corruption:
```
Producer thread Stop thread
--------------- -----------
submit_buffer():
if (!src->running) // FALSE src->running = FALSE
g_queue_pop_head (obj_queue) ─┐
queue_object(): │
OBJECT_LOCK; │
g_queue_push_tail (obj_queue); <- concurrent mutation ────────┘
→ GQueue list corruption / UAF
start_task():
priv->thread_handle = handle; src->priv->thread_handle = NULL
→ missed join, leaked handle
```
Possible outcomes:
* Queue corruption / double-free / UAF.
* Missed join → leaked task handle (and possibly the worker still running past finalize → use-after-free on src).
* gst_task_pool_join racing with set_thread_pool.
Locking-order note:
stop() is called with STREAM_LOCK held (via pad deactivation), so taking OBJECT_LOCK here is fine in terms of order.
Joining the worker is the operation that must happen without OBJECT_LOCK, because the worker takes it itself.
…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
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.
camilo-celis
force-pushed
the
camilo/baseidlesrc-fixups
branch
from
ba450c4 to
5dd2f60
Compare
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.