Skip to content

VideoManager: Remove unused/incorrect override - #14589

Closed
DonLakeFlyer wants to merge 1 commit into
mavlink:masterfrom
DonLakeFlyer:fix-videomanager-incorrect-override
Closed

VideoManager: Remove unused/incorrect override#14589
DonLakeFlyer wants to merge 1 commit into
mavlink:masterfrom
DonLakeFlyer:fix-videomanager-incorrect-override

Conversation

@DonLakeFlyer

Copy link
Copy Markdown
Contributor

Removes the unused/incorrect isDmaBuf() const override from GstDmaBufVideoBuffer. The method has no callers anywhere in the codebase.

This replaces #14365, which had become unmergeable after the GStreamer backend overhaul (#... "Video: overhaul GStreamer backend and packaging") relocated GstDmaBufVideoBuffer.h into the new HwBuffers/dmabuf/ directory. This branch re-applies the same one-line removal at the file's new location on top of current master.

Original author: @Ryanf55

Copilot AI review requested due to automatic review settings July 6, 2026 15:20
@github-actions github-actions Bot added the Video label Jul 6, 2026
@DonLakeFlyer DonLakeFlyer added this to the Release V5.1 milestone Jul 6, 2026

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

Removes an unused and incorrect override (isDmaBuf() const) from GstDmaBufVideoBuffer, addressing a compilation failure caused by a method being marked override when the base class (GstHwVideoBuffer) does not declare it.

Changes:

  • Delete bool isDmaBuf() const override { return true; } from GstDmaBufVideoBuffer.
  • Leaves the DMABuf path identification to the existing storageTag() override.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14589      +/-   ##
==========================================
+ Coverage   25.47%   30.60%   +5.13%     
==========================================
  Files         769      785      +16     
  Lines       65912    66778     +866     
  Branches    30495    30940     +445     
==========================================
+ Hits        16788    20435    +3647     
+ Misses      37285    32423    -4862     
- Partials    11839    13920    +2081     
Flag Coverage Δ
unittests 30.60% <ø> (+5.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 437 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 1e2d58e...f29affa. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Jul 6, 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: 25 passed, 0 skipped
linux-coverage-unit: 103 passed, 0 skipped
Total: 128 passed, 0 skipped

Code Coverage

Coverage: 66.2%

No baseline available for comparison

Artifact Sizes

Artifact Size
QGroundControl 221.84 MB
QGroundControl-aarch64 179.43 MB
QGroundControl-installer-AMD64 137.93 MB
QGroundControl-installer-AMD64-ARM64 80.45 MB
QGroundControl-installer-ARM64 109.20 MB
QGroundControl-linux 87.09 MB
QGroundControl-mac 87.08 MB
QGroundControl-windows 86.14 MB
QGroundControl-x86_64 191.64 MB
No baseline available for comparison

Updated: 2026-07-06 19:00:33 UTC • Commit: f29affa • Triggered by: Android

Signed-off-by: Ryan Friedman <25047695+Ryanf55@users.noreply.github.com>
@DonLakeFlyer
DonLakeFlyer force-pushed the fix-videomanager-incorrect-override branch from 60eeaa9 to f29affa Compare July 6, 2026 17:37
@HTRamsey

HTRamsey commented Jul 6, 2026

Copy link
Copy Markdown
Member

So that function does actually get used internally in Qt but at least in Qt 6.10 it was only used in QtWidgets. Something to keep an eye on if we remove this

@DonLakeFlyer

Copy link
Copy Markdown
Contributor Author

If it ever comes back it will fail compile right?

@HTRamsey

HTRamsey commented Jul 6, 2026

Copy link
Copy Markdown
Member

Nah there's a default virtual that return false. It's used to determine video texture fallback policy in qvideotexturehelper
image

image

@DonLakeFlyer

Copy link
Copy Markdown
Contributor Author

Oh, ick. So this is a GST version problem in that Ubuntu 22 has an older GST? I originally thought this was just a compile time thing on an override that wasn't actually implemented. What's the way to deal with this? How does Qt run on Ubuntu 22 if this virtual is missing?

@HTRamsey

HTRamsey commented Jul 6, 2026

Copy link
Copy Markdown
Member

This is entirely qt based and unrelated to gstreamer, since it applies to all video backends that qt supports. I guess the best solution would be to handle the error in cmake or with compile defs

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.

4 participants