Skip to content

fix(VideoManager): preserve Windows software frames - #14728

Merged
DonLakeFlyer merged 1 commit into
mavlink:masterfrom
alireza787b:agent/windows-system-memory-fallback
Jul 30, 2026
Merged

fix(VideoManager): preserve Windows software frames#14728
DonLakeFlyer merged 1 commit into
mavlink:masterfrom
alireza787b:agent/windows-system-memory-fallback

Conversation

@alireza787b

@alireza787b alireza787b commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

QGC's Windows GPU-capable sink was inserting d3d11upload ! d3d11convert ahead of every decoded frame. That also promoted software-decoded system-memory frames into the D3D path. In the reproduced JPEG case those frames rendered solid green, while QGC's existing Force CPU video path rendered the same stream correctly.

This change restores direct caps routing into qgcqvideosink:

  • native D3D11/D3D12-memory frames retain the existing zero-copy import path;
  • system-memory frames use QGC's existing CPU-copy fallback;
  • unsupported formats remain excluded by the shared Qt-renderable caps.

This is a generic Windows sink regression fix. It adds no transport-specific behavior and no new user setting.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change
  • Documentation update
  • Refactoring
  • CI/Build changes
  • Other

Testing

  • Tested locally
  • Added/updated unit tests
  • Tested with simulator (SITL)
  • Tested with hardware

The integrated candidate was manually exercised with JPEG streams on Windows, Linux, and Android. Windows reproduced the green-frame failure before this routing correction and rendered correctly through the system-memory fallback afterward.

Exact PR head 1b532926ab276969a9eefc8e15bbe1747bbc5767 passed QGC's Linux, Windows, macOS, Android, iOS, Docker, sanitizer, unit, integration, pre-commit, and CodeQL checks.

Platforms Tested

  • Linux
  • Windows
  • macOS
  • Android
  • iOS

Flight Stacks Tested

  • PX4
  • ArduPilot

Not applicable: this change is confined to decoded-frame routing and does not enter a vehicle command path.

Screenshots

Not applicable; there is no UI change.

Checklist

  • I have read the Contribution Guidelines
  • I have read the Code of Conduct
  • My code follows the project's coding standards
  • I have added tests that prove my fix works
  • New and existing unit tests pass in QGC CI

Related Issues

This is an independent regression fix extracted from draft #13594. HTTP and WebSocket JPEG source support are intentionally proposed separately.


By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).

@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 32.61%. Comparing base (f29efd3) to head (1b53292).
⚠️ Report is 213 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14728      +/-   ##
==========================================
+ Coverage   25.47%   32.61%   +7.14%     
==========================================
  Files         769      783      +14     
  Lines       65912    67513    +1601     
  Branches    30495    31263     +768     
==========================================
+ Hits        16788    22020    +5232     
+ Misses      37285    30657    -6628     
- Partials    11839    14836    +2997     
Flag Coverage Δ
unittests 32.61% <ø> (+7.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 476 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94ab3c6...1b53292. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Build Results

Platform Status

Platform Status Details
Linux Passed View
Windows Passed View
MacOS Passed View
Android Passed View

All builds passed.

Pre-commit

Check Status Details
pre-commit Failed (non-blocking) View

Pre-commit hooks: 0 passed, 0 failed, 0 skipped.

Test Results

linux-coverage-integration: 37 passed, 0 skipped
linux-coverage-unit: 114 passed, 0 skipped
linux-sanitizers-integration: 20 passed, 0 skipped
linux-sanitizers-unit: 114 passed, 0 skipped
Total: 285 passed, 0 skipped

Code Coverage

Coverage Baseline Change
69.3% 69.3% +0.0%

Artifact Sizes

Artifact Size Δ from master
QGroundControl 221.79 MB -4.44 MB (decrease)
QGroundControl-aarch64 179.35 MB -0.00 MB (decrease)
QGroundControl-installer-AMD64 137.84 MB -0.45 MB (decrease)
QGroundControl-installer-AMD64-ARM64 80.37 MB -0.20 MB (decrease)
QGroundControl-installer-ARM64 109.10 MB -0.02 MB (decrease)
QGroundControl-linux 87.02 MB -78.72 MB (decrease)
QGroundControl-mac 87.02 MB No change
QGroundControl-x86_64 191.58 MB +0.00 MB (increase)
Total size decreased by 83.82 MB

Updated: 2026-07-28 07:21:07 UTC • Commit: 1b53292 • Triggered by: Windows

@alireza787b

Copy link
Copy Markdown
Contributor Author

@HTRamsey, this focused Windows system-memory regression fix is ready for review. Exact head 1b532926ab276969a9eefc8e15bbe1747bbc5767 passed the full QGC platform, Docker, sanitizer, unit, integration, pre-commit, and CodeQL matrix. The integrated candidate reproduced solid-green software-decoded JPEG frames before the fix and rendered them correctly through QGC's existing CPU-copy fallback afterward. This PR is independent of the HTTP and WebSocket transport proposals.

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

Do you have anything like a gstreamer test stream setup which can reproduce this?

@DonLakeFlyer DonLakeFlyer added this to the Release V5.1 milestone Jul 29, 2026
@alireza787b

Copy link
Copy Markdown
Contributor Author

Yes. This PR includes a deterministic, camera-free GStreamer reproducer:

videotestsrc pattern=red num-buffers=5 ! video/x-raw,format=BGRA,width=320,height=240,framerate=30/1 ! qgcvideosinkbin gpu-zerocopy=true

On a Windows D3D11 Debug build, run it with:

QGroundControl.exe --unittest:GStreamerTest -- -v2 _testWindowsGpuSinkPreservesSoftwarePixels

The test maps the QVideoFrame delivered by the QGC sink and verifies that the center pixel remains red. It directly isolates the failing condition: a system-memory frame entering the GPU-capable sink while gpu-zerocopy=true. In the app-level Windows JPEG test, the parent routing produced a solid-green frame; enabling Force CPU rendered the same source correctly. This change keeps software frames on the existing CPU mapping fallback.

There is also a companion test, _testWindowsGpuSinkPreservesDirectD3D11Memory, which sends NV12 through d3d11upload and verifies that native D3D11Memory still reaches the sink directly.

void GStreamerTest::_testWindowsGpuSinkPreservesSoftwarePixels()
{
#if defined(Q_OS_WIN) && defined(QGC_HAS_GST_D3D11_GPU_PATH)
GStreamer::redirectGLibLogging();
QVERIFY2(GStreamer::completeInit(), "completeInit failed");
QVideoSink videoSink;
QGCQVideoSinkController* controller = nullptr;
const auto result = runPipelineThroughQVideoSink(videoSink, controller,
"video/x-raw,format=BGRA,width=320,height=240,framerate=30/1",
/*numBuffers*/ 5, /*gpuZerocopy*/ true, "pattern=red",
/*captureCenterPixel*/ true);
QVERIFY2(result.eos, qUtf8Printable(QStringLiteral("Software-frame pipeline: %1").arg(result.errorMessage)));
QTRY_VERIFY_WITH_TIMEOUT(result.frameCount > 0, 2000);
QCOMPARE(result.lastPixelFormat, QVideoFrameFormat::Format_BGRA8888);
QVERIFY2(result.centerPixelValid, "System-memory fallback frame could not be mapped for pixel verification");
QVERIFY2((result.centerPixel.red() > 200) && (result.centerPixel.green() < 50) && (result.centerPixel.blue() < 50),
qUtf8Printable(QStringLiteral("Expected a red software frame, received RGB(%1,%2,%3)")
.arg(result.centerPixel.red())
.arg(result.centerPixel.green())
.arg(result.centerPixel.blue())));
#else
QSKIP("Windows D3D11 GPU sink path is unavailable in this build");
#endif
}
void GStreamerTest::_testWindowsGpuSinkPreservesDirectD3D11Memory()
{
#if defined(Q_OS_WIN) && defined(QGC_HAS_GST_D3D11_GPU_PATH)
GStreamer::redirectGLibLogging();
QVERIFY2(GStreamer::completeInit(), "completeInit failed");
GstElementFactory* uploadFactory = gst_element_factory_find("d3d11upload");
if (!uploadFactory) {
QSKIP("d3d11upload is unavailable");
}
gst_object_unref(uploadFactory);
QVideoSink videoSink;
QGCQVideoSinkController* controller = nullptr;
auto result =
runPipelineThroughQVideoSink(videoSink, controller,
"video/x-raw,format=NV12,width=320,height=240,framerate=30/1 ! d3d11upload ! "
"video/x-raw(memory:D3D11Memory),format=NV12",
/*numBuffers*/ 5, /*gpuZerocopy*/ true, "pattern=red");
if (!result.eos && (result.errorMessage.contains(QStringLiteral("D3D"), Qt::CaseInsensitive) ||
result.errorMessage.contains(QStringLiteral("device"), Qt::CaseInsensitive))) {
QSKIP(qPrintable(QStringLiteral("D3D11 device unavailable in this runner: %1").arg(result.errorMessage)));
}
QVERIFY2(result.eos, qUtf8Printable(QStringLiteral("D3D11 pipeline: %1").arg(result.errorMessage)));
QTRY_VERIFY_WITH_TIMEOUT(result.frameCount > 0, 2000);
QCOMPARE(result.lastPixelFormat, QVideoFrameFormat::Format_NV12);
#else
QSKIP("D3D11 GPU sink path is only available in supported Windows builds");
#endif
}

I can also provide an external RTSP/JPEG recipe if that is more useful for your setup.

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

I finally got a windows build up and running and I can see the problem as well with the MockLink Camera/Video support I added a few days ago.

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

Verified that the fix works. Just doing code review now...

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a Windows-specific regression in QGroundControl’s GStreamer GPU sink bin where the pipeline was unconditionally inserting d3d11upload ! d3d11convert, which could incorrectly promote system-memory (software-decoded) frames into the D3D path and produce green frames. The change restores direct caps-driven routing into qgcqvideosink, preserving zero-copy for native D3D frames while keeping the existing CPU fallback for system-memory frames.

Changes:

  • Simplified the Windows GPU path wiring to link capsfilter → qgcqvideosink directly (no forced D3D11 upload/convert stage).
  • Updated/extended GStreamer unit tests to assert Windows GPU bin element composition and to validate both system-memory fallback rendering and direct D3D11 memory handling.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/VideoManager/VideoReceiver/GStreamer/gstqgc/gstqgcvideosinkbin.cc Removes forced d3d11upload ! d3d11convert insertion and restores direct caps-based routing into qgcqvideosink.
test/VideoManager/GStreamer/GStreamerTest.h Adds declarations for two new Windows GPU sink regression tests.
test/VideoManager/GStreamer/GStreamerTest.cc Adds skip stubs for the new tests when GStreamer is not enabled.
test/VideoManager/GStreamer/gstqgc/GStreamerGstQgcTest.cc Updates GPU-bin structure assertions for Windows and adds two new Windows-specific validation tests (software/system-memory fallback + direct D3D11Memory path).

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

Note: This review was generated by a Claude Fable (AI) code review.

Concerns

  1. Lost 4:4:4/packed normalization. The deleted comment explicitly documented that d3d11convert normalized non-Qt-renderable D3D11 formats (e.g. 4:4:4/packed) to a sink format. With it gone, a HW decoder emitting a D3D11Memory format not in advertisedFormatList() can't be converted inside the bin and must renegotiate down to a system-memory download. Rare (d3d11 decoders mostly emit NV12/P010), but the trade-off should be acknowledged in the code comment, and it would be good to know whether 4:2:2/4:4:4 HW-decoded streams were tested.

  2. Inert QTRY_VERIFY_WITH_TIMEOUT in both new tests. result is a by-value return that can no longer change after runPipelineThroughQVideoSink() returns, so QTRY_VERIFY_WITH_TIMEOUT(result.frameCount > 0, 2000) just burns 2 s before failing. A plain QVERIFY would be clearer.

  3. Unused pattern=red in _testWindowsGpuSinkPreservesDirectD3D11Memory. The pattern is passed but captureCenterPixel is false, so nothing verifies it — either drop the arg or verify the pixel.

  4. Overly broad skip heuristic in the D3D11 test. errorMessage.contains("device", Qt::CaseInsensitive) will silently skip any genuine failure whose message happens to mention "device". Matching "d3d11" instead would be safer.

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

@alireza787b Can you look at the Claude review?

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

@alireza787b I'm going to merge this an you can deal with the code review after the fact. I want to mark stable RC1 today and this pull does work and is certainly better than without it!

@DonLakeFlyer
DonLakeFlyer merged commit ed09a05 into mavlink:master Jul 30, 2026
48 checks passed
@alireza787b

Copy link
Copy Markdown
Contributor Author

Thanks for reproducing and verifying the Windows fallback before merge. The requested post-merge comment and test cleanup is now isolated in #14752. It does not change runtime behavior and explicitly avoids claiming separate hardware-decoder coverage for 4:2:2 or 4:4:4 streams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants