ci: add build deb#4
Conversation
Use libuvc timestamps for setting more accurate PTS/DTS
de-jitter our PTSes to avoid skipped and duplicated frames
- Add build-check.yml: runs on PR/push to main, builds for arm64/amd64 - Replace publish-deb.yml with publish-release.yml - Manual workflow_dispatch trigger (was push/release) - CalVer versioning with stable/beta channels - Builds .deb packages and .tar.gz archives for both architectures - GPG signs APT repository metadata - Uploads to Cloudflare R2 - Creates GitHub release with all artifacts and checksums
- Remove hardcoded ARM64 platform constraints from Dockerfile - Add TARGETARCH build arg for architecture-agnostic builds - Map TARGETARCH to GNU triplets (aarch64-linux-gnu, x86_64-linux-gnu) - Update workflows to use native runners (ubuntu-24.04-arm64 for ARM64) - Remove QEMU/emulation complexity from CI workflows - Use matrix.config.runner and matrix.config.arch pattern This enables faster builds using native ARM64 GitHub runners instead of slow QEMU emulation on x86_64.
GitHub Actions ARM64 runners use 'ubuntu-24.04-arm' not 'ubuntu-24.04-arm64'. This fixes the stuck/queued builds.
Catch-up merge of irlserver/gstlibuvch264src:main (9 commits, base 45df046) into CERALIVE/main. Brings in the pipeline-clock timestamping rework, libuvc_control socket + video control + zoom (canonical b11a18b), H265 UVC support, the unistd.h fix, and the switch from bundled libuvc to upstream v0.0.7 built with the bundled patches/. Conflict resolution: - libuvch264src/src/*.c,*.h: took irlserver (canonical functional changes; supersedes CERALIVE's rough 45ef3b4/ecee7c5 video-control commits). - Dockerfile (add/add): irlserver's upstream-libuvc build stage + CERALIVE's multiarch model (TARGETARCH->triplet). Plugin and libuvc relocated under /usr/lib/<triplet>/ (preserving file names) so build-check finds build/usr/lib/*/gstreamer-1.0/ and publish-release packages build/usr/=/usr/. - .github/workflows: kept CERALIVE's build-check.yml + publish-release.yml; dropped irlserver's publish-deb.yml (parallel IRL-branded CI infra). - libuvc/ bundled tree deleted (replaced by patches/ + upstream clone). Also pass -DCMAKE_POLICY_VERSION_MINIMUM=3.5 to the libuvc cmake invocation: ubuntu:latest is now 26.04 with CMake 4.x, which rejects libuvc v0.0.7's cmake_minimum_required(VERSION 3.1). Verified the amd64 image builds end to end and installs build/usr/lib/x86_64-linux-gnu/gstreamer-1.0/libgstlibuvch264src.so.
* chore: add .clang-format
* test(gstlibuvch264src): add plugin-load smoke test
What
Add a hardware-independent gst-check smoke test that verifies the
libuvch264src plugin loads and its element registers correctly, wired as a
ctest target.
Why
The plugin had no automated test coverage. A load/registration smoke test
catches symbol/registration regressions (renamed factories, dropped
properties, broken pad templates) without needing a UVC capture device.
How
- tests/test_plugin_load.c: gst-check suite asserting the plugin registers,
both factories (libuvch264src + libuvch26xsrc alias) exist, the element is
a GstPushSrc, the "index" string property defaults to "0", and the ALWAYS
"src" pad template advertises video/x-h264.
- Top-level CMakeLists.txt (TEST-ONLY): compiles the plugin from its
unmodified source and vendors libuvc v0.0.7 + patches via FetchContent
(mirroring the Dockerfile), then exposes the test via ctest. The canonical
production build stays Meson; runtime behavior is unchanged.
- tests/CMakeLists.txt: registers the ctest target, isolating GST_PLUGIN_PATH
/registry and resolving the vendored libuvc at dlopen time.
- CI: new smoke-test job in build-check.yml runs
`cmake -B build && cmake --build build && ctest --test-dir build`.
- Doc: AGENTS.md TEST section + structure refresh.
How to verify
cmake -B build && cmake --build build && ctest --test-dir build --output-on-failure
-> 1/1 passed (5 gst-check assertions). Negative control with an empty
GST_PLUGIN_PATH fails all 5, proving the test is not vacuous.
Risks
Low. Additive test-only build; no plugin source or Meson/Docker build change.
The CMake path clones libuvc at build time (network), matching the Dockerfile.
Document the authoritative CalVer version scheme for gstlibuvch264src. The .deb version is derived purely from git tags at publish time via the publish-release.yml workflow. There is no separate VERSION file by design. Scheme: YYYY.MINOR.PATCH - YYYY = current year (UTC) - MINOR = current month (UTC, no zero-pad) - PATCH = monotonic counter per month Example: 2026.6.1 (June 2026, patch 1) Tag format: - Stable: v<VERSION> (e.g., v2026.6.1) - Beta: v<VERSION>-beta.<N> (e.g., v2026.6.2-beta.1) The VERSION env var from calculate-version is passed directly to FPM's -v flag, producing .deb packages with CalVer versions like: gstreamer1.0-libuvch264src_2026.6.1_arm64.deb Single source of truth: git tag namespace (v*). Verified: build-check gate green (plugin-load smoke test passed).
…rror on zero devices
Untrusted NAL lengths from parse_nal_units could exceed the fixed 1024 B vps/sps/pps instance buffers. Both the live frame_callback path and the disk-cache load path memcpy'd straight into them, overflowing the GObject allocation and corrupting adjacent fds/mutex/pointers. Clamp every copy: drop any NAL with len <= 0 or len > SPSPPSBUFSZ with a GST_WARNING instead of copying. Add an ASan regression test driven by the mock feeder's oversized-SPS mode (sps_bounds + sps_bounds_asan); the mock payload is enlarged so the unclamped overflow crosses the allocation boundary where ASan's redzone lives.
… (capability-gated)
…ward-compatible) Parse the index property once at start() into a tagged selector and match one enumerated device: - "N" ordinal into the device list (unchanged; cerastream passes i.to_string()) - "vid:pid" hex vendor:product id - "serial:<sn>" exact USB serial-number string - "bus:<b>:<a>" decimal USB bus number and device address Malformed selectors still fail loudly with RESOURCE/SETTINGS; a selector that matches no enumerated device is RESOURCE/NOT_FOUND (never a silent fallback to device 0). The mock gains per-device descriptors plus an opened-index probe, and test_device_select covers all four forms, the ordinal backward-compat path, and the no-match cases.
feat(uvc): robustness, reliability & multi-system hardening (v2026.6.2)
merge the hardening work from the downstream ceralive fork: module split (frame_pipeline, spspps_cache, ptz_control, uvc_device, error), a mock-backed ctest suite (tsan/asan variants), 19 memory-safety and correctness fixes (sps/pps heap-overflow clamp, usb teardown double-close, nal 3-byte start codes, device-list leak, strict index parsing, framerate guard, pts monotonicity), native pan/tilt/zoom properties, richer device selection (vid:pid / serial / bus), an opt-in hardened control socket under $XDG_RUNTIME_DIR (mode 0600), latency query, and a v4l2 capability probe. adaptations on merge: - keep our publish-deb release ci; drop the fork's publish-release workflow - pin plugin version to 2.0 (our scheme, not calver) - control socket PAN_TILT/ZOOM now accept normalized floats in [-1,1] mapped onto the device range, matching the irl control client - de-brand docs/comments to product-neutral wording; credit ceralive verified: meson build loads (version 2.0), full ctest suite 36/36 pass.
WalkthroughThis PR delivers the Changesgstlibuvch264src 2.0 Hardening Release
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
64-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the runtime image as non-root by default.
Line 64 starts the runtime stage, but no
USERis set, so the final container runs as root. This increases impact if the process or plugin is compromised.🔧 Suggested hardening patch
FROM ubuntu:24.04@sha256:786a8b558f7be160c6c8c4a54f9a57274f3b4fb1491cf65146521ae77ff1dc54 AS runtime @@ RUN apt-get update && apt-get install -y \ libgstreamer1.0-0 \ libgstreamer-plugins-base1.0-0 \ libusb-1.0-0 \ && rm -rf /var/lib/apt/lists/* @@ RUN GNUARCH=$(cat /tmp/gnuarch) && \ mv /tmp/stage/libgstlibuvch264src.so /usr/lib/${GNUARCH}/gstreamer-1.0/ && \ mv /tmp/stage/libuvc.* /usr/lib/${GNUARCH}/ && \ rm -rf /tmp/stage /tmp/gnuarch + +# Drop root privileges for runtime execution. +RUN groupadd -r gst && useradd -r -g gst -d /nonexistent -s /usr/sbin/nologin gst +USER gst🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 64 - 97, The runtime stage in the Dockerfile does not specify a non-root user, causing the final container to run as root which increases security risk. Add a USER directive at the end of the runtime stage (after the final RUN command that moves the GStreamer plugin and libuvc libraries) to specify a non-root user account. This ensures the container runs with reduced privileges by default.Source: Linters/SAST tools
🧹 Nitpick comments (3)
libuvch264src/src/frame_pipeline.c (1)
248-258: 💤 Low valueMinor:
offsetvariable isintwhile used withgsizebuffer operations.The local
offsetvariable at line 249 is declared asint, but it's used alongsidegsizevalues for buffer fill positions. WhileSPSPPSBUFSZbounds keep the combined offset well underINT_MAX, usinggsizewould be more consistent with the rest of the function.Suggested fix
- int offset = 0; + gsize offset = 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libuvch264src/src/frame_pipeline.c` around lines 248 - 258, The offset variable is declared as int but is used with GStreamer buffer operations that expect gsize types. Change the declaration of the offset variable from int to gsize to ensure consistency with the gst_buffer_fill function calls that follow. This aligns with GStreamer API conventions where buffer positions and sizes use gsize rather than int.tests/test_lifecycle.c (1)
122-129: ⚡ Quick winPrefer explicit state convergence checks over fixed sleep in restart cycles.
Using a fixed 100ms delay makes this regression test timing-sensitive. Waiting for state completion each cycle (
gst_element_get_statewith bounded timeout) gives deterministic coverage of the mutex lifecycle path.Refactor sketch
for (int i = 0; i < cycles; i++) { fail_unless (gst_element_set_state (pipeline, GST_STATE_PLAYING) != GST_STATE_CHANGE_FAILURE, "cycle %d: could not set PLAYING", i); - /* Let start() bring the control thread fully up before tearing it down. */ - g_usleep (100 * G_TIME_SPAN_MILLISECOND); + fail_unless (gst_element_get_state (pipeline, NULL, NULL, 5 * GST_SECOND) + != GST_STATE_CHANGE_FAILURE, "cycle %d: PLAYING did not converge", i); fail_unless (gst_element_set_state (pipeline, GST_STATE_NULL) != GST_STATE_CHANGE_FAILURE, "cycle %d: could not set NULL", i); + fail_unless (gst_element_get_state (pipeline, NULL, NULL, 5 * GST_SECOND) + != GST_STATE_CHANGE_FAILURE, "cycle %d: NULL did not converge", i); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_lifecycle.c` around lines 122 - 129, The restart cycle loop uses a fixed 100ms sleep delay with g_usleep to wait for the control thread to complete state setup before transitioning to NULL, which makes the test timing-sensitive and non-deterministic. Replace the g_usleep call with an explicit state convergence check using gst_element_get_state with a bounded timeout after setting the pipeline to PLAYING state. This ensures the test waits for the actual state change to complete before proceeding to the NULL state transition, providing deterministic coverage of the mutex lifecycle path without relying on arbitrary sleep durations..github/workflows/build-check.yml (1)
34-37: ⚡ Quick winVerify the plugin artifact directly, not just the directory.
The current check can pass when the directory exists but the plugin
.sois absent or renamed. Assert the expected artifact path explicitly.✅ Stronger artifact assertion
- name: Verify plugin was built run: | - ls -la build/usr/lib/*/gstreamer-1.0/ + ls -la build/usr/lib/*/gstreamer-1.0/ + test -f build/usr/lib/*/gstreamer-1.0/libgstlibuvch264src.so🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-check.yml around lines 34 - 37, The "Verify plugin was built" step currently only lists directory contents with ls, which allows the check to pass even if the actual plugin .so file is missing or has been renamed. Replace the ls command with an explicit test assertion that verifies the expected plugin artifact file exists at its full path (e.g., using a test command like [ -f ] or similar to assert the presence of the .so file in the build/usr/lib/*/gstreamer-1.0/ directory). This ensures the verification actually confirms the plugin binary was successfully created.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-check.yml:
- Around line 21-22: The actions/checkout@v4 steps are missing the
persist-credentials configuration, which allows token-backed git credentials to
remain in the workspace and creates a security risk. At
.github/workflows/build-check.yml lines 21-22 (anchor) and lines 49-50
(sibling), add persist-credentials: false to the with section of each
actions/checkout@v4 step to ensure credentials are not persisted after the
checkout completes.
- Around line 1-9: The GitHub Actions workflow in build-check.yml is using
default token permissions instead of explicitly declaring minimal,
least-privilege permissions. Add a permissions block at the workflow level
(after the on section and before the jobs section) to explicitly set minimal
read-only permissions. For a build check workflow, this typically means setting
permissions to read-only access for repository contents or using an empty
permissions object if no special token permissions are required.
- Line 22: Replace all mutable GitHub Actions version tags with exact
40-character commit SHAs in the build-check.yml workflow file. For each uses
statement at lines 22, 25, 28, and 50, change the reference from the format
actions/[owner]/[action]`@v`[version] to
actions/[owner]/[action]@[full-40-char-commit-sha]. This pins the actions to
immutable commit references, protecting against supply-chain attacks and
upstream action substitution.
In `@libuvch264src/src/frame_pipeline.c`:
- Around line 19-25: The H.265 frame type mapping in the switch statement within
the UVC_FRAME_FORMAT_H265 conditional block is incomplete. Currently only case 1
maps to UNIT_FRAME_NON_IDR and case 20 maps to UNIT_FRAME_IDR, while all other
cases implicitly return UNIT_INVALID, causing frame loss. Extend the switch
cases to handle all H.265 non-IDR slice types 0–9 by adding multiple case labels
(case 0 through case 9) that all return UNIT_FRAME_NON_IDR. Additionally,
evaluate whether H.265 recovery point types 16–18 (BLA_W_LP, BLA_W_RADL,
BLA_N_LP) should be handled as IDR frames or a separate recovery point type
depending on your application requirements. Verify that UVC cameras in your
target deployment emit slice types beyond 1 and 20 to ensure the complete
mapping prevents frame loss.
In `@libuvch264src/src/spspps_path.h`:
- Around line 43-45: When the HOME environment variable is not set and home_dir
becomes NULL, the current code at lines 43-45 unsafely falls back to an empty
string, which causes subsequent path construction at lines 49 and 61-62 to build
paths under root-level directories like /.spspps. Instead of allowing this
fallback, implement fail-closed behavior by returning early from the function or
setting an error state at lines 43-45 when home_dir is NULL, preventing the
function from continuing and avoiding any root-level cache writes at lines 49
and 61-62.
In `@libuvch264src/src/uvc_device.c`:
- Around line 88-97: The comment in the UVC device code states the intent is to
"Reattach detached interfaces to the kernel" but the implementation calls
libusb_detach_kernel_driver() which does the opposite. To align the code with
the stated goal of restoring kernel control over the device, replace the
libusb_detach_kernel_driver() call with libusb_attach_kernel_driver() to
actually reattach the kernel driver to each interface. The surrounding loop
logic with libusb_kernel_driver_active() will then correctly identify which
interfaces need reattachment and restore them to a usable state for other
consumers.
In `@tests/mock_libuvc.c`:
- Around line 408-415: Add a NULL check immediately after the calloc call in the
mock_new_device function to guard against allocation failures. Before
dereferencing dev with dev->ctx, dev->index, or dev->refcount, verify that
calloc did not return NULL and return NULL early if allocation failed.
Additionally, the caller loop at line 462 must be updated to handle the case
where mock_new_device returns NULL, ensuring the test does not attempt to use a
NULL device pointer.
In `@tests/test_error_map.c`:
- Around line 32-35: Replace the call to gst_bus_pop_filtered() with
gst_bus_timed_pop_filtered() and provide a reasonable timeout value (in
milliseconds) as the second parameter. This change prevents the test from
hanging indefinitely if the GST_MESSAGE_ERROR fails to be posted to the bus for
any reason, allowing the test to fail promptly and enabling proper teardown
instead of waiting for an external timeout.
In `@tests/test_functional.c`:
- Around line 122-138: Move the pipeline teardown calls
(gst_element_set_state(pipeline, GST_STATE_NULL) and gst_object_unref(pipeline))
to execute before the fail_unless() assertion checks in both affected files. In
tests/test_functional.c (lines 122-138), relocate the state-change and unref
calls from after the assertions to immediately after the gst_message_unref(msg)
and gst_object_unref(bus) calls and before the fail_unless() checks. Apply the
same reordering in tests/test_mock_smoke.c (lines 75-92) to ensure cleanup
completes before any longjmp from a failed assertion can occur, preventing the
streaming pipeline from remaining active and causing resource leaks.
In `@tests/test_socket.c`:
- Around line 169-170: The g_str_has_prefix checks in the underA and underB
variable assignments are vulnerable to false positives with sibling directory
names (e.g., /tmp/run matching /tmp/run2). For each g_str_has_prefix call, add
an additional boundary-safety check to ensure the matched prefix is followed by
a path separator (/) or is at the end of the string. This prevents the function
from incorrectly matching partial directory names that happen to share a common
prefix.
In `@tests/test_sps_bounds.c`:
- Around line 101-109: The error branch calls fail() before cleaning up
resources like the pipeline and bus, which skips teardown and can cause hanging
or flaky behavior. Restructure the error handling in the GST_MESSAGE_ERROR
branch (where gst_message_parse_error is called) to ensure that any pipeline and
bus unreffing/cleanup operations occur before the fail() call, so that shutdown
happens even when the test fails on error.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 64-97: The runtime stage in the Dockerfile does not specify a
non-root user, causing the final container to run as root which increases
security risk. Add a USER directive at the end of the runtime stage (after the
final RUN command that moves the GStreamer plugin and libuvc libraries) to
specify a non-root user account. This ensures the container runs with reduced
privileges by default.
---
Nitpick comments:
In @.github/workflows/build-check.yml:
- Around line 34-37: The "Verify plugin was built" step currently only lists
directory contents with ls, which allows the check to pass even if the actual
plugin .so file is missing or has been renamed. Replace the ls command with an
explicit test assertion that verifies the expected plugin artifact file exists
at its full path (e.g., using a test command like [ -f ] or similar to assert
the presence of the .so file in the build/usr/lib/*/gstreamer-1.0/ directory).
This ensures the verification actually confirms the plugin binary was
successfully created.
In `@libuvch264src/src/frame_pipeline.c`:
- Around line 248-258: The offset variable is declared as int but is used with
GStreamer buffer operations that expect gsize types. Change the declaration of
the offset variable from int to gsize to ensure consistency with the
gst_buffer_fill function calls that follow. This aligns with GStreamer API
conventions where buffer positions and sizes use gsize rather than int.
In `@tests/test_lifecycle.c`:
- Around line 122-129: The restart cycle loop uses a fixed 100ms sleep delay
with g_usleep to wait for the control thread to complete state setup before
transitioning to NULL, which makes the test timing-sensitive and
non-deterministic. Replace the g_usleep call with an explicit state convergence
check using gst_element_get_state with a bounded timeout after setting the
pipeline to PLAYING state. This ensures the test waits for the actual state
change to complete before proceeding to the NULL state transition, providing
deterministic coverage of the mutex lifecycle path without relying on arbitrary
sleep durations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e710c8e-3faa-41b1-95f0-0375de39a59f
📒 Files selected for processing (50)
.clang-format.github/workflows/build-check.yml.gitignoreCHANGELOG.mdCMakeLists.txtDockerfileREADME.mdlibuvch264src/docs/notes/reconnect-spike.mdlibuvch264src/meson.buildlibuvch264src/src/frame_pipeline.clibuvch264src/src/frame_pipeline.hlibuvch264src/src/gstlibuvch264src.clibuvch264src/src/gstlibuvch264src.hlibuvch264src/src/gstlibuvch264src_error.clibuvch264src/src/gstlibuvch264src_error.hlibuvch264src/src/gstlibuvch264src_internal.hlibuvch264src/src/meson.buildlibuvch264src/src/ptz_control.clibuvch264src/src/ptz_control.hlibuvch264src/src/spspps_cache.clibuvch264src/src/spspps_cache.hlibuvch264src/src/spspps_path.hlibuvch264src/src/uvc_device.clibuvch264src/src/uvc_device.htests/CMakeLists.txttests/lsan_negotiate.suppressionstests/mock_libusb.ctests/mock_libusb.htests/mock_libuvc.ctests/mock_libuvc.htests/test_cache.ctests/test_device_select.ctests/test_error_map.ctests/test_functional.ctests/test_lifecycle.ctests/test_live_source.ctests/test_mock_smoke.ctests/test_nal_parse.ctests/test_negotiate.ctests/test_plugin_load.ctests/test_pts_monotonic.ctests/test_pts_thread_safety.ctests/test_ptz.ctests/test_reconnect.ctests/test_socket.ctests/test_sps_bounds.ctests/test_usb_teardown.ctests/test_v4l2_probe.ctests/tsan.suppressionstests/tsan_pts.suppressions
| name: Build Check | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| pull_request: | ||
| branches: [main] | ||
|
|
||
| jobs: |
There was a problem hiding this comment.
Set least-privilege token permissions explicitly.
The workflow currently inherits default token permissions; declare minimal read-only permissions at workflow (or per-job) scope.
🔒 Suggested hardening
name: Build Check
on:
push:
branches: [main]
pull_request:
branches: [main]
+
+permissions:
+ contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Build Check | |
| on: | |
| push: | |
| branches: [main] | |
| pull_request: | |
| branches: [main] | |
| jobs: | |
| name: Build Check | |
| on: | |
| push: | |
| branches: [main] | |
| pull_request: | |
| branches: [main] | |
| permissions: | |
| contents: read | |
| jobs: |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 1-83: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-check.yml around lines 1 - 9, The GitHub Actions
workflow in build-check.yml is using default token permissions instead of
explicitly declaring minimal, least-privilege permissions. Add a permissions
block at the workflow level (after the on section and before the jobs section)
to explicitly set minimal read-only permissions. For a build check workflow,
this typically means setting permissions to read-only access for repository
contents or using an empty permissions object if no special token permissions
are required.
Source: Linters/SAST tools
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Disable credential persistence in checkout steps.
actions/checkout should set persist-credentials: false to avoid leaving token-backed git credentials in the workspace config.
🔐 Suggested change
- name: Checkout code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false
@@
- name: Checkout code
uses: actions/checkout@v4
+ with:
+ persist-credentials: falseAlso applies to: 49-50
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 21-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-check.yml around lines 21 - 22, The
actions/checkout@v4 steps are missing the persist-credentials configuration,
which allows token-backed git credentials to remain in the workspace and creates
a security risk. At .github/workflows/build-check.yml lines 21-22 (anchor) and
lines 49-50 (sibling), add persist-credentials: false to the with section of
each actions/checkout@v4 step to ensure credentials are not persisted after the
checkout completes.
Source: Linters/SAST tools
| runner: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect no output after pinning to 40-char SHAs.
rg -nP 'uses:\s*[^@]+@(?![0-9a-fA-F]{40}\b)' .github/workflows/build-check.ymlRepository: irlserver/gstlibuvch264src
Length of output: 236
Pin GitHub Actions to immutable commit SHAs instead of version tags.
Using mutable tags (@v*) exposes the workflow to upstream action substitution and supply-chain attacks. Pin all uses: statements to exact 40-character commit SHAs.
Applies to lines: 22, 25, 28, 50
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 21-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-check.yml at line 22, Replace all mutable GitHub
Actions version tags with exact 40-character commit SHAs in the build-check.yml
workflow file. For each uses statement at lines 22, 25, 28, and 50, change the
reference from the format actions/[owner]/[action]`@v`[version] to
actions/[owner]/[action]@[full-40-char-commit-sha]. This pins the actions to
immutable commit references, protecting against supply-chain attacks and
upstream action substitution.
Source: Linters/SAST tools
| } else if (format == UVC_FRAME_FORMAT_H265) { | ||
| switch (type) { | ||
| case 1: | ||
| return UNIT_FRAME_NON_IDR; | ||
| case 20: | ||
| return UNIT_FRAME_IDR; | ||
| case 32: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are tests or documentation mentioning H.265 NAL type handling
rg -n -C2 'HEVC|H\.265|h265' --type=cRepository: irlserver/gstlibuvch264src
Length of output: 8885
🏁 Script executed:
# Check the convert_unit_type function and understand how unmapped types are handled
sed -n '1,40p' libuvch264src/src/frame_pipeline.c | cat -nRepository: irlserver/gstlibuvch264src
Length of output: 1468
🏁 Script executed:
# Check what happens when an unmapped NAL type is encountered
rg -n 'UNIT_INVALID|UNIT_FRAME' libuvch264src/src/frame_pipeline.c | head -20Repository: irlserver/gstlibuvch264src
Length of output: 482
🏁 Script executed:
# Look at frame_callback to see how it handles UNIT_INVALID
sed -n '128,170p' libuvch264src/src/frame_pipeline.c | cat -nRepository: irlserver/gstlibuvch264src
Length of output: 2180
🌐 Web query:
H.265 HEVC NAL unit types slice types 0-21 specification
💡 Result:
In the H.265/HEVC (High Efficiency Video Coding) standard, Network Abstraction Layer (NAL) unit types are defined in Table 7-1 of the ITU-T H.265 specification [1][2][3]. The values 0 through 21 correspond specifically to Video Coding Layer (VCL) NAL units, which contain the actual slice segment data [4][5]. The NAL unit type codes 0-21 are specified as follows [2][3][4]: 0: TRAIL_N (Trailing, non-TSA, non-STSA) 1: TRAIL_R (Trailing, REAP) 2: TSA_N (Temporal sub-layer access, non-TSA) 3: TSA_R (Temporal sub-layer access) 4: STSA_N (Step-wise temporal sub-layer access, non-STSA) 5: STSA_R (Step-wise temporal sub-layer access) 6: RADL_N (Random access decodable leading, non-RADL) 7: RADL_R (Random access decodable leading) 8: RASL_N (Random access skipped leading, non-RASL) 9: RASL_R (Random access skipped leading) 10: RSV_VCL_N10 (Reserved VCL NAL unit type) 11: RSV_VCL_R11 (Reserved VCL NAL unit type) 12: RSV_VCL_N12 (Reserved VCL NAL unit type) 13: RSV_VCL_R13 (Reserved VCL NAL unit type) 14: RSV_VCL_N14 (Reserved VCL NAL unit type) 15: RSV_VCL_R15 (Reserved VCL NAL unit type) 16: BLA_W_LP (Broken link access with leading pictures) 17: BLA_W_RADL (Broken link access with RADL pictures) 18: BLA_N_LP (Broken link access with no leading pictures) 19: IDR_W_RADL (Instantaneous decoding refresh with RADL pictures) 20: IDR_N_LP (Instantaneous decoding refresh with no leading pictures) 21: CRA_NUT (Clean random access) These types categorize the NAL units based on their role in the decoding process, such as whether they are leading pictures, random access points (like IDR or CRA), or standard trailing pictures [5]. Note that "slice type" is a distinct concept within the slice segment header (e.g., I, P, or B slices) and should not be confused with the NAL unit type [5][6].
Citations:
- 1: https://www.itu.int/rec/dologin_pub.asp?id=T-REC-H.265-202309-S%21%21PDF-E&lang=s&type=items
- 2: https://www.ffmpeg.org/doxygen/7.1/libavcodec_2hevc_2hevc_8h.html
- 3: https://ffmpeg.org/doxygen/7.1/libavcodec_2hevc_2hevc_8h_source.html
- 4: https://github.com/strukturag/libde265/blob/master/libde265/nal.h
- 5: https://biblio.telecom-paristech.fr/cgi-bin/download.cgi?id=13870
- 6: https://www.ffmpeg.org/doxygen/7.0/structSliceHeader.html
H.265 non-IDR slice type mapping is incomplete and should handle all slice types 0–9 and potentially types 16–18.
H.265 defines non-IDR slice NAL types 0–9 (TRAIL_N, TRAIL_R, TSA_N, TSA_R, STSA_N, STSA_R, RADL_N, RADL_R, RASL_N, RASL_R) and IDR-like types 16–18 (BLA_W_LP, BLA_W_RADL, BLA_N_LP) per ITU-T H.265. Currently only type 1 maps to UNIT_FRAME_NON_IDR; all others return UNIT_INVALID and are silently dropped in frame_callback().
The mapping should be extended to at minimum:
case 0: case 1: case 2: case 3: case 4: case 5: case 6: case 7: case 8: case 9:
return UNIT_FRAME_NON_IDR;Types 16–18 (BLA_LP, BLA_RADL, BLA_N_LP) may also warrant handling as recovery point slices depending on the application requirements (they function similarly to IDR frames in random access scenarios). Consider whether these should map to UNIT_FRAME_IDR or a separate recovery point type.
Verify whether UVC cameras in the target deployment emit H.265 slice types beyond 1 and 20; if they do, the incomplete mapping will cause frame loss.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libuvch264src/src/frame_pipeline.c` around lines 19 - 25, The H.265 frame
type mapping in the switch statement within the UVC_FRAME_FORMAT_H265
conditional block is incomplete. Currently only case 1 maps to
UNIT_FRAME_NON_IDR and case 20 maps to UNIT_FRAME_IDR, while all other cases
implicitly return UNIT_INVALID, causing frame loss. Extend the switch cases to
handle all H.265 non-IDR slice types 0–9 by adding multiple case labels (case 0
through case 9) that all return UNIT_FRAME_NON_IDR. Additionally, evaluate
whether H.265 recovery point types 16–18 (BLA_W_LP, BLA_W_RADL, BLA_N_LP) should
be handled as IDR frames or a separate recovery point type depending on your
application requirements. Verify that UVC cameras in your target deployment emit
slice types beyond 1 and 20 to ensure the complete mapping prevents frame loss.
| if (home_dir == NULL) { | ||
| home_dir = ""; | ||
| } |
There was a problem hiding this comment.
Fail closed when HOME is missing to avoid root-level cache writes.
Line 43 currently falls back to home_dir = "", and Lines 49/61 then build paths under /.spspps. If HOME is unset, cache I/O can target a global root path instead of user scope.
Suggested fix
- if (home_dir == NULL) {
- home_dir = "";
- }
+ if (home_dir == NULL || home_dir[0] == '\0') {
+ return -1;
+ }Also applies to: 49-49, 61-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libuvch264src/src/spspps_path.h` around lines 43 - 45, When the HOME
environment variable is not set and home_dir becomes NULL, the current code at
lines 43-45 unsafely falls back to an empty string, which causes subsequent path
construction at lines 49 and 61-62 to build paths under root-level directories
like /.spspps. Instead of allowing this fallback, implement fail-closed behavior
by returning early from the function or setting an error state at lines 43-45
when home_dir is NULL, preventing the function from continuing and avoiding any
root-level cache writes at lines 49 and 61-62.
| static uvc_device_t *mock_new_device(uvc_context_t *ctx, int index) { | ||
| uvc_device_t *dev = calloc(1, sizeof(*dev)); | ||
| dev->ctx = ctx; | ||
| dev->index = index; | ||
| dev->refcount = 1; | ||
| if (ctx->device_count < MOCK_MAX_DEVICES) | ||
| ctx->devices[ctx->device_count++] = dev; | ||
| return dev; |
There was a problem hiding this comment.
Guard allocation failure before dereferencing mock device pointers.
Line 410 dereferences dev immediately after calloc without a NULL check. On OOM, this can crash the test process, and the caller loop at Line 462 currently assumes allocation always succeeded.
🛠️ Proposed fix
static uvc_device_t *mock_new_device(uvc_context_t *ctx, int index) {
uvc_device_t *dev = calloc(1, sizeof(*dev));
+ if (!dev)
+ return NULL;
dev->ctx = ctx;
dev->index = index;
dev->refcount = 1;
if (ctx->device_count < MOCK_MAX_DEVICES)
ctx->devices[ctx->device_count++] = dev;
return dev;
}
@@
for (int i = 0; i < n; i++)
- list[i] = mock_new_device(ctx, i);
+ {
+ list[i] = mock_new_device(ctx, i);
+ if (!list[i]) {
+ free(list);
+ *devs = NULL;
+ return UVC_ERROR_NO_MEM;
+ }
+ }Also applies to: 461-463
🧰 Tools
🪛 Cppcheck (2.20.0)
[warning] 410-410: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 411-411: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 412-412: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/mock_libuvc.c` around lines 408 - 415, Add a NULL check immediately
after the calloc call in the mock_new_device function to guard against
allocation failures. Before dereferencing dev with dev->ctx, dev->index, or
dev->refcount, verify that calloc did not return NULL and return NULL early if
allocation failed. Additionally, the caller loop at line 462 must be updated to
handle the case where mock_new_device returns NULL, ensuring the test does not
attempt to use a NULL device pointer.
Source: Linters/SAST tools
| GstMessage *msg = gst_bus_pop_filtered (bus, GST_MESSAGE_ERROR); | ||
| fail_unless (msg != NULL, | ||
| "expected a GST_MESSAGE_ERROR on the bus (uvc_error_t=%d, disconnect=%d)", | ||
| err, disconnect); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "test_error_map.c" -type fRepository: irlserver/gstlibuvch264src
Length of output: 95
🏁 Script executed:
head -50 tests/test_error_map.cRepository: irlserver/gstlibuvch264src
Length of output: 2211
🌐 Web query:
GStreamer gst_bus_pop_filtered gst_bus_timed_pop_filtered API documentation
💡 Result:
In GStreamer, gst_bus_pop_filtered and gst_bus_timed_pop_filtered are functions used to retrieve specific types of messages from a GstBus while discarding non-matching messages [1][2][3]. gst_bus_pop_filtered This function retrieves the next message from the bus that matches the provided types mask [1][4]. Any messages posted before the first matching message that do not match the mask are discarded [1][5]. If no message matches the type, all existing messages are discarded, and it returns NULL [1][4]. The returned message must be unreferenced using gst_message_unref after use [4][5]. gst_bus_timed_pop_filtered This function acts similarly to gst_bus_pop_filtered but includes a timeout mechanism [2][3]. It waits up to the specified timeout (in nanoseconds) for a matching message, while discarding non-matching messages encountered during the wait [2][6]. Key behaviors: - If timeout is 0, the function behaves identically to gst_bus_pop_filtered [2][3]. - If timeout is GST_CLOCK_TIME_NONE, the function will block indefinitely until a matching message is posted [2][6]. - If no matching message is found before the timeout expires, it returns NULL [2][6]. - Similar to the non-timed version, the returned message must be unreferenced using gst_message_unref [2][6]. For most applications, GStreamer developers recommend using an asynchronous bus watch rather than these synchronous polling functions, except for specific cases involving short timeouts [1][4][2].
Citations:
- 1: https://gstreamer.freedesktop.org/documentation/gstreamer/gstbus.html?gi-language=c
- 2: https://www.manpagez.com/html/gstreamer-1.0/gstreamer-1.0-1.11.1/GstBus.php
- 3: https://people.freedesktop.org/~tsaunier/documentation/gstreamer/gstbus.html
- 4: http://www.manpagez.com/html/gstreamer-1.0/gstreamer-1.0-1.14.2/GstBus.php
- 5: https://www.manpagez.com/html/gstreamer-1.0/gstreamer-1.0-1.14.0/GstBus.php
- 6: https://api.gtkd.org/gstreamer.Bus.Bus.timedPopFiltered.html
🏁 Script executed:
cat -n tests/test_error_map.c | sed -n '20,60p'Repository: irlserver/gstlibuvch264src
Length of output: 1911
🏁 Script executed:
rg -A 10 "gst_libuvc_h264_src_post_error\|gst_libuvc_h264_src_post_disconnect_error" --type cRepository: irlserver/gstlibuvch264src
Length of output: 52
🏁 Script executed:
fd -type f -name "*.c" -o -name "*.h" | xargs rg -l "gst_libuvc_h264_src_post_error|gst_libuvc_h264_src_post_disconnect_error" | head -5Repository: irlserver/gstlibuvch264src
Length of output: 421
🏁 Script executed:
find . -name "*error*" -type f | grep -E '\.(c|h)$'Repository: irlserver/gstlibuvch264src
Length of output: 185
🏁 Script executed:
cat -n libuvch264src/src/gstlibuvch264src_error.cRepository: irlserver/gstlibuvch264src
Length of output: 2098
🏁 Script executed:
cat -n libuvch264src/src/gstlibuvch264src_error.hRepository: irlserver/gstlibuvch264src
Length of output: 1344
Replace gst_bus_pop_filtered() with timed variant to prevent indefinite test hangs.
The test posts an error message and immediately pops it from the bus. However, gst_bus_pop_filtered() blocks indefinitely if the message isn't found—if error posting fails silently for any reason, the test hangs until external timeout rather than failing quickly. Use gst_bus_timed_pop_filtered() with a reasonable timeout to ensure test failures surface promptly and teardown runs.
Proposed fix
- GstMessage *msg = gst_bus_pop_filtered (bus, GST_MESSAGE_ERROR);
+ GstMessage *msg =
+ gst_bus_timed_pop_filtered (bus, 2 * GST_SECOND, GST_MESSAGE_ERROR);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GstMessage *msg = gst_bus_pop_filtered (bus, GST_MESSAGE_ERROR); | |
| fail_unless (msg != NULL, | |
| "expected a GST_MESSAGE_ERROR on the bus (uvc_error_t=%d, disconnect=%d)", | |
| err, disconnect); | |
| GstMessage *msg = | |
| gst_bus_timed_pop_filtered (bus, 2 * GST_SECOND, GST_MESSAGE_ERROR); | |
| fail_unless (msg != NULL, | |
| "expected a GST_MESSAGE_ERROR on the bus (uvc_error_t=%d, disconnect=%d)", | |
| err, disconnect); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_error_map.c` around lines 32 - 35, Replace the call to
gst_bus_pop_filtered() with gst_bus_timed_pop_filtered() and provide a
reasonable timeout value (in milliseconds) as the second parameter. This change
prevents the test from hanging indefinitely if the GST_MESSAGE_ERROR fails to be
posted to the bus for any reason, allowing the test to fail promptly and
enabling proper teardown instead of waiting for an external timeout.
| if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) { | ||
| GError *gerr = NULL; | ||
| gchar *dbg = NULL; | ||
| gst_message_parse_error (msg, &gerr, &dbg); | ||
| fail ("pipeline errored instead of reaching EOS: %s (%s)", | ||
| gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)"); | ||
| g_clear_error (&gerr); | ||
| g_free (dbg); | ||
| } | ||
| fail_unless (msg != NULL, "timed out waiting for EOS"); | ||
| fail_unless (GST_MESSAGE_TYPE (msg) == GST_MESSAGE_EOS, | ||
| "expected EOS, got %s", GST_MESSAGE_TYPE_NAME (msg)); | ||
| gst_message_unref (msg); | ||
| gst_object_unref (bus); | ||
|
|
||
| gst_element_set_state (pipeline, GST_STATE_NULL); | ||
| gst_object_unref (pipeline); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the actual code from test_functional.c at lines 122-138
sed -n '122,138p' tests/test_functional.cRepository: irlserver/gstlibuvch264src
Length of output: 727
🏁 Script executed:
# Read the actual code from test_mock_smoke.c at lines 75-92
sed -n '75,92p' tests/test_mock_smoke.cRepository: irlserver/gstlibuvch264src
Length of output: 768
🏁 Script executed:
# Check what fail() and fail_unless() do - likely from a test framework header
find . -name "*.h" -o -name "*.c" | xargs grep -l "define fail\|^fail(" | head -5Repository: irlserver/gstlibuvch264src
Length of output: 52
🏁 Script executed:
# Look for other similar cleanup patterns in test files to understand common practice
rg "gst_element_set_state.*GST_STATE_NULL" tests/ -A 2 -B 10 | head -100Repository: irlserver/gstlibuvch264src
Length of output: 5658
Move pipeline teardown before EOS/error assertion checks to prevent resource leaks when assertions fail.
In both tests/test_functional.c (lines 122–138) and tests/test_mock_smoke.c (lines 75–92), fail_unless() calls execute before gst_element_set_state(pipeline, GST_STATE_NULL) and gst_object_unref(pipeline). When assertions fail, fail_unless() longjmps past the cleanup code, leaving the streaming pipeline active and preventing proper resource teardown. Other test files in this repository (test_usb_teardown.c, test_reconnect.c) explicitly document and follow the correct pattern: capture outcome details, execute cleanup, then assert.
Move the state-change and unref calls before the fail_unless() checks in both files.
📍 Affects 2 files
tests/test_functional.c#L122-L138(this comment)tests/test_mock_smoke.c#L75-L92
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_functional.c` around lines 122 - 138, Move the pipeline teardown
calls (gst_element_set_state(pipeline, GST_STATE_NULL) and
gst_object_unref(pipeline)) to execute before the fail_unless() assertion checks
in both affected files. In tests/test_functional.c (lines 122-138), relocate the
state-change and unref calls from after the assertions to immediately after the
gst_message_unref(msg) and gst_object_unref(bus) calls and before the
fail_unless() checks. Apply the same reordering in tests/test_mock_smoke.c
(lines 75-92) to ensure cleanup completes before any longjmp from a failed
assertion can occur, preventing the streaming pipeline from remaining active and
causing resource leaks.
| gboolean underA = (pathA != NULL) && g_str_has_prefix (pathA, runtime); | ||
| gboolean underB = (pathB != NULL) && g_str_has_prefix (pathB, runtime); |
There was a problem hiding this comment.
Make runtime-dir containment check boundary-safe.
g_str_has_prefix(path, runtime) alone can false-pass for sibling prefixes (e.g., /tmp/run vs /tmp/run2). Add a separator/end boundary check.
🧪 Tighten assertion
- gboolean underA = (pathA != NULL) && g_str_has_prefix (pathA, runtime);
- gboolean underB = (pathB != NULL) && g_str_has_prefix (pathB, runtime);
+ gsize rlen = strlen (runtime);
+ gboolean underA = (pathA != NULL) && g_str_has_prefix (pathA, runtime)
+ && (pathA[rlen] == G_DIR_SEPARATOR || pathA[rlen] == '\0');
+ gboolean underB = (pathB != NULL) && g_str_has_prefix (pathB, runtime)
+ && (pathB[rlen] == G_DIR_SEPARATOR || pathB[rlen] == '\0');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gboolean underA = (pathA != NULL) && g_str_has_prefix (pathA, runtime); | |
| gboolean underB = (pathB != NULL) && g_str_has_prefix (pathB, runtime); | |
| gsize rlen = strlen (runtime); | |
| gboolean underA = (pathA != NULL) && g_str_has_prefix (pathA, runtime) | |
| && (pathA[rlen] == G_DIR_SEPARATOR || pathA[rlen] == '\0'); | |
| gboolean underB = (pathB != NULL) && g_str_has_prefix (pathB, runtime) | |
| && (pathB[rlen] == G_DIR_SEPARATOR || pathB[rlen] == '\0'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_socket.c` around lines 169 - 170, The g_str_has_prefix checks in
the underA and underB variable assignments are vulnerable to false positives
with sibling directory names (e.g., /tmp/run matching /tmp/run2). For each
g_str_has_prefix call, add an additional boundary-safety check to ensure the
matched prefix is followed by a path separator (/) or is at the end of the
string. This prevents the function from incorrectly matching partial directory
names that happen to share a common prefix.
| if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) { | ||
| GError *gerr = NULL; | ||
| gchar *dbg = NULL; | ||
| gst_message_parse_error (msg, &gerr, &dbg); | ||
| fail ("pipeline errored instead of reaching EOS: %s (%s)", | ||
| gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)"); | ||
| g_clear_error (&gerr); | ||
| g_free (dbg); | ||
| } |
There was a problem hiding this comment.
Avoid failing before teardown in the error branch.
This branch calls fail() before shutting down/unreffing the pipeline and bus, which can skip cleanup and cause hanging/flaky ctest behavior under non-forking execution.
🛠️ Teardown-safe pattern
- if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) {
- GError *gerr = NULL;
- gchar *dbg = NULL;
- gst_message_parse_error (msg, &gerr, &dbg);
- fail ("pipeline errored instead of reaching EOS: %s (%s)",
- gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)");
- g_clear_error (&gerr);
- g_free (dbg);
- }
+ gboolean got_error = FALSE;
+ gchar *error_text = NULL;
+ if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) {
+ GError *gerr = NULL;
+ gchar *dbg = NULL;
+ gst_message_parse_error (msg, &gerr, &dbg);
+ got_error = TRUE;
+ error_text = g_strdup_printf ("%s (%s)",
+ gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)");
+ g_clear_error (&gerr);
+ g_free (dbg);
+ }
@@
gst_element_set_state (pipeline, GST_STATE_NULL);
gst_object_unref (pipeline);
+
+ fail_unless (!got_error, "pipeline errored instead of reaching EOS: %s",
+ error_text ? error_text : "(unknown)");
+ g_free (error_text);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) { | |
| GError *gerr = NULL; | |
| gchar *dbg = NULL; | |
| gst_message_parse_error (msg, &gerr, &dbg); | |
| fail ("pipeline errored instead of reaching EOS: %s (%s)", | |
| gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)"); | |
| g_clear_error (&gerr); | |
| g_free (dbg); | |
| } | |
| gboolean got_error = FALSE; | |
| gchar *error_text = NULL; | |
| if (msg != NULL && GST_MESSAGE_TYPE (msg) == GST_MESSAGE_ERROR) { | |
| GError *gerr = NULL; | |
| gchar *dbg = NULL; | |
| gst_message_parse_error (msg, &gerr, &dbg); | |
| got_error = TRUE; | |
| error_text = g_strdup_printf ("%s (%s)", | |
| gerr ? gerr->message : "(none)", dbg ? dbg : "(no debug)"); | |
| g_clear_error (&gerr); | |
| g_free (dbg); | |
| } | |
| gst_element_set_state (pipeline, GST_STATE_NULL); | |
| gst_object_unref (pipeline); | |
| fail_unless (!got_error, "pipeline errored instead of reaching EOS: %s", | |
| error_text ? error_text : "(unknown)"); | |
| g_free (error_text); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_sps_bounds.c` around lines 101 - 109, The error branch calls
fail() before cleaning up resources like the pipeline and bus, which skips
teardown and can cause hanging or flaky behavior. Restructure the error handling
in the GST_MESSAGE_ERROR branch (where gst_message_parse_error is called) to
ensure that any pipeline and bus unreffing/cleanup operations occur before the
fail() call, so that shutdown happens even when the test fails on error.
3 participants
Summary by CodeRabbit
New Features
set-ptzaction signalBug Fixes
Documentation
Tests
Build & Infrastructure