Video: GStreamer source-factory extraction, thread-safety, iOS xcframework-only#14350
Open
HTRamsey wants to merge 1 commit into
Open
Video: GStreamer source-factory extraction, thread-safety, iOS xcframework-only#14350HTRamsey wants to merge 1 commit into
HTRamsey wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors and hardens QGroundControl’s GStreamer video receiver stack by extracting source-bin construction into a dedicated factory, improving cross-thread safety around pipeline teardown and GPU context bridging, adding diagnostics/perf toggles, and simplifying iOS GStreamer SDK discovery to xcframework-only (≥ 1.28).
Changes:
- Extracted
GstSourceFactoryto centralize RTSP/UDP/TCP source-bin construction and reduceGstVideoReceivercomplexity. - Improved safety and observability: pipeline DOT dumps on ERROR, bounded EOS waits on stop, appsink negotiation diagnostics, and safer D3D bridge priming via a render-thread QRhi device snapshot.
- Simplified iOS SDK discovery in CMake to require
GStreamer.xcframework(GStreamer ≥ 1.28) and removed the legacy framework/shim paths.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/QGCRhiCapture.h | Adds a thread-safe native device snapshot API for cross-thread GPU bridging. |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/QGCRhiCapture.cc | Populates/clears the snapshot on scene graph init/invalidate using atomics. |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstDmaBufVideoBuffer.cc | Adds an env-var opt-out to skip the linear-buffer mmap “fence” stall. |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstD3DContextBridgeCommon.h | Switches backend gating to snapshot-based checks (no QRhi cross-thread reads). |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstD3DContextBridgeCommon.cc | Implements snapshot backend checks and adjusts adapter-match logging. |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstD3D12ContextBridge.cc | Primes D3D12 using snapshot LUID (render-thread captured). |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstD3D11ContextBridge.cc | Primes D3D11 using snapshot device pointer + LUID (render-thread captured). |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstContextBridgeRegistry.h | Adds handle-based registration/unregistration + dedup semantics. |
| src/VideoManager/VideoReceiver/GStreamer/HwBuffers/GstContextBridgeRegistry.cc | Implements dedup + freed-slot reuse and unregister APIs. |
| src/VideoManager/VideoReceiver/GStreamer/GstVideoReceiver.h | Adds _pipelineMutex + _acquirePipelineRef() to reduce stop/bus callback races. |
| src/VideoManager/VideoReceiver/GStreamer/GstVideoReceiver.cc | Uses GstSourceFactory, adds bounded EOS wait and DOT dump on ERROR with fallback path. |
| src/VideoManager/VideoReceiver/GStreamer/GstSourceFactory.h | New source-bin factory interface and jitter-buffer policy enum. |
| src/VideoManager/VideoReceiver/GStreamer/GstSourceFactory.cc | New implementation for RTSP/UDP/TCP source-bin building. |
| src/VideoManager/VideoReceiver/GStreamer/gstqgc/gstqgcvideosinkbin.cc | Narrows sink template to video/x-raw(ANY) and adds optional DMA_DRM LINEAR offer. |
| src/VideoManager/VideoReceiver/GStreamer/GstAppSinkAdapter.h | Exposes negotiated allocator/format/features/resolution as Q_PROPERTY diagnostics. |
| src/VideoManager/VideoReceiver/GStreamer/GstAppSinkAdapter.cc | Captures caps negotiation details on caps changes and emits negotiationChanged. |
| src/VideoManager/VideoReceiver/GStreamer/CMakeLists.txt | Adds GstSourceFactory.{h,cc} to the build. |
| cmake/find-modules/GStreamer/gst_ios_init.m.in | Removes the legacy iOS GStreamer init shim template. |
| cmake/find-modules/FindQGCGStreamer.cmake | Makes iOS discovery xcframework-only (≥1.28) and removes legacy framework paths. |
Comments suppressed due to low confidence (1)
cmake/find-modules/FindQGCGStreamer.cmake:468
- _qgc_discover_ios_sdk now ignores a user-supplied GStreamer_ROOT_DIR: if GStreamer_ROOT_DIR is set (and valid) but /Library/Developer/.../GStreamer.xcframework is not present, the code still enters the auto-download branch because the condition is only “if(NOT DEFINED _gst_ios_system_xcfw)”. This breaks the documented ability to point at a custom/bundled iOS SDK. Consider restoring the previous guard (only download when GStreamer_ROOT_DIR is not defined or doesn’t exist) and, when GStreamer_ROOT_DIR is provided, derive _gst_ios_system_xcfw from it instead of forcing a download.
# ── System install ────────────────────────────────────────────────────────
if(NOT DEFINED GStreamer_ROOT_DIR
AND EXISTS "/Library/Developer/GStreamer/iPhone.sdk/GStreamer.xcframework")
set(_gst_ios_system_xcfw "/Library/Developer/GStreamer/iPhone.sdk/GStreamer.xcframework")
endif()
# ── Auto-download / expand ────────────────────────────────────────────────
if(NOT DEFINED _gst_ios_system_xcfw)
if(CPM_SOURCE_CACHE)
set(_gst_ios_cache_dir "${CPM_SOURCE_CACHE}/gstreamer-ios-${GStreamer_FIND_VERSION}")
else()
set(_gst_ios_cache_dir "${CMAKE_BINARY_DIR}/_deps/gstreamer-ios-${GStreamer_FIND_VERSION}")
endif()
set(_gst_ios_expanded "${_gst_ios_cache_dir}/expanded")
Comment on lines
+18
to
+25
| /// Opaque slot handle returned by register*; pass to unregister* to clear that slot. | ||
| /// kInvalidHandle indicates the registry was full or the handler was already registered. | ||
| using RegistrationHandle = int; | ||
| constexpr RegistrationHandle kInvalidHandle = -1; | ||
|
|
||
| /// Register a bridge handler. Called from static initializers before any pipeline starts. | ||
| void registerBridgeHandler(BridgeHandler handler); | ||
| /// Duplicate registrations of the same handler pointer are ignored (returns the existing slot). | ||
| RegistrationHandle registerBridgeHandler(BridgeHandler handler); |
Comment on lines
+33
to
+41
| /// True if QGCRhiCapture's atomic snapshot has been populated and matches @p expectedBackend. | ||
| /// Reads atomic fields only — safe from the bus-sync thread. @p backendName is purely for | ||
| /// logging ("D3D11" / "D3D12"). Returns false if the snapshot isn't ready yet (caller should | ||
| /// retry on next NEED_CONTEXT) or if the backend is wrong (caller logs once via | ||
| /// @p state.warnedWrongBackend). | ||
| bool checkSnapshotBackend(BridgeState &state, | ||
| const QLoggingCategory &cat, | ||
| int expectedBackend, | ||
| const char *backendName); |
| unsigned(info.vendorId), unsigned(info.deviceId), | ||
| int(info.deviceType), static_cast<long long>(expectedLuid)); | ||
| << apiName << "bridge adapter LUID=" | ||
| << QString::asprintf("0x%llx", static_cast<long long>(expectedLuid)); |
Comment on lines
+43
to
+44
| /// Returns the global snapshot. Atomic fields make individual reads thread-safe. | ||
| DeviceSnapshot &deviceSnapshot() noexcept; |
|
|
||
| #include <QtCore/qglobal.h> | ||
| #include <QtCore/QObject> | ||
| #include <QtCore/QString> |
Comment on lines
+400
to
+406
| // Lock before nulling so an in-flight _onBusMessage on the streaming thread cannot read | ||
| // a half-destroyed _pipeline. _acquirePipelineRef takes its own ref under the same lock. | ||
| { | ||
| QMutexLocker lock(&_pipelineMutex); | ||
| gst_clear_object(&_pipeline); | ||
| _pipeline = nullptr; | ||
| } |
| [[maybe_unused]] static const bool dotDirHinted = []() { | ||
| if (qgetenv("GST_DEBUG_DUMP_DOT_DIR").isEmpty()) { | ||
| qCInfo(GstVideoReceiverLog).noquote() | ||
| << "Pipeline dot-graph dumps are disabled. Set GST_DEBUG_DUMP_DOT_DIR=/path/to/dir to enable."; |
| GstObject *parent = gst_object_get_parent(GST_OBJECT(element)); | ||
| if (parent) { | ||
| if (!gst_element_add_pad(GST_ELEMENT(parent), ghostpad)) { | ||
| qCCritical(GstSourceFactoryLog) << "gst_element_add_pad() failed"; |
HTRamsey
force-pushed
the
video/gst-source-factory
branch
from
612c23f to
abe7c23
Compare
Contributor
Build ResultsPlatform Status
Some builds failed. Pre-commit
Pre-commit hooks: 4 passed, 44 failed, 7 skipped. Test Resultslinux-coverage: 98 passed, 0 skipped Code Coverage
Artifact Sizes
Updated: 2026-05-10 22:04:17 UTC • Triggered by: Linux |
HTRamsey
force-pushed
the
video/gst-source-factory
branch
from
abe7c23 to
948418e
Compare
github-actions
Bot
added
Platform: Android
github_actions
Move source-bin construction (rtspsrc / tcpclientsrc / udpsrc + parsebin
wrapping, MPEG-TS demux, optional rtpjitterbuffer) and its four pad-glue
helpers (filterParserCaps, padProbe, linkPad, wrapWithGhostPad) out of
GstVideoReceiver into GStreamer::SourceFactory. The receiver now calls
SourceFactory::create(uri, jitterBuffer); the factory owns transport
dispatch, the receiver owns pipeline lifecycle.
Drive-by fixes in the extracted code:
- Use-after-free in error paths after gst_bin_add_many: locals are
nulled and non-owning aliases (upstream / binParser / demux / jitter)
carry through, so the unconditional gst_clear_object cleanup never
double-unrefs bin-owned children.
- Validate host/port for tcpclientsrc and udpsrc; previously a missing
port (-1) silently narrowed to 65535.
- udp:// URI rebuild via QUrl(setScheme/setUserInfo) + toEncoded()
preserves query params (multicast-iface, etc.); the old format-string
reconstruction dropped them.
- URI scheme detection via QUrl::scheme() instead of QString::contains;
unknown schemes now log qCWarning and fail fast.
- RTSP credentials via QUrl::userName/password (FullyDecoded) instead
of manual indexOf(':') split, so percent-encoded passwords work.
- Typed jitter-buffer policy: enum class JitterBuffer { None,
DropOnLatency, Buffered } replaces the overloaded int (-1/0/>0).
HTRamsey
force-pushed
the
video/gst-source-factory
branch
from
948418e to
411a6c7
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.
Summary
Single-commit PR consolidating GStreamer-side cleanup on
video/gst-source-factory.Code (
src/VideoManager/VideoReceiver/GStreamer/)GstSourceFactoryfromGstVideoReceiver— RTSP/UDP/TCP/file source construction now lives in its own translation unit (GstSourceFactory.{h,cc})._pipelineMutex+_acquirePipelineRef()so bus sync-message handlers and worker-thread callbacks can't race againststop().gst_bus_timed_pop_filterednow caps at 3 s instead ofGST_CLOCK_TIME_NONE.QGCRhiCapture::DeviceSnapshot(atomic backend / device pointers / adapter LUID) on the render thread; D3D11/D3D12 context bridges read the snapshot instead of dereferencingQRhi*from the GStreamer streaming thread.GstContextBridgeRegistry: handle-based registration with pointer-dedup and freed-slot reuse; supportsunregister*.gstqgcvideosinkbinadvertisesvideo/x-raw(ANY);QGC_GST_OFFER_DMA_DRM_LINEARopt-in for explicit LINEAR DMA_DRM caps.QGC_GST_DMABUF_NO_MMAP_FENCEopt-out skips the per-frame mmap fence on linear buffers.GstAppSinkAdapterexposes negotiated allocator/format/features/resolution asQ_PROPERTYs; auto DOT-dump on pipeline ERROR (capped at 10 files inQStandardPaths::CacheLocation/qgc-pipeline-dot/).Build (
cmake/find-modules/)GStreamer.xcframework; the legacy.frameworklayout is gone. Removed:_qgc_discover_ios_sdk.frameworkwalk + fallback (~145 LOC)/Library/Developer/GStreamer/iPhone.sdk/GStreamer.frameworksystem pathfind_package(GStreamerMobile)gategst_ios_init.m.inshim (101 LOC)GStreamer ≥ 1.28.0.FindQGCGStreamer.cmake1243 → 1112 LOC.Test plan
ios-arm64_x86_64-simulatorslice)